moveToFile: fix check for global name resolving

[iisaduan]

Fixes #59799.

Fixes isGlobalType, which was added in #58811.

[iisaduan]

close/reopening to fix ci

[andrewbranch]

I'm having trouble wrapping my head around this function, I think because it takes a symbol but only uses it for its name. Do the function name and parameters need to be tweaked a bit to better reflect what this is trying to accomplish?

[iisaduan]

Is there another way to check if a symbol refers to a global, and can be checked reliably after symbol merging/through aliasing? I guess this function doesn't need a Symbol to be passed in as it currently is, but I couldn't think of a better way to do this check for globals.

The old code was checking if there was any global symbol with that name that was a type without seeing first if there was a local symbol with that name. The function name seems alright to me since it's checking if, at this node, the symbol refers to a global type.

[andrewbranch]

The function name seems alright to me since it's checking if, at this node, the symbol refers to a global type

It’s checking if a given name refers to a global type; it’s not really checking anything about the symbol that’s being passed in. If you wanted to check whether the symbol itself is a type that is accessible as a global at a particular location, you might check for equality between the symbol and the resolveName result. But I’m not sure that’s relevant here.

But the line added here seems to allow for a false negative of the simple question “does this name refer to a global type.” Just because a name resolves to a local value doesn’t mean it doesn’t also resolve to a global type at the same location:

const Event = {};
let e: Event; // <--

If you pass in the node for the Event type, isGlobalType will return false. That feels like the wrong answer just reading the function name, even if it has the desired behavior for Move To File (does it?).

[iisaduan]

But the line added here seems to allow for a false negative of the simple question “does this name refer to a global type.” Just because a name resolves to a local value doesn’t mean it doesn’t also resolve to a global type at the same location:

The correct check to have here is if the symbol refers to a global type or not, and the previous check did not do that (and seems like the previous check wanted to check the symbol, given that they took a symbol instead of just the name).

even if it has the desired behavior for Move To File (does it?).

It makes sense to me that if we try to move a symbol that references a local definition, but also happens to have an unrelated global definition, that we keep the reference in the new location as the same symbol that was in the original file. Otherwise, the code in the new file can be broken, like in the bug report above. I spoke to @navya9singh about this, and we concluded that we should generate the import to keep the original reference. Also, other codefixes keep local references across files, even if the symbol resolves to a global (added test to show this).

[DanielRosenwasser]

@andrewbranch am I correct that you're mostly not a fan of the function doing the following?

1. taking a Symbol instead of a string (since all it needs is the name of the symbol)
2. being named isGlobalType, since a type might not actually be declared in the global scope (so e.g. cannotResolveTypeAt would be a "better" name)

[andrewbranch]

Yes - I didn't recontextualize myself with the broader problem enough to tell whether the implementation/behavior is wrong; I can just tell from reading the function in isolation that something is a little fishy (specifically, your points above).

[iisaduan]

@DanielRosenwasser @andrewbranch How do you feel about the new behavior? I can find a better way to do this check and/or rename the function if the new behavior makes sense.

I can change the first part of this function to skipAlias (which in checker is resolveSymbol), but I think I'm confused because I've looked through the existing code, and I don't know how else to perform the second part of this check to find if there is a global without using the name resolver.