Skip to content

Forbid unused property renaming in destructuring binding in function types #41044

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.

Sign up for GitHub

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

Merged
merged 31 commits into from
Jun 14, 2022
Merged

Forbid unused property renaming in destructuring binding in function types #41044

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
4aec8ef
Forbid renaming a propertyin function type parameters
uhyo Oct 11, 2020
a37d6d4
add tests
uhyo Oct 11, 2020
67732d3
Remove renaming from declaration output
uhyo Oct 11, 2020
1d114e6
accept baseline
uhyo Oct 11, 2020
4abcfa1
accept baseline
uhyo Jun 8, 2022
39e90ce
renew tests (not very right now)
uhyo Jun 8, 2022
79ef4fd
get correct result
uhyo Jun 8, 2022
69b8a58
update diagnostic text
uhyo Jun 8, 2022
4ddd9ae
accept baseline
uhyo Jun 8, 2022
e210484
add declaration emit test
uhyo Jun 8, 2022
2797958
fix declaration emit
uhyo Jun 8, 2022
cb326d2
fix formatting
uhyo Jun 8, 2022
17a29ff
revert unnecessary change
uhyo Jun 8, 2022
c4c80db
accept baseline
uhyo Jun 8, 2022
1e3dc2e
extend tests
uhyo Jun 8, 2022
32c340e
Revert "revert unnecessary change"
uhyo Jun 8, 2022
9c9fe5e
accept baseline
uhyo Jun 8, 2022
f0e5112
Rename and refactor potentialAlways... stuff
uhyo Jun 9, 2022
7aa2b6e
add non-identifier names
uhyo Jun 9, 2022
755c7c4
extend check to non-identifier original property names
uhyo Jun 9, 2022
c7768cf
update diagnostic message
uhyo Jun 9, 2022
bdb42e1
add related span
uhyo Jun 9, 2022
6d4d147
accept baseline
uhyo Jun 9, 2022
bc92945
add symbol-keyed test case
uhyo Jun 9, 2022
e2710b7
oops?
uhyo Jun 9, 2022
1e4c14b
workaround for unstable test
uhyo Jun 9, 2022
4e519ff
fix suggested name
uhyo Jun 10, 2022
07f162f
add comment about non-identifier property names
uhyo Jun 10, 2022
6c8a95b
simplify isReferenced check
uhyo Jun 10, 2022
bd0da30
accept baseline
uhyo Jun 10, 2022
600b614
move it one step further
uhyo Jun 10, 2022
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
update diagnostic text
  • Loading branch information
@uhyo
uhyo committed Jun 8, 2022
commit 69b8a58ca1fbd5530cd28949670699c15a7730ab
20 changes: 12 additions & 8 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,7 +1028,7 @@ namespace ts {
const potentialReflectCollisions: Node[] = [];
const potentialAlwaysCheckedUnusedTypes: {
Copy link
Member

@andrewbranch andrewbranch Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like name and diagnostic are always predictable / derivable from node, so this can just be Node[] 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? 😅
Copy link
Contributor Author

@uhyo uhyo Jun 9, 2022

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 🙂
node: Node;
name: BindingName;
name: Identifier;
diagnostic: DiagnosticMessage;
}[] = [];
const awaitedTypeStack: number[] = [];
Expand Down Expand Up @@ -37579,6 +37579,14 @@ namespace ts {
});
}
Copy link
Member

