-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Don't skip markLinkedReferences on ambient properties #59325
Conversation
@typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Everything seems fine in the above; @weswigham is there a major downside to removing this bit? |
I think the only |
I can scope it down, the trouble is just that there's an "unspecified" hint which is hit from We just didn't have this optimization at all in the original code before #58366 so reverting it seemed okayish. |
@@ -29679,9 29679,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
if (!canCollectSymbolAliasAccessabilityData) { | |||
return; | |||
} | |||
if (location.flags & NodeFlags.Ambient) { |
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.
Yeah, I wouldn't want to remove this outright - perf tests won't show it, but this is pretty important to keep this fast from my testing. I'd just except ambient property signatures from the early skip, since they're like... not really ambient? It looks like when we have something like
class A {
@decorator(class B {})
declare prop: string;
}
the prop
is ambient marked, but the decorator is not, so we should just be able to except the prop, like @rbuckton says.
if (location.flags & NodeFlags.Ambient && !isPropertySignature(location) && !isPropertyDeclaration(location)) {
seems like it aughta work?
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.
Done; that seemed to be enough.
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.
declare
fields aren't quite as ambient as other declarations. They were added as a compatiblity mechanism due to native class fields using Define semantics, with legacy decorators being one of the main motivations.
Updated; PTAL |
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.
Funky to me that we allow a supposedly ambient prop to be decorated at all - seems like that kinda forces it to be non-ambient 🤷♂️
@typescript-bot cherry-pick this to release-5.5 |
Hey, @jakebailey! I've created #59336 for you. |
…e-5.5 (#59336) Co-authored-by: Jake Bailey <5341706 [email protected]>
Fixes #59051
Alternatively, I can also only walk ambients when
emitDecoratorMetadata
is on, or only when the hint isUnspecified
orDecorator
.