[Flow 171]: invalid-exported-annotation errors disappear when the file is focused

[flex-tjon]

I'm not sure if this is an issue with Flow or with the VS Code extension, but I noticed this on our repo when I upgraded Flow to 0.171.0 and was able to reproduce it in a minimal repo as well.

The issue tl;dr is that the new invalid-exported-annotation errors specifically disappear in the editor (both in the problems tab and in code highlighting) when the file they're in is focused in VS Code. The errors are still reported when running yarn flow in a terminal.

Steps to reproduce are in the linked repo README!

[flex-tjon]

This issue seems specific to experimental.new_merge being enabled in .flowconfig: that option is off by default in 0.170.0 and on by default in 0.171.0, but in either version the issue only appears when the option is enabled.

[gkz]

cc @panagosg7 who worked on new merge

[panagosg7]

This is indeed due to new_merge becoming the default. This change is mostly an internal refactoring, but there are some corner-cases where the change will limit the kind of types that can be exported.

In your example it is the export of an operation (property access ENUM_DEFINITION.one) over a variable that has a complex type (ENUM_DEFINITION is of type $ObjMap<T, <V>(V) => $Keys<T>> where is the type of some object).

In your example you can avoid the error if you use the $ObjMapConst utility type (added in facebook/flow@f4403cc) which has a special-case of $ObjMap, but has a simpler implementation that allows the above operation. So you can define EnumType as

export type EnumType<T> = $ObjMapConst<T, $Keys<T>>;

Sorry for the inconvenience. I'll add this type to utility type docs and update the changelog with this side-effect.

[kejistan]

Nice to know about $ObjMapConst. I think the more interesting bit is the disappearing errors, rather than the fact that there are new cases of invalid-exported-annotation errors. new_merge adding new errors is fine, but VS Code (or maybe the LSP?) not reporting these errors for open files seems like a bug.

[panagosg7]

I'm also looking into this issue.

Ultimately this is a buggy interaction of "evaluated" utility types and the exporting mechanism. (Note that this is not specific to new_merge). Most such utility types (e.g. $Call, $ObjMap, etc.) are computed lazily, so if there is an error involving such a type it may only be discoverable by opening a file downstream that would trigger the actual computation of this type and thus the error.

As an example consider the following code:

// file a.js
export type A = $Call<(number) => number, string>;

and

// file b.js
import type {A} from './a';
(1: A);

Type A clearly has an error, but if you just open that file in vscode you won't get an error. The error appears if you open b.js, which attempts to force type A into evaluation, and thus causes the error to show up.

In the past I tried to fix this flakiness by forcing all such evaluated types when checking a file, but his caused a perf regression and some spurious errors. This time I'll focus on just the evaluated types in the files extracted signature, since these are the ones that cause the errors downstream.

[flex-tjon]

Thanks for the context @panagosg7, and for updating the docs so quickly! One more thing I noticed is that regardless of what VS Code (or the Flow LSP) was reporting or which files were open, the flow client (at yarn flow) always reported the correct set of errors. Are utility types computed lazily for the LSP integration but non-lazily otherwise? This happened whether or not lazy_mode was enabled so I'm not sure if that's related as well.

[panagosg7]

Are utility types computed lazily for the LSP integration but non-lazily otherwise?

There is no distinction in how utility types are handled. I think the difference in behavior has to do with which files are checked and what errors are shown.

Assuming the file structure from my example above:

In the CLI:

• If you start a non-lazy server: error is reported immediately.
• If you start a lazy server: initially no errors will be reported, since no file is checked. If you flow force-recheck --focus file a.js, this will cause its dependent b.js to also be checked, and so the error will be raised. The same holds if you force b.js. So, either way the error is raised.

In the IDE, what happens corresponds roughly to the command flow check-contents, but not 100%: In the CLI this command will show the error if you flow check-contents b.js < b.js. The error's location will be in file a.js (a dependency of b.js). This is where the difference with the IDE comes in: opening file b.js in the IDE does not show the error in the "Problems" panel (at least not in my experiments). (Of course errors do show up in the opposite direction, ie. a change in a.js that causes an error in b.js).

I believe the rationale here is that this behavior should not happen: Dependents should not affect the checking result of their dependencies. Flow is good at maintaining this invariant in general, but in some cases it breaks:

• Evaluated utility types is a common culprit that has existed for a while now. Fixing this case is more involved: just forcing them all indiscreetly at the file they are defined causes many spurious errors and perf regression. It is still in our TODO list, however.
• New-merge errors (like the one in the OP) is a new one, but since yesterday I have work in progress that will actually force these errors to be raised at the file that introduces them, so hopefully this category will be gone soon.
• There is also a long tail of other exotic cases that occasionally show up, where we are just not diligent enough to make sure the proper location is blamed, and so the blame ends up being attributed to a dependency.

[flex-tjon]

Ahhhh I understand now, thanks! Since this is a Flow bug rather than an extension bug I can go ahead and close this issue for now, I'll be on the lookout for the new-merge error location fix you mentioned. Thanks again for your hard work!