@andrewbranch andrewbranch Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, isReferenced is marked with Type in the case of typeof renamedThing? I would have thought that counts as a value usage. The tests look right so I assume this works, I’m just surprised. Do we need to check against Type at all or can we just say if (!(getSymbolOfNode(node)?.isReferenced)?
Copy link
Contributor Author

@uhyo uhyo Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently just checking truthiness of isReferenced is enough. Thank you 🙂

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:
Expand Down Expand Up @@ -37933,7 +37941,7 @@ namespace ts {
potentialAlwaysCheckedUnusedTypes.push({
node,
name: node.name,
diagnostic: Diagnostics.Renaming_a_property_in_destructuring_assignment_is_only_allowed_in_a_function_or_constructor_implementation
diagnostic: Diagnostics.Variable_0_is_not_used_Did_you_mean_to_write_a_type_annotation_for_whole_object
});
return;
}
Expand Down Expand Up @@ -41801,12 +41809,8 @@ namespace ts {
}
});
}
if (!node.isDeclarationFile && potentialAlwaysCheckedUnusedTypes.length) {
for (const { node, name, diagnostic } of potentialAlwaysCheckedUnusedTypes) {
if (!(getSymbolOfNode(node)?.isReferenced! & SymbolFlags.Type)) {
error(name, diagnostic);
}
}
if (!node.isDeclarationFile) {
checkPotentialAlwaysCheckedUnusedTypes();
}
});

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3487,7 +3487,7 @@
"category": "Error",
"code": 2841
},
"Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.": {
"Variable '{0}' is not used. Did you mean to write a type annotation for whole object?": {
Copy link
Member

@andrewbranch andrewbranch Jun 8, 2022

Choose a reason for hiding this comment

The 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 Renaming (or rebinding?) destructured parameters is not allowed in function types... I think the message should say what it is you’ve written, but everything sounds 🤢
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Variable '{0}' is unused - did you intend to use it as a type annotation for '{1}'?

Related span on the end of the object if a type annotation doesn't already exist

We can only write a type for '{0}' by adding a type for the entire parameter here.

What do you think?
Copy link
Member

@andrewbranch andrewbranch Jun 8, 2022

Choose a reason for hiding this comment

The 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 {1} being unwieldy (not uncommon to destructure a dozen props in a React component, for example).
Copy link
Member

@andrewbranch andrewbranch Jun 8, 2022

Choose a reason for hiding this comment

The 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 😆
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, {1} would be the original property name, not the local.

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;
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a mash-up?

'{0}' is an unused renaming of '{1}'. Did you intend to use it as a type annotation?
Copy link
Member

@andrewbranch andrewbranch Jun 8, 2022

Choose a reason for hiding this comment

The 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.
Copy link
Contributor Author

@uhyo uhyo Jun 9, 2022

Choose a reason for hiding this comment

The 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?

Screenshot of below snippet

src/index.ts:5:17 - error TS2842: 'string' is an unused renaming of 'a'. Did you intend to use it as a type annotation?

5 type F4 = ({ a: string }) => any; // Error
                  ~~~~~~

  src/index.ts:5:24
    5 type F4 = ({ a: string }) => any; // Error
                             ~
    We can only write a type for '{ a: string }' by adding a type for the entire parameter here.
Copy link
Member

@andrewbranch andrewbranch Jun 9, 2022

Choose a reason for hiding this comment

The 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 end - 1 to end was a little weird, but it shows up pretty well here. I would maybe move it back one character ([end, end + 1]).

Also, mentioned this in the review, but I think it should be We can only write a type for 'a'.
Copy link
Contributor Author

@uhyo uhyo Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed interpolation and moved the span one step further.

スクリーンショット 2022-06-10 11 10 14

I also tested zero-length span but it didn't show any mark.
"category": "Error",
"code": 2842
},
Expand Down
52 changes: 26 additions & 26 deletions tests/baselines/reference/renamingDestructuredPropertyInFunctionType.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,73 +1,73 @@
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(3,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(4,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(5,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(6,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(8,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(13,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(14,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(15,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(16,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(18,17): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(24,16): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(27,9): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(30,13): error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(3,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(4,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(5,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(6,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(8,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(13,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(14,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(15,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(16,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(18,17): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(24,16): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(27,9): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts(30,13): error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?


==== tests/cases/compiler/renamingDestructuredPropertyInFunctionType.ts (13 errors) ====
type O = { a: string; b: number; c: number; };
type F1 = (arg: number) => any; // OK
type F2 = ({ a: string }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type F3 = ({ a: string, b, c }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type F4 = ({ a: string }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type F5 = ({ a: string, b, c }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type F6 = ({ a: string }) => typeof string; // OK
type F7 = ({ a: string, b: number }) => typeof number; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type F8 = ({ a, b: number }) => typeof number; // OK
type F9 = ([a, b, c]) => void; // Error

type G1 = (arg: number) => any; // OK
type G2 = ({ a: string }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type G3 = ({ a: string, b, c }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type G4 = ({ a: string }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type G5 = ({ a: string, b, c }: O) => any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type G6 = ({ a: string }) => typeof string; // OK
type G7 = ({ a: string, b: number }) => typeof number; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
type G8 = ({ a, b: number }) => typeof number; // OK
type G9 = ([a, b, c]) => void; // Error

interface I {
method1(arg: number): any; // OK
method2({ a: string }): any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

(arg: number): any; // OK
({ a: string }): any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

new (arg: number): any; // OK
new ({ a: string }): any; // Error
~~~~~~
!!! error TS2842: Renaming a property in destructuring assignment is only allowed in a function or constructor implementation.
!!! error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?
}