flow icon indicating copy to clipboard operation
flow copied to clipboard

Use SingletonStrT, SingletonNumT, SingletonBoolT in case of const literal declarations

Open goodmind opened this issue 6 years ago • 18 comments

Revived https://github.com/facebook/flow/pull/4175 Fixes #2639

goodmind avatar Apr 02 '19 20:04 goodmind

@mrkev probably

goodmind avatar Apr 02 '19 21:04 goodmind

Let's check this out

mrkev avatar Apr 04 '19 19:04 mrkev

I'm seeing issues similar to this:

const foo = {
  bar: 10,
  baz(x: number) {
    foo.bar = x; // Error, number incompatible with number
  },
}

Also tagging @nmote so this PR get some visibility from someone in the team.

mrkev avatar Apr 05 '19 20:04 mrkev

@mrkev I'm not sure how this is related, because it only uses affects const primitive literals, not object literals properties, I'll check

goodmind avatar Apr 05 '19 20:04 goodmind

Maybe you mean something like this

const bar = 10;
const foo = {
  bar,
  baz(x: number) {
    foo.bar = x; // Cannot assign `x` to `foo.bar` because  number [1] is incompatible with  number
  },
}

goodmind avatar Apr 05 '19 21:04 goodmind

Yeah, totally bug, I should replace reason of DefT

goodmind avatar Apr 05 '19 21:04 goodmind

Now this errors correctly

Cannot assign `x` to `foo.bar` because  number [1] is incompatible with  number literal `10` [2]

goodmind avatar Apr 05 '19 21:04 goodmind

@mrkev do you think this shouldn't error at all and property type should be widened to number?

goodmind avatar Apr 05 '19 21:04 goodmind

It's interesting that in TypeScript if type is inferred then it's widened, but if it's specified by user it isn't

const str = 'string';
const num = 200;
const bool = true;
const str1: 'string' = 'string';
const num1: 200 = 200;
const bool1: true = true;

const foo = {
  str,
  num,
  bool,
  str1,
  num1,
  bool1,
  method() {
    foo.str = 'any string' // no error
    foo.num = 300 // no error
    foo.bool = false // no error
    foo.str1 = 'any string' // error
    foo.num1 = 300 // error
    foo.bool1 = false // error
  }
}

goodmind avatar Apr 05 '19 21:04 goodmind

I think it depends if this code is bug or not https://flow.org/try/#0MYewdgzgLgBAYgeQQLhgIkQtMC87NoDcAUMADYCGEEMAQhQE4wDexMMUAngA4Cmu8JCXYALXmTIgAFAEoWbdhxEBLCADoufAWjDLgvIgoC+xE6EiwARowGt2m3qkwAaBWInS5dxdYYae-Hg6egbCMCZGhEA

goodmind avatar Apr 05 '19 23:04 goodmind

I'm now not sure what the cases for this are, refinements and switch seems to work fine with just regular const

goodmind avatar Apr 08 '19 18:04 goodmind

I think it depends if this code is bug or not https://flow.org/try/#0MYewdgzgLgBAYgeQQLhgIkQtMC87NoDcAUMADYCGEEMAQhQE4wDexMMUAngA4Cmu8JCXYALXmTIgAFAEoWbdhxEBLCADoufAWjDLgvIgoC+xE6EiwARowGt2m3qkwAaBWInS5dxdYYae-Hg6egbCMCZGhEA

I think what typescript does falls in the right direction, and would personally err on the side of widening, though this is a good question to bring up on the Discord channel for discussion IMO.

mrkev avatar Apr 11 '19 18:04 mrkev

So this is stalled because we didn't found out how to widen literals to string in mutable locations. This would definitely need another kind of SingletonStrT, and more handling related to variance?

goodmind avatar Jun 29 '19 19:06 goodmind

@mrkev what you think about implementing const assertions instead? https://devblogs.microsoft.com/typescript/announcing-typescript-3-4/#const-assertions

EDIT: nvm, same problems with inference https://flow.org/try/#0DYUwLgBAthC8EAoDaBGANBATBgzAXQC4JUNsJ8BKAKCpnhSqA

The only working thing is fixing typeof directly, since this is most used case of getting literal type from string

goodmind avatar Jul 23 '19 09:07 goodmind

Related issue https://github.com/facebook/flow/issues/7961

goodmind avatar Jul 26 '19 13:07 goodmind

@mrkev

declare var foo: {+a: 'str'}
declare var foo_a: 'str';

const bar = {
  a: foo.a,
  b: foo_a,
  method() {
    bar.b = 'bar' // error
    bar.a = 'bar' // no error
  }
}

https://flow.org/try/#0CYUwxgNghgTiAEA3W8BmB7dAueBvA1FDgOQDOALjMQL4BQoksCyMamA+kfGZcQNy1aYdADsK8AEYoAvHlrx4XDOgB0UADTzJOZZ00KAtiHIALdMAAUASjkKFUmConxZxB8S33Yal93da6aiA

Kinda strange, how can we get bar.b work the same as bar.a?

goodmind avatar Aug 20 '19 00:08 goodmind

@mrkev I solved this problem, should I add more tests?

goodmind avatar Aug 20 '19 02:08 goodmind

This actually makes it inconsistent with let, hmm

https://flow.org/try/#0PTAEAEDMBsHsHcBQjoFMAupKgLygOST6LZ4CMA3CbqOgE4CuqViAxrAHYDOmsNA3lgBcBIqAC+ydt0wAjAIZ0BiUKAAeIwvgA0K0LBGwAdJESSSDDq3QBLTuoAUASlD89CukbU1KoEKBsrTi4bHlQOTHgbdAALUBiEUAADNHQk0HhYOgBrLndFLxp6Jgo-MEDpELCIjOi4hPhk1PTMnLzVfM8+clL-CuDQ9HDIutB5WVgAN1ROo27aRmYygKDuQeHa2LGJ6bNkDznEA7UgA

but it is separate issue

goodmind avatar Aug 20 '19 08:08 goodmind