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

array access inconsistency, why? #3702

Closed
gabor opened this issue Apr 11, 2017 · 6 comments
Closed

array access inconsistency, why? #3702

gabor opened this issue Apr 11, 2017 · 6 comments

Comments

@gabor
Copy link

@gabor gabor commented Apr 11, 2017

const x = [1,2,3,4];
const z1:number = x[1]; // works, undefined-case is ignored
const z2:number = x.find(elem => elem === 2); // fails

why is the possibly-undefined tolerated in the first case, and not in the second?

@kimjoar

This comment has been minimized.

Copy link

@kimjoar kimjoar commented Apr 11, 2017

@gabor

This comment has been minimized.

Copy link
Author

@gabor gabor commented Apr 11, 2017

i'm sorry, i do know that it is documented. i'd like to know why it does not apply to .find() too.

@madbence

This comment has been minimized.

Copy link

@madbence madbence commented Apr 11, 2017

array access on T[] returns T, but find on T[] returns ?T (technically it's T | void, because null is not allowed).

@gabor

This comment has been minimized.

Copy link
Author

@gabor gabor commented Apr 11, 2017

i am probably describing the question in a wrong way.

i do understand all that. find() is typed based on how it behaves, and for array-indexing the flow-team decided to create an exception.

i just don't understand the reason. in the documentation the argument is that "it would be extremely inconvenient to use", which i honestly don't understand. using flow is more inconvenient than not using it, but i still decided to use it, because it helps me to have less bugs.

also, when documenting objects-as-maps, and it's similar exeption, the documentation says that "It is the programmer’s responsibility to ensure the access is safe, as with arrays". again, the whole reason for me using flow is to have the static type system help me find bugs like getting undefined into variables which should not have undefined. and the flow-team seemed to have a similar preference, because flow contains a rich system of marking something optional, with the ?number, number|null and {attr?: number} ... but for some strange reason array-access and object-access is an exception.

i personally would prefer to get an error in such situations, after all, that is the correct description of the functionality. array indexing's return type is really ?Item.

perhaps it would help demonstrating my situation by describing how i arrived here :)

i was working on code that worked with data-structures made of arrays. something like:

type Item = { id: string, data: something };
type Structure = Array<item>;

so, as you can imagine, the code was full of const q = x.find(item => item.id === 'id1') code. and flow complained that the q might be undefined. as it should. so i had to write the code to handle the undefined-case. even if it is something like if (!item) {throw new Error('data-consistency-error)}. (this is still better than doing nothing, because this way the code will crash at the point where things went wrong, not two hours later when the undefined is used somewhere else)

but of course this kind of lookup is very slow, so later i modified the code, and converted the arrays to objects, with a structure like this:

type Item = {id: string, data: something };
type Structure2 = { [string]: Item };

and i could simplify the x.find(item => item.id === 'id1') to x['id1']. shorter and faster code, nice.
i did not remove the undefined-checks, because they seemed to be necessary. object-lookup can return undefined, after all.

later, for testing something, i commented out the undefined-check, and found that flow does not complain. and then after some internet-reading i arrived at the documentation that says this is intended behaviour.

so it would be nice if someone from the flow-time could jump in and explain how is this meant to help the developer. because i honestly don't know.

also, if anyone knows, is there some kind of way to massage flow into being strict here? :) perhaps a command-line flag, or a special datatype, like StrictArray or anything?

@madbence

This comment has been minimized.

Copy link

@madbence madbence commented Apr 11, 2017

i guess the reason behind this decision was simply to make development more convenient, but i'm sure that core developers can explain you this topic in detail.

if you use maps (regular object literals), you can enforce that only valid writes are allowed, but reads must be checked.

type Item = { id: number };
type Items = { [string]: Item };

function consume(items: { +[string]: ?Item }) {
  const item = items.foo;
  
  // item must be checked
  // item.id
  
  // writes are not permitted, because that could violate the type constaints in `Items`
  // items.bar = null;
}

function produce(): Items {
  const items: Items = {};
  items.foo = { id: 1 };
  
  // invalid writes are not permitted
  // items.bar = null;
  return items;
}

consume(produce());

https://flow.org/try/#0C4TwDgpgBAksEFsoF4oG8oEsAmAuKAdgK4IBGEATlAL4DcAUKJLPAgM4rpQDabwFmAgHMAuvjiIaDegDMiBAMbBMAewJQFatiQgAKTKzb4MAal79Bo-AH4JSagEp09KBq3AsrTgcRsAdDIqKgyuLlAA9OGekghEfFDkGgAWEAoA1hDYYZHRCH44YdlRggBuAIYANjhQAO4C8BxlFNAEKh6QFAgG8NgANEW5-qRNnMQVFQzU9LLySqrqYBQq2EQKeg7ihs6umgTxPuybvpxodGEH-oEqJ1h4UACMUoWuOaWV1XU+jc2EbVAdXWAPQGFz8wyoqDGEzCzWARAo6guk2mu20CD0i2Wq3WDloQA

@nmote

This comment has been minimized.

Copy link
Contributor

@nmote nmote commented Apr 11, 2017

As you've pointed out there is no logically consistent reason to allow Array accesses to be unsafe while being more strict with other operations. I think it just came down to a pragmatic choice based on the perception that people would be really angry if we forced them to null-check every array access. And in practice, I'm not sure how often people actually access arrays out of bounds -- I think most have at this point learned to avoid that error. So it's a tradeoff -- will we actually catch enough real bugs that it will justify the extra effort? Of course this is the very question people ask about using Flow as a whole.

Adopting Flow, while it has considerable benefits, can also be painful. There was a time when its adoption inside Facebook was not a foregone conclusion. Every barrier to entry had to be carefully considered. Now that it has become a standard part of writing JavaScript at Facebook, we might be able to get away with increasing the strictness here. It has certainly been discussed, and there is precedent (some time ago Map.get was made strict). But it would be a major change and should not be taken lightly. So far, whenever it has come up, the conclusion has been that it's not worth the tradeoff. But that could someday change.

I do not believe there is any way to change this behavior through a config option.

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.