Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Covariant length on ReadOnlyArray<.> #4266

Closed
wants to merge 3 commits into from

Conversation

popham
Copy link
Contributor

@popham popham commented Jun 28, 2017

$ReadOnlyArray shouldn't allow length setting because it can mutate the array.

@vkurchatkin
Copy link
Contributor

I personally would prefer to have length covariant even on mutable arrays

@popham
Copy link
Contributor Author

popham commented Jun 28, 2017

@vkurchatkin Sure, although I recall reading something about Flow striving toward "JavaScript as it is." Constraining length on Array seems more in line with "JavaScript as it ought to be." I personally agree with you because I prefer false negatives to false positives (#4246 (comment) was a similar instance of "JavaScript as it ought to be" in tension with "JavaScript as it is").

Can I get a "shall" directive from the Flow hive at FB? I was too lazy to look it up on the spec, but I noticed that the Node interpreter admits length assignments on strings and ignores them:

const a = "asdf"; //a.length===4
a.length = 1; //a.length===4
a.length = 7; //a.length===4

If length assignment on Array is forbidden, then it seems reasonable that it should be forbidden on String also.

@vkurchatkin
Copy link
Contributor

length mutation on arrays is definetely observable and might even be considered useful. On strings it should do nothing, so that is a no-brainier

@popham
Copy link
Contributor Author

popham commented Jun 28, 2017

Agreed. The PR is good as-is.

@nmote
Copy link
Contributor

nmote commented Jun 28, 2017

Looks reasonable to me. I'd like to see some tests to ensure that (a) length mutations are in fact banned on $ReadOnlyArrays, and that (b) length mutations are still allowed on Arrays.

@popham
Copy link
Contributor Author

popham commented Jun 28, 2017

Sure. This closes #4265 BTW.

@facebook-github-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants