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
Forbid unused property renaming in destructuring binding in function types #41044
Changes from 17 commits
4aec8ef
a37d6d4
67732d3
1d114e6
4abcfa1
39e90ce
79ef4fd
69b8a58
4ddd9ae
e210484
2797958
cb326d2
17a29ff
c4c80db
1e3dc2e
32c340e
9c9fe5e
f0e5112
7aa2b6e
755c7c4
c7768cf
bdb42e1
6d4d147
bc92945
e2710b7
1e4c14b
4e519ff
07f162f
6c8a95b
bd0da30
600b614
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1026,6 +1026,11 @@ namespace ts { | |
const potentialNewTargetCollisions: Node[] = []; | ||
const potentialWeakMapSetCollisions: Node[] = []; | ||
const potentialReflectCollisions: Node[] = []; | ||
const potentialAlwaysCheckedUnusedTypes: { | ||
node: Node; | ||
name: Identifier; | ||
diagnostic: DiagnosticMessage; | ||
}[] = []; | ||
const awaitedTypeStack: number[] = []; | ||
|
||
const diagnostics = createDiagnosticCollection(); | ||
|
@@ -5964,19 +5969,29 @@ namespace ts { | |
return parameterNode; | ||
|
||
function cloneBindingName(node: BindingName): BindingName { | ||
return elideInitializerAndSetEmitFlags(node) as BindingName; | ||
function elideInitializerAndSetEmitFlags(node: Node): Node { | ||
return elideInitializerAndPropertyRenamingAndSetEmitFlags(node) as BindingName; | ||
function elideInitializerAndPropertyRenamingAndSetEmitFlags(node: Node): Node { | ||
if (context.tracker.trackSymbol && isComputedPropertyName(node) && isLateBindableName(node)) { | ||
trackComputedName(node.expression, context.enclosingDeclaration, context); | ||
} | ||
let visited = visitEachChild(node, elideInitializerAndSetEmitFlags, nullTransformationContext, /*nodesVisitor*/ undefined, elideInitializerAndSetEmitFlags)!; | ||
let visited = visitEachChild(node, elideInitializerAndPropertyRenamingAndSetEmitFlags, nullTransformationContext, /*nodesVisitor*/ undefined, elideInitializerAndPropertyRenamingAndSetEmitFlags)!; | ||
if (isBindingElement(visited)) { | ||
visited = factory.updateBindingElement( | ||
visited, | ||
visited.dotDotDotToken, | ||
visited.propertyName, | ||
visited.name, | ||
/*initializer*/ undefined); | ||
if (visited.propertyName && isIdentifier(visited.propertyName) && isIdentifier(visited.name)) { | ||
visited = factory.updateBindingElement( | ||
visited, | ||
visited.dotDotDotToken, | ||
/* propertyName*/ undefined, | ||
visited.propertyName, | ||
/*initializer*/ undefined); | ||
} | ||
else { | ||
visited = factory.updateBindingElement( | ||
visited, | ||
visited.dotDotDotToken, | ||
visited.propertyName, | ||
visited.name, | ||
/*initializer*/ undefined); | ||
} | ||
} | ||
if (!nodeIsSynthesized(visited)) { | ||
visited = factory.cloneNode(visited); | ||
|
@@ -37564,6 +37579,14 @@ namespace ts { | |
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently just checking truthiness of |
||
|
||
function checkPotentialAlwaysCheckedUnusedTypes() { | ||
for (const { node, name, diagnostic } of potentialAlwaysCheckedUnusedTypes) { | ||
if (!(getSymbolOfNode(node)?.isReferenced! & SymbolFlags.Type)) { | ||
error(name, diagnostic, declarationNameToString(name)); | ||
} | ||
} | ||
} | ||
|
||
function bindingNameText(name: BindingName): string { | ||
switch (name.kind) { | ||
case SyntaxKind.Identifier: | ||
|
@@ -37905,6 +37928,24 @@ namespace ts { | |
} | ||
|
||
if (isBindingElement(node)) { | ||
if ( | ||
node.propertyName && | ||
isIdentifier(node.propertyName) && | ||
isIdentifier(node.name) && | ||
isParameterDeclaration(node) && | ||
nodeIsMissing((getContainingFunction(node) as FunctionLikeDeclaration).body)) { | ||
// type F = ({a: string}) => void; | ||
// ^^^^^^ | ||
// variable renaming in function type notation is confusing, | ||
// so we forbid it even if noUnusedLocals is not enabled | ||
potentialAlwaysCheckedUnusedTypes.push({ | ||
node, | ||
name: node.name, | ||
diagnostic: Diagnostics.Variable_0_is_not_used_Did_you_mean_to_write_a_type_annotation_for_whole_object | ||
}); | ||
return; | ||
} | ||
|
||
if (isObjectBindingPattern(node.parent) && node.dotDotDotToken && languageVersion < ScriptTarget.ES2018) { | ||
checkExternalEmitHelpers(node, ExternalEmitHelpers.Rest); | ||
} | ||
|
@@ -41748,6 +41789,7 @@ namespace ts { | |
clear(potentialNewTargetCollisions); | ||
clear(potentialWeakMapSetCollisions); | ||
clear(potentialReflectCollisions); | ||
clear(potentialAlwaysCheckedUnusedTypes); | ||
|
||
forEach(node.statements, checkSourceElement); | ||
checkSourceElement(node.endOfFileToken); | ||
|
@@ -41767,6 +41809,9 @@ namespace ts { | |
} | ||
}); | ||
} | ||
if (!node.isDeclarationFile) { | ||
checkPotentialAlwaysCheckedUnusedTypes(); | ||
} | ||
}); | ||
|
||
if (compilerOptions.importsNotUsedAsValues === ImportsNotUsedAsValues.Error && | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3487,6 +3487,10 @@ | |
"category": "Error", | ||
"code": 2841 | ||
}, | ||
"Variable '{0}' is not used. Did you mean to write a type annotation for whole object?": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DanielRosenwasser do you want to take a stab at this? I was thinking something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Related span on the end of the object if a type annotation doesn't already exist
What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These assume you meant to write a type annotation but don’t say what you did write. If our assumption is wrong, it’s even more confusing. I’m also a little concerned about the interpolation of the object There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait, you can’t get this in a React component, this is only in types 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, In theory there is a weird case of non-identifier names (and please add tests for these @uhyo): const sym = Symbol();
type Func1 = ({ "prop": string }) => void;
type Func2 = ({ 2: string }) => void;
type Func3 = ({ ["prop"]: string }) => void;
type Func4 = ({ [4]: string }) => void;
type Func5 = ({ [sym]: string }) => void; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a mash-up?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this. I also like the related span message. I feel like the use of “here” in related spans can be confusing, but that’s how all of our related spans are, and it’s more of an editor display issue than an issue on our end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added test cases and updated diagnostic messages! Does this match your intention?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does any mark show up for the related span if it’s zero-length? At first I thought the span from Also, mentioned this in the review, but I think it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"category": "Error", | ||
"code": 2842 | ||
}, | ||
|
||
"Import declaration '{0}' is using private name '{1}'.": { | ||
"category": "Error", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts(12,18): error TS2842: Variable 'b' is not used. Did you mean to write a type annotation for whole object? | ||
tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts(12,28): error TS2842: Variable 'a' is not used. Did you mean to write a type annotation for whole object? | ||
|
||
|
||
==== tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts (2 errors) ==== | ||
interface a { a } | ||
interface b { b } | ||
interface c { c } | ||
|
||
type T1 = ([a, b, c]); | ||
type F1 = ([a, b, c]) => void; | ||
|
||
type T2 = ({ a }); | ||
type F2 = ({ a }) => void; | ||
|
||
type T3 = ([{ a: b }, { b: a }]); | ||
type F3 = ([{ a: b }, { b: a }]) => void; | ||
~ | ||
!!! error TS2842: Variable 'b' is not used. Did you mean to write a type annotation for whole object? | ||
~ | ||
!!! error TS2842: Variable 'a' is not used. Did you mean to write a type annotation for whole object? | ||
|
||
type T4 = ([{ a: [b, c] }]); | ||
type F4 = ([{ a: [b, c] }]) => void; | ||
|
||
type C1 = new ([{ a: [b, c] }]) => void; | ||
|
||
var v1 = ([a, b, c]) => "hello"; | ||
var v2: ([a, b, c]) => string; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
tests/cases/compiler/excessPropertyCheckWithSpread.ts(1,25): error TS2842: Variable 'number' is not used. Did you mean to write a type annotation for whole object? | ||
|
||
|
||
==== tests/cases/compiler/excessPropertyCheckWithSpread.ts (1 errors) ==== | ||
declare function f({ a: number }): void | ||
~~~~~~ | ||
!!! error TS2842: Variable 'number' is not used. Did you mean to write a type annotation for whole object? | ||
interface I { | ||
readonly n: number; | ||
} | ||
declare let i: I; | ||
f({ a: 1, ...i }); | ||
|
||
interface R { | ||
opt?: number | ||
} | ||
interface L { | ||
opt: string | ||
} | ||
declare let l: L; | ||
declare let r: R; | ||
f({ a: 1, ...l, ...r }); | ||
|
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.
It looks like
name
anddiagnostic
are always predictable / derivable fromnode
, so this can just beNode[]
like the others. (I don’t think there’s a need to try to make it general-purpose for future use.) I also don’t like the name but it’s extremely hard to come up with anything better...potentialUnusedRenamedBindingElementsInTypes
? 😅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.
Thank you for feedback, fixed 🙂