Number literal object keys typechecking #7593
Conversation
|
Thanks @mroch for this library https://github.com/flowtype/ocaml-dtoa |
|
Not sure who should review this. @dsainati1, @jbrown215, or @mvitousek, any of you have some time to spare for this? |
|
Given that @mroch had raised concerns on #380 I'd like to get his opinion on this, but at the very least I think we'd want to see more tests here. It would be good to test number keys with different variance, as well as cases where objects are declared with number/string keys that are in conflict. I.e., what should happen here? |
|
@dsainati1 it does the same as |
|
Parser test for contravariant number keys fails, probably it parses it as negative number |
|
Fixed contravariant properties parsing. |
|
|
|
@dsainati1 do you think tuples should be affected by this? //@flow
declare var e: { 0: number, 1: number, length: 2 };
declare var b: [number, number];
(e[0]: number);
(e[1]: number);
(b[0]: number);
(b[1]: number);
const m: { 0: number, 1: number, length: 2 } = b; // error currently
const m2: [number, number] = e; // error currentlyRelated PR: #5048 |
| | DefT (reason_x, _, NumT (Literal (_, (x, _)))) -> | ||
| let x = Dtoa.ecma_string_of_float x in | ||
| let reason_prop = replace_reason_const (RProperty (Some x)) reason_x in | ||
| (* Probably should be different type constructor *) |
There was a problem hiding this comment.
Looks like this broke {[key: number]: any} reads,
declare var e: { [key: number]: any };
e[0]; // string `0` [1] is incompatible with number [2].Flow(InferError)There was a problem hiding this comment.
I think this #2293 (comment) applies to this also, so probably I fixed it wrong
|
This PR probably shouldn't mix number keys with string keys |
|
@mroch can you look at this? |
|
😃I forgot about this PR completely. I think to split parsing from typechecking in two PRs |
goodmind
force-pushed
the
number-literal-keys
branch
2 times, most recently
from
89a15ca to
9b70048
Compare
|
Moved parsing here #7798 |
goodmind
changed the title
goodmind
force-pushed
the
number-literal-keys
branch
from
9b70048 to
5b9960c
Compare
goodmind
force-pushed
the
number-literal-keys
branch
from
5b9960c to
575d803
Compare
|
@nnmrts I don't think this would get in as-is, I hope at least parsing would get reviewed |
|
@goodmind So it seems you're just getting ignored here. Nice! 👍👍👍 |
|
@nnmrts not really, I have tons of PR merged already |
|
Any update on this? 😆 |
|
Really? It's almost 9 months now!? This is ridiculous. Maintainers, then open up flowlib (and js types) for developers at least, if you don't want to fix major bugs like this one. Flow is officially an unusable typechecker for 2014 javascript now. Numerical keys are standard since JUNE 2015!??? How is this even a typechecker when half of the types aren't following any standard? |
|
This pull request has been merged in 80ca16f. |
Fixes #380
Doesn't allow in interfaces
Doesn't allow in class declarations