Adding common interface implemented by Map and Vector that is mutable and iterable

[acrylic-origami]

MutableMap and MutableVector both already implement KeyedContainer and IndexAccess. The former extends KeyedTraversable, while the latter gives these classes basic mutability through set[All] and removeKey.

Currently, the only way to admit both Map and Vector to an immutable wrapper with mutable descendants is to upper-bound the collection generic type by ConstIndexAccess and refine to a mutable IndexAccess in the descendants. For example:

<?hh // strict
class ImmutableWrapper<Tk, +Tv, +TCollection as \ConstIndexAccess<Tk, Tv>> {
	public function __construct(private TCollection $collection) {}
	public function get_collection(): TCollection {
		return $this->collection;
	}
}
class MutableWrapper<Tk, Tv, +TCollection as \IndexAccess<Tk, Tv>> extends ImmutableWrapper<Tk, Tv, TCollection> {
	public function mutate(Tk $k, Tv $v): void {
		$this->get_collection()->set($k, $v); // for example
	}
}

However, without iterability, working with the IndexAccess family is cumbersome, and attempts to add iterability get messy. Keeping track of keys in a separate iterable collection, for example, leads quickly to generic hell, especially when the mutability of this collection is considered.

Of the Traversable descendants, I'm proposing KeyedContainer because it and Indexish are the only common ancestors of Map and Vector to define an indexing method — in this case, the square bracket syntax. Indexish is also a good candidate, but I figure that in the future, some core Hack classes might extend KeyedContainer but not Indexish, judging from Typing[4005] (the illegal-use-of-square-bracket-indexing error).

I should mention the associated issue, #7573 .

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[hhvm-bot]

@acrylic-origami updated the pull request - view changes

[hhvm-bot]

@mofarrell has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[dlreeves]

KeyedContainer was introduced to be a union of Hack Collections and PHP arrays. This requires adding special support in the runtime to have arrays behave as instances of KeyedContainer. Do you imagine arrays also being instances of MutableKeyContainer?

[acrylic-origami]

@dlreeves That's a good point. I don't include them because they don't implement IndexAccess. Functionally, they definitely fit the concept behind MutableKeyedContainer. However, there are no interfaces for square-bracket syntax for mutation.

MutableKeyedContainer would actually be in the right place to be that interface, if it ditched the extension of IndexAccess and allowed setting via square brackets behind the scenes.

For now, I figured Map and Vector was a good start for MutableKeyedContainer.

[fredemmott]

@sgolemon was thinking of this a long while back - any comments, and still have a copy of the diagram?

[dlreeves]

If it doesn't support arrays then I wonder if a mutable version of Indexish would be better. At one point we allowed Indexish to be modified with [] in the type checker, but removed it since the immutable collections also implemented this interface.

[acrylic-origami]

@dlreeves Are you proposing reviving this historic mutable Indexish implementation? I'm not sure I follow your rationale that Indexish is more appropriate because of arrays, when arrays implement Indexish as well. Could you elaborate?

Also, did this mutable Indexish include a mechanism to remove elements? That's the one functional thing IndexAccess would implement over a mutable Indexish as it stands.

[dlreeves]

@acrylic-origami - Ok took a look at the type hierarchy again. Would having IndexAccess extend Indexish solve this particularly issue for you? That way you have the mutable methods available via IndexAccess, but can access values using square-brackets. You still wouldn't be able to write using square-brackets though.

[acrylic-origami]

@dlreeves Well, Indexish can't be usefully extended by custom interfaces, but IndexAccess definitely can. So, I suggested they stay separate and are combined by a common descendant to avoid misleading upcasts of custom IndexAccesses to KeyedContainer. That way, the only and more correct option would be to upcast to ConstIndexAccess to achieve that functionality.

[mofarrell]

https://pbs.twimg.com/media/C_pNhu3XgAAibNw.jpg
^ Collections diagram

[facebook-github-bot]

@acrylic-origami has updated the pull request. Re-import the pull request