-
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
Isolated declarations fix signature serialization scoping #58409
Isolated declarations fix signature serialization scoping #58409
Conversation
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.
I stopped reviewing at the letter C (there's a lot changing, of course)
tests/baselines/reference/assertionsAndNonReturningFunctions.types
Outdated
Show resolved
Hide resolved
tests/baselines/reference/assignmentCompatWithGenericCallSignaturesWithOptionalParameters.types
Outdated
Show resolved
Hide resolved
tests/baselines/reference/asyncFunctionsAndStrictNullChecks.types
Outdated
Show resolved
Hide resolved
@typescript-bot perf test this |
@@ -18,16 +18,16 @@ function run(configuration: commands.IConfiguration) { | |||
> : ^^^^^^ | |||
>configuration.workspace.toAbsolutePath(configuration.server) : string | |||
> : ^^^^^^ | |||
>configuration.workspace.toAbsolutePath : (server: IServer, workspaceRelativePath?: string) => string | |||
> : ^ ^^ ^^ ^^^ ^^^^^^^^^^^ | |||
>configuration.workspace.toAbsolutePath : (server: import("visibilityOfCrossModuleTypeUsage_server").IServer, workspaceRelativePath?: string) => string |
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.
Huh, interesting; I guess this was a bug before?
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.
trackExistingEntityName
would previously check if the symbol could be resolved in the original context and if the symbol is accessible from the enclosing declaration. The answer to both of those is yes. The issue is that the symbol is not accessible in the same way. This can lead to the scope confusion I showed in one of the bugs.
What I do now is I test if the symbol is also resolvable in the context it is being used in. And in this case IServer
is not directly accessible so it falls back on typeToTypeNodeHelper
which will print the import.
I have a follow on PR that will try to find a more appropriate name in the enclosing declaration where this is printed (such as commands.IServer
in this case) and will also try to use more of the original node even if sometimes this means replacing parts of the node with nodes printed from types.
@jakebailey 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: |
@typescript-bot perf test this |
@jakebailey 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: |
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.
I like it - injecting new unknownSymbol
refs into the synthetic scope to block lookups we know won't work due to the transform we've done to the parameters is good.
@@ -2320,7 +2320,8 @@ function isDeclarationKind(kind: SyntaxKind) { | |||
|| kind === SyntaxKind.VariableDeclaration | |||
|| kind === SyntaxKind.JSDocTypedefTag | |||
|| kind === SyntaxKind.JSDocCallbackTag | |||
|| kind === SyntaxKind.JSDocPropertyTag; | |||
|| kind === SyntaxKind.JSDocPropertyTag | |||
|| kind === SyntaxKind.NamedTupleMember; |
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.
NamedTupleMember
seemed to be missing here. This meant that tuple member names were being funneled through trackExistingEntityName
even when other declaration names were not.
if (isEntityName(node) || isEntityNameExpression(node)) {
if (isDeclarationName(node)) {
return node;
}
const { introducesError, node: result } = trackExistingEntityName(node, context);
hadError = hadError || introducesError;
// We should not go to child nodes of the entity name, they will not be accessible
return result;
}
Fixes #58408
Fixes #58407