8000 chore: enable no-unreachable-loop by abrahamguo · Pull Request #9540 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

chore: enable no-unreachable-loop #9540

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 5 commits into from
Jul 20, 2024
Merged

chore: enable no-unreachable-loop #9540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0d0e907
no-unreachable-loop
abrahamguo Jul 11, 2024
47050cb
add test
abrahamguo Jul 11, 2024
ea884fb
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into n…
abrahamguo Jul 16, 2024
74bae9e
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into n…
abrahamguo Jul 19, 2024
8bf8fd3
Merge branch 'main' into no-unreachable-loop
Josh-Cena Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eslint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ export default tseslint.config(
{ commentPattern: '.*intentional fallthrough.*' },
],
'no-lonely-if': 'error',
'no-unreachable-loop': 'error',
'no-useless-call': 'error',
'no-useless-computed-key': 'error',
'no-useless-concat': 'error',
Expand Down
21 changes: 9 additions & 12 deletions packages/eslint-plugin/src/rules/no-mixed-enums.ts
10000 View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,18 +170,15 @@ export default createRule({
.getSymbolAtLocation(tsNode)!
.getDeclarations()!;

for (const declaration of declarations) {
for (const member of (declaration as ts.EnumDeclaration).members) {
return member.initializer
? tsutils.isTypeFlagSet(
typeChecker.getTypeAtLocation(member.initializer),
ts.TypeFlags.StringLike,
)
? AllowedType.String
: AllowedType.Number
: AllowedType.Number;
}
}
const [{ initializer }] = (declarations[0] as ts.EnumDeclaration)
Copy link
Member

Choose a reason for hiding this comment

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

Love this because this exposes a bug (not sure if it's intentional)! Checking declarations[0] is almost always a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this truly a bug? Is it possible for a symbol to have multiple declarations, that would be different from each other?

Copy link
Member

Choose a reason for hiding this comment

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

No; if a symbol returns multiple declarations, that means these sites are merged together and should be treated as one. That's my understanding; I haven't verified it.

.members;
return initializer &&
tsutils.isTypeFlagSet(
typeChecker.getTypeAtLocation(initializer),
ts.TypeFlags.StringLike,
)
? AllowedType.String
: AllowedType.Number;
}

// Finally, we default to the type of the first enum member
Expand Down
3 changes: 2 additions & 1 deletion packages/eslint-plugin/src/rules/no-unsafe-return.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ export default createRule({
});
}

for (const signature of functionType.getCallSignatures()) {
const signature = functionType.getCallSignatures().at(0);
if (signature) {
const functionReturnType = signature.getReturnType();
const result = isUnsafeAssignment(
returnNodeType,
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ function foo(): Set<number> {
return [] as any[];
}
`,
'const foo: (() => void) | undefined = () => 1;',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

functionType.getCallSignatures() can return an empty array, but that code path was not being tested. This ensures that the code path is now covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was brought to light because when I initially refactored the rule to assume that getCallSignatures()[0] was always defined, the tests passed, but the rule crashed when linting this repo, therefore showing that a test was missing.

],
invalid: [
{
Expand Down
Loading
0