Flow: Comparators not checking enum validity

[DamonOehlman]

I'm not sure whether this is a feature or a bug, but I found the following behaviour a little surprising:

// @flow

type ItemType =
    'FOO'
  | 'BAR'
  | 'BAZ';

type Item = {|
  itemType: ItemType,
  value: string
|};

const item: Item = {
  itemType: 'FOO',
  value: 'test'
};

// why is this ok?
if (item.itemType == 'bar') {
  console.log('apparently this is ok');
}

if (item.name === 'Bob') {
  console.log('Apparently my name is Bob');
}

Is there a reason that a comparison of item.itemType with the value bar is considered OK? I'm guessing if the functionality is intentional it's to cater for checking code that might be passed as an input to the flow code that has come from a non-type checked source.

If this is the case, that's ok, but it does have a couple of pretty major drawbacks:

1. When refactoring the literal values within for ItemType (i.e. removal, case change) flow doesn't complain in these instances.

2. When refactoring an attribute name on the object type (e.g. itemType => type) this also isn't reported. As shown in the second if statement it's also possible to query an attribute that does not exist on an exact object type (which is also surprising IMO) - happy to submit a separate issue for this if this is also a bug.

[lll000111]

Why should it not be okay? You can check against anything. Flow's task is to ensure the value of what's in property item.itemType, not what you compare it to. So your check is useless because if Flow does its job the left side will always be different, so the comparison will always be false, but that is not a type issue.

Not sure about the name thing though. Technically one could argue along similar lines that nothing is added to item that doesn't belong there by merely doing an always failing check, but sure, one would like a warning, maybe. As soon as you add an assignment, for example item.name = 'Bob';, you get an error. Flow doesn't care if you just "look" and don't actually do anything to the object forbidden by the type, it seems.

I agree, a full type system should at least warn, but I lowered my expectations since starting to work with the 3rd party type systems (Typescript too), it's just really really messy, those systems on top of Javascript, I have come to the conclusion the types must be in the language or we'll always have those unclear and interpretable situations. Here, does mere "passive access" constitute a type violation? Not if one is inclined to the interpretation that type systems for Javascript should only add what's necessary but leave the underlying language semantics intact (and partly visible and available even with types). In that case it will require yet more tools for more code analysis...

[jesseschalken]

@lll000111 You're right, but it is often handy for a type checker to report that some comparison is always true or always false, or that some code is unreachable (the body of the if block), just for the purpose of catching bugs and pruning dead code. I don't know if the Flow team wants to do stuff like that, though.

[DamonOehlman]

Coming from using closure compiler to provide type checking over a large project, the ability to check that a comparison operation is valid based on the defined type is invaluable. Enum types in whatever form they take is a good example. I will validate that this is the case though in closure compiler before singing its praises too much (and it does have significant other issues).

With regards to the property access, I don't think it should matter unless you have defined an object type as exact ({| property: string |}). In the case where you are querying a non existent property on these objects I can't see any case in which flow shouldn't report this as an error.

[lll000111]

@DamonOehlman

I can't see any case in which flow shouldn't report this as an error.

Struggling with related problems just now, of loading JS objects that I stored back into the running app and then ensuring that the type is correct (Flow cannot check what I load from storage so that has to be done during runtime, see #3560 if interested) I can see a use case for that scenario.

For example, I check during runtime if a property exists (or doesn't) to ensure the (compile time) types are valid, that I don't accidentally assign a dynamically created object (and if its coming from storage the static type checks cannot do anything to help) to a variable where I don't actually want that type. I might want to do runtime type checks in code to make sure data matches my compile time static types. And that's just an example I can think of.

[DamonOehlman]

Turns out I was completely wrong about closure compiler having my back in this situation. The following is considered completely ok:

/** @enum {string} */
const SpinnerStyle = {
  STANDARD: 'STANDARD',
  FANCY: 'FANCY'
};

/**
 * @type {SpinnerStyle}
 */
let style = SpinnerStyle.STANDARD;
if (style === 'TEST') {
  console.log('is dead code');
}

I guess I'll just have to continue to keep my expectations low across the board 😞

I still think it would probably be a nice feature to have in a type checker but there's probably a good reason why it isn't. @lll000111 as you pointed out, it's just dead code and that's probably not reason enough to enforce strict type checking.

[Novex]

I think this kind of got introduced in 0.43 in https://github.com/facebook/flow/commit/9e42924cb5da000d50ca9a5d68e0ea4d4cf64e8f, though it only seems to work for variables outside of a class/object (I'm not sure if this is intentional?).

Eg, in this example the first part works as you'd expect, but the second part doesn't

/* @flow */

type SomeType = 'valid' | 'also-valid';

// Part 1 - works
let normalVariable: SomeType = 'valid';

if (normalVariable === 'totally-invalid') {
    throw "oops!";
}

// Part 2 - doesn't work
type SomeObject = { prop: SomeType };
let objectVariable: SomeObject = { prop: 'valid' };

if (objectVariable.prop === 'apparently-not-invalid?') {
    throw "this is fine!";
}

Output, which I guess should also have an error for 'apparently-not-invalid?'

8: if (normalVariable === 'totally-invalid') {
                          ^ string literal `totally-invalid`. This type is incompatible with
6: let normalVariable: SomeType = 'valid';
                       ^ string enum

I think if this also checked object/class properties that would resolve this issue?

[Novex]

@nmn just wondering why this got closed?

[nmn]

The central question seemed to be:

// why is this ok?
if (item.itemType == 'bar') {
  console.log('apparently this is ok');
}

When item.itemType is not allowed to be 'bar'.

Checking for the type of something is not a type error. Thought your answer explained that. But re-opening because it was not my intention to close an unresolved issue.

[DamonOehlman]

I guess I do see some inconsistency when the phrase "Checking for the type of something is not a type error" is used. Consider the following example:

  const test: number = 5;
  if (test == 'test') {
    console.log('hello');
  }

In the above example flow reports the following with regards to the comparison.

number. This type cannot be compared to string

Incidentally, the same error is not reported when strict equality checks (===) are used. This seems to be covered under a separate wishlist issue though see #2359.

Apologies, I digress. My point above being that IMO a comparison of bounded set of values ('FOO' | 'BAR' | 'BAZ') to a value outside of that set (bar) should be considered as invalid as a comparison of the entire range of numbers to the entire range of strings.

[ClaireNeveu]

We were recently confused by this in the context of switch statements.

type FooBarBaz = 'Foo' | 'Bar' | 'Baz'

class Wrapper {
  style: FooBarBaz
  constructor(style: FooBarBaz) {
    this.style = style;
  }
}
const wrapper = new Wrapper('Bar')

switch (wrapper.style) {
   case 'Foo':
      break;
   case 'Barr':
      break;
}

I would expect an error at case 'Barr' since it isn't a valid value here but instead this passes Flow and does something unexpected at runtime. This can be remedied by exhaustively checking all values and doing value: empty in the default case but that adds a lot of boilerplate in places where you only want to handle one or two cases out of several.

Curiously this only seems to be a problem when the type is a class member.

[mrtnbroder]

Anything new on this? I've stumbled upon this today and was wondering why flow didn't warn me.

type Color = 'Blue' | 'Red'

const color: Color = 'Red'

color === 1 // no warning here that color can ever be a Number.