Fixed too eager type resolution when looking for symbol aliases

[Andarist]

fixes #57357
fixes #57843

[weswigham]

I'm not a huge fan of this - this looks like it's attempting to reverse some of the logic done within getTypeFromTypeAliasReference and the like to assign alias symbols to a type in the first place, and I can't be confident it covers all cases, since that operates on types, while this is basically alternating between symbols and AST nodes while avoiding the information trafficked in the (declared) type. I'd be much happier if the logic here was actually shared with type alias assignments, eg, it actually used getAliasSymbolForTypeNode to lookup the symbol, and then shared logic with type creation to determine if that symbol actually applied to the resulting type. (Eg, this looks kinda like the isTypeReferenceType(node) branch in getTypeFromTypeAliasReference, with some heuristic only-types-without-type-parameters-would-hold-old-aliases thrown in, but because the logic isn't actually shared, I can't be sure they match)

[Andarist]

This kinda tries to follow checkTypeReferenceOrImport. I understand your hesitancy here but I'm not quite sure how I'm supposed to unify this with this function or the ones that were mentioned. The problem is that all of them primarily operate on the declared types so they resolve this information eagerly (what the previous code was doing).

The problem with the eager resolution is that type printing in error messages goes through similar code paths as the declaration emit. So an error raised on some relation ended up resolving types that were previously not resolved and that led to the raised circularity.

This new code is also recursive and the mentioned functions are not.

Consider this:

import { SomeType as IndirectSomeType } from './indirection'
export type SomeType/*1*/ = IndirectSomeType/*2*/;

When requesting getAliasSymbolForTypeNode on 2 we just end up on 1. This holds true:

getAliasSymbolForTypeNode(decl.type).declarations[0] === decl // true

So it doesn't give us any new information because it doesn't even go to the import location.

In addition to that, resolveTypeReferenceName (that I could use instead of getSymbolAtLocation) goes just to the import statement. And even resolveAlias goes just to the intermediate declaration site in ./indirection.ts and not to ./inner.ts.

So all in all, I don't see how to reuse the existing machinery, in a clean way, because that machinery is just shallow and still relies on declared types heavily - and that has to be avoided here. So I think that any attempt at unifying this would not be that clean/straightforward.

Alternatively, I was thinking about using the existing solution (what is currently on main) but to combine it with either one of those 2:

• pass around some flag that would avoid this lookup when printing errors
• make relation error printing lazy so by the time they are printed all the declared types are already set up correctly

PS. I have quite huge gaps in my understanding of how symbol aliases etc work today. It's not something that I have touched a lot.

Eg, this looks kinda like the isTypeReferenceType(node) branch in getTypeFromTypeAliasReference,

yes, kinda - that branch is only entered when type parameters are involved though and this code specifically wants to resolve those symbols when there are no type parameters. I feel like this is another indication that this is actually not that same-y

[weswigham]

make relation error printing lazy so by the time they are printed all the declared types are already set up correctly

Personally, I think I'd much prefer this - the need for a setup phase like that is why we use checkNodeDeferred for many node kinds - it's a common pattern for us.

[Andarist]

It's different from checkNodeDeferred - we don't want to defer checking, just error reporting. I thought that I could use addLazyDiagnostic for this but it turns out that it's still often called quite eagerly by checkSourceFileWithEagerDiagnostics. So - unless there is already something like this - I'd have to introduce a new (small) mechanism (maybe just an extension to either checkNodeDeferred or addLazyDiagnostic). I'll look into this tomorrow.

[Andarist]

I've looked into it more. If you don't like the current approach then deferring those errors would be best.

However, it's not that straightforward. Reporting relation errors is quite convoluted and spread out across multiple places. reportRelationError eagerly converts types to strings using getTypeNamesForErrorDisplay (there are some extra eager typeToString calls there too).

It wouldn't be hard to adjust this and make it lazier if only this would directly be added to the diagnostic collection here. That's not the case though - this just creates (overrides?) errorInfo, that is later conditionally chained with containingMessageChain in checkTypeRelatedTo, relatedInformation might get appended to it, and only after that DiagnosticWithLocation gets created, and added to the diagnostic collection.

All of this assumes already a certain structure - one that is string-oriented. It's not impossible to adjust this. I feel that it would be a disruptive change though. I can commit to working on it if you feel that it's the best solution.

This PR fixes a regression and it would be great to backport a fix to the upcoming 5.4. Since making this type-to-string conversion deferred isn't straightforward I feel like it's too risky for it to be that (at this moment). After thinking about it more, I don't think the currently proposed solution is that bad. It does its job in an isolated manner and the rules around this lookup are pretty straightforward.

If you feel strongly that this is just not the best solution for the problem (which it might not be, I'm not the judge of that :P) then I propose adding a new internal TypeFormatFlags/NodeBuilderFlags/SymbolFormatFlags based on which the extra lookup introduced by #56087 would be avoided when printing types for relation errors. This would be less than ideal than the currently proposed solution since, well, a better print wouldn't be used by relation error messages - that's not a regression though (with 5.3 as the baseline). With this as the temporary fix in place, the regression would get addressed and I could work on making the relation error lazier for 5.5