fix(60908): Unexpected "'Type' is declared but its value is never read." error with jsdoc @import syntax #60921
Conversation
typescript-bot
added
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
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! |
|
Looking at the playground, I believe this does fix #58368, yes.
|
typescript-bot
added
the
For Milestone Bug
src/compiler/binder.ts
Outdated
| if (isJSDocImportTag(node)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why is the import tag special here?
Also, this early return will leave inAssignmentPattern in a bad state, so I think it's better to skip the steps below that are broken with import tags if needed, or maybe there's something else wrong otherwise.
There was a problem hiding this comment.
@jakebailey thanks for the feedback. These changes are open for discussion :). This applies to the case when we have
/** @import { Foo } from 'foo' */
function foo() {}The function acts as a container that holds locals (bind importClause), including the symbol Foo. Since this symbol is unused (and doesn't have an exportSymbol like the typedef does), the checker triggers an error.
if (.... local.exportSymbol) {
return;
}Perhaps the container for the import tag should be treated like a SourceFile...
ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals;Anyway, I’d be grateful for any ideas or suggestions regarding this case. thanks
There was a problem hiding this comment.
Mainly, I'm trying to understand why import tags have to be special cased, but other JSDoc "declarations" don't.
There was a problem hiding this comment.
@jakebailey JsDocImportTag reuses importClause, however, it doesn't bind it immediately, opting for a delayed binding instead
TypeScript/src/compiler/binder.ts
Lines 1202 to 1204 in 8da951c
TypeScript/src/compiler/binder.ts
Lines 2126 to 2135 in 8da951c
When the binder executes bindChildren and bindEachChild, it iterates through all child nodes, including importClause, and binds them
TypeScript/src/compiler/binder.ts
Lines 3062 to 3063 in 8da951c
TypeScript/src/compiler/binder.ts
Lines 3058 to 3059 in 8da951c
this creates incorrect locals for the parent container. Other tags, such as @typedef and @callback, don't rely on such nodes.
There was a problem hiding this comment.
Hm, I see; this honestly makes me wonder why the code doesn't just fall through into the code below, which seems to have a lot more special cases that feel like they'd matter in the same way...
There was a problem hiding this comment.
As in, be written:
// ...
if (checkUnreachable(node)) {
if (canHaveFlowNode(node) && node.flowNode) {
node.flowNode = undefined;
}
} else if (node.kind >= SyntaxKind.FirstStatement && node.kind <= SyntaxKind.LastStatement && (!options.allowUnreachableCode || node.kind === SyntaxKind.ReturnStatement)) {
(node as HasFlowNode).flowNode = currentFlow;
}
switch (node.kind) {
// ...There was a problem hiding this comment.
I mean it falls over, so that can't be right either
There was a problem hiding this comment.
I think unreachable flow cannot be handled properly without
bindEachChild(node);
bindJSDoc(node);I'm exploring other options to skip processing child nodes for JSDoc imports...
There was a problem hiding this comment.
I think unreachable flow can remain as is. Alternatively, an additional flag could be added to symbols bound from the JSDoc import (ImportSpecifier) to exclude them from the unused locals check. @sandersn @jakebailey what do you think?
There was a problem hiding this comment.
Two architecture questions:
- Why is importClause binding delayed? Could it be not delayed, so that the normal unreachable bind/bindChildren calls work?
- Why does checkUnreachable return/why doesn't Jake's idea work? That would be the least confusing fix, I think.
Otherwise .. the current code is the smallest fix, but it makes the code even more confusing to read.
|
@typescript-bot test it |
| case SyntaxKind.JSDocImportTag: | ||
| return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals; |
There was a problem hiding this comment.
As I understand it, these 3 flags are to prevent mistakenly using a function host as the container for the imported bindings. Is that correct? Can you add a comment like that? I didn't naturally expect imports to be containers otherwise.
(This won't be a problem in Corsa since tags that generate statements won't even have hosts for the generated statements.)
|
@sandersn Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Hey @sandersn, the results of running the DT tests are ready. Everything looks the same! |
|
@sandersn Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@sandersn Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Fixes #60908
Fixes #58368