fix(isolated_declarations): Emit computed properties when they are well known symbols

[MichaelMitchell-at]

Fixes #4016.

To fully comply with the reference behavior of the TypeScript compiler, we need to be able to detect the case when Symbol or globalThis.Symbol do not refer to the global Symbol. I achieve this by creating a similar structure to ScopeTree called GlobalSymbolBindingTracker (will take suggestions for a better name) and have it visit the entire program before we begin transformation. We can keep track of which computed properties reference non-global Symbol by storing a set of their spans, which should each be uniquely identifying since we're in the context of a single file.
Later, if and when we transform the computed property into a type declaration, we can refer back to this set to see if it should be allowed.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[codspeed-hq]

CodSpeed Performance Report

Merging #4099 will not alter performance

_{Comparing MichaelMitchell-at:well_known_symbols (0ffa3c2) with main (8ef85a4)}

Summary

✅ 28 untouched benchmarks

[Dunqing]

Have you looked at scope.rs?

[Dunqing]

Can you explain more what the difference between this approach and typescript's behavior?

[MichaelMitchell-at]

Can you explain more what the difference between this approach and typescript's behavior?

With an alternative approach this code would have an isolated declarations error:

export function foo(Symbol: any): void {
  const x = {[Symbol.iterator]: 1};
}

because Symbol is bound in the current scope, and we are trying to use Symbol.* as a property key. In TypeScript this wouldn't be an isolated declarations error because x clearly has no effect on emit.

[Dunqing]

Let's try this alternative approach.

[MichaelMitchell-at]

Ok, I might not get to this until next weekend

[MichaelMitchell-at]

@Dunqing actually I thought of an approach that matches the same behavior as TypeScript, but I'm not sure if the performance/memory usage would be considered acceptable by the project. Please take a look.

[MichaelMitchell-at]

We could probably avoid allocations for this most of the time by first checking if there is both an occurrence of overriding the global Symbol and references to Symbol anywhere in the program, not necessarily in the same scope, but I'm not sure what the cost of that extra traversal would be.

[Dunqing]

Is it possible to check on demand? For example, we initially only check for the root scope. When we transform the function we will check the function. This will only check for declarations that will be exported

[MichaelMitchell-at]

Possibly, I will think about it some more. I think one of the main challenges comes just from organizing the code and performing state management. It feels like similar to with ScopeTree we would need to tell GlobalSymbolBindingTracker when it is entering or leaving a scope, but the logic will diverge from ScopeTree (otherwise we would have just used ScopeTree for this purpose), which may result in the code looking more confusing.

(Just voicing my thoughts out loud) I think what feels redundant with my current approach is that the short-circuit optimizations I take are making assumptions about how the tree will be traversed for emitting declarations, so that logic kind of feels like it's being reproduced in two places. The code could be potentially cleaner if that logic could be pulled out. What I imagine is something like alternative Visitor traits that only traverse the parts of the AST relevant to emitted declarations. Such traits would already have the "optimizations" I wrote naturally built-in. What's harder for me to imagine right now is how easy it would be to re-implement declaration transforms on top of such traits.

Please let me know your thoughts.

[Dunqing]

Possibly, I will think about it some more. I think one of the main challenges comes just from organizing the code and performing state management. It feels like similar to with ScopeTree we would need to tell GlobalSymbolBindingTracker when it is entering or leaving a scope, but the logic will diverge from ScopeTree (otherwise we would have just used ScopeTree for this purpose), which may result in the code looking more confusing.

Only Function and TSModuleDeclaration will have nested scopes, which I don't think should be too much trouble. We could try.

(Just voicing my thoughts out loud) I think what feels redundant with my current approach is that the short-circuit optimizations I take are making assumptions about how the tree will be traversed for emitting declarations, so that logic kind of feels like it's being reproduced in two places. The code could be potentially cleaner if that logic could be pulled out. What I imagine is something like alternative Visitor traits that only traverse the parts of the AST relevant to emitted declarations. Such traits would already have the "optimizations" I wrote naturally built-in. What's harder for me to imagine right now is how easy it would be to re-implement declaration transforms on top of such traits.

Adding another trait instead of Visitor may significantly increase maintenance costs. Currently Visitor is automatically generated with ast-codegen

[MichaelMitchell-at]

Adding another trait instead of Visitor may significantly increase maintenance costs

That's a good point. I suppose it would be necessary for such a trait to extend Visit and only override a small subset of the method implementations.

Only Function and TSModuleDeclaration will have nested scopes, which I don't think should be too much trouble. We could try.

I may not be able to get to this soon, but as you said, we have time!

[Dunqing]

Sorry for the late response. I will review this week. Also, I wanted to ask, do you think we merge after TypeScript 5.6 is released or before?

[MichaelMitchell-at]

Sorry for the late response. I will review this week.

No problem, no rush :)

Also, I wanted to ask, do you think we merge after TypeScript 5.6 is released or before?

I think it's ok to wait, though I pinged the TypeScript repo to see if they might include the change in a new patch release of 5.5.

[Dunqing]

Looks like we can skip visit_statement in GlobalSymbolBindingTracker. We just need to collect bindings.

[MichaelMitchell-at]

Looks like we can skip visit_statement in GlobalSymbolBindingTracker. We just need to collect bindings.

I believe we need to traverse into the bodies of if/for/while loops and there are some cases where declaration is allowed, for example for (const Symbol of []) { ... }.

[Dunqing]

I believe we need to traverse into the bodies of if/for/while loops and there are some cases where declaration is allowed, for example for (const Symbol of []) { ... }.

Oh yes. You are right. Can we skip some statements that are not binding, traversing the whole AST has a performance impact.

[MichaelMitchell-at]

@Dunqing I made some optimizations to bring the benchmark back to baseline performance, but each one needs careful attention to make sure there isn't a scenario that I didn't think of.

[Dunqing]

@Dunqing I made some optimizations to bring the benchmark back to baseline performance, but each one needs careful attention to make sure there isn't a scenario that I didn't think of.

Well done!

[Dunqing]

Can you add a test(like this) that uses non-global Symbol and globalThis in non-export declarations? I would like to see have any errors in this test.

[Dunqing]

I think it's ok to wait, though I pinged the TypeScript repo to see if they might include the change in a new patch release of 5.5.

I saw that v5.5.4 doesn't include this change. We have more time to improve this!

[MichaelMitchell-at]

Can you add a test(like this) that uses non-global Symbol and globalThis in non-export declarations? I would like to see have any errors in this test.

Sure, just added them

[Dunqing]

Is it possible to check on demand? For example, we initially only check for the root scope. When we transform the function we will check the function. This will only check for declarations that will be exported

[DonIsaac]

@Dunqing what do we need to do to get this over the finish line?

[Dunqing]

Feel free to convert it to open and ping me if it ready to review

[MichaelMitchell-at]

I'm about to go on vacation so will probably not be able to touch this for at least another couple weeks. If anyone wants to iterate on this in the meantime, please feel free :)

[Dunqing]

I'm about to go on vacation so will probably not be able to touch this for at least another couple weeks. If anyone wants to iterate on this in the meantime, please feel free :)

Thank you for your hard work on Isolated Declaration! Have a nice holiday!!