-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Skip markLinkedReferences import elision walk entirely in some common cases #59398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip markLinkedReferences import elision walk entirely in some common cases #59398
Conversation
…s only apparent when the linked references pass is skipped
@typescript-bot test this |
checkIdentifierCalculateNodeCheckFlags(node, s); | ||
nodeLinks.calculatedFlags |= NodeCheckFlags.ConstructorReference; | ||
if (isIdentifier(node)) { | ||
nodeLinks.calculatedFlags |= NodeCheckFlags.BlockScopedBindingInLoop | NodeCheckFlags.CapturedBlockScopedBinding; // Can't set on all arbitrary nodes (these nodes have this flag set by `checkSingleBlockScopeBinding` only) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The references walk was implicitly calling checkComputedPropertyName
on the computed properties, which was calculating the CapturedBlockScopedBinding
and BlockScopedBindingInLoop
flag on them, which was hiding the fact that preemptively setting these computed flags on all nodes we walk while walking only identifiers would block us from calculating them properly for computed and private names. So that's fixed here, too.
@@ -961,6 +961,7 @@ export function emitFiles( | |||
} | |||
|
|||
function markLinkedReferences(file: SourceFile) { | |||
if (ts.isSourceFileJS(file)) return; // JS files don't use reference calculations as they don't do import ellision, no need to calculate it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we remember to remove this if/when decorator metadata becomes a thing again with non-experimental decorators, since preserving imports used for those types I think will end up mattering even in JS files (cross this issue with that other bug I fixed).
Actually, does this work for JSX files? or does this trigger that other bug where we don't realize that import "react"
is going to happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. should we copy the test from #59193 and make it a .jsx file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In JSX files the import "react"
we add to the output is injected by the emitter based on compiler options and doesn't depend on the refrenced-ness of the synthetic import thing in the checker (you can check pretty easily by seeing there's no resolver.isReferencedAliasDeclaration
type calls in the jsx transform - only in the ts transform, and only if the file isn't JS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah, should circumstances change and shouldEmitAliasDeclaration
in ts.ts
changes, this'd need to change to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact: I tried skipping this walk for files with nothing in their .imports
member, too (so no references to external modules), but tests showed that we still needed import elision calculation done even in files with no external imports, because local elision on export import a = b
statements is done using the same logic and relies on this running to work. And there's no simple way to know if those are present without traversing the tree anyway. 🤷♂️ I could have done if (!length(file.imports) && !(file.transformFlags & TransformFlags.TypeScript)) return;
(since export import =
is TS syntax), but how many .ts
files don't contain any typescript syntax - hardly seems worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess it doesn't matter because we don't care about exact errors in this case, yeah.
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
@weswigham Here are the results of running the user tests with tsc comparing Everything looks good! |
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@weswigham Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
@typescript-bot cherry-pick this into release-5.5 |
Hey, @weswigham! I've created #59404 for you. |
5.5.4 already went out, are we doing a 5.5.5? |
Yeah, I think we should try to unbreak that repro. |
This should just improve performance of js->js emit with
noCheck
on.