-
Notifications
You must be signed in to change notification settings - Fork 204
Enable the @typescript-eslint/no-unsafe-return rule #4110
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
Enable the @typescript-eslint/no-unsafe-return rule #4110
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.
Pull Request Overview
This PR enables the @typescript-eslint/no-unsafe-return
ESLint rule that was previously disabled during the ESLint 9 migration. The changes primarily involve adding type assertions and proper typing to resolve unsafe return warnings.
- Adds type assertions using
as
to ensure type safety in return statements - Updates function signatures and variable declarations with proper typing
- Implements explicit handling for unsafe returns in production code
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
eslint.config.mjs | Removes the disabled @typescript-eslint/no-unsafe-return rule |
test/vscode-tests/no-workspace/query-testing/test-runner.test.ts | Adds type import and type assertion for mocked fs-extra |
test/vscode-tests/no-workspace/language-support/ast-viewer/ast-builder.test.ts | Updates function signatures and adds type assertions for BQRS data |
test/vscode-tests/no-workspace/index.ts | Improves typing for mock extension context methods |
test/vscode-tests/no-workspace/codeql-cli/distribution.test.ts | Adds type assertion for mocked os module |
test/unit-tests/variant-analysis/custom-errors.test.ts | Adds explicit return type handling for error messages |
test/unit-tests/common/invocation-rate-limiter.test.ts | Adds type assertion for map value retrieval |
test/unit-tests/common/disposable-object.test.ts | Fixes typo and adds void operator for dispose calls |
test/mocked-object.ts | Adds ESLint disable comments for intentionally unsafe returns |
test/mock-memento.ts | Improves type parameter consistency |
src/view/common/SuggestBox/useEffectEvent.ts | Adds ESLint disable comment for callback return |
src/view/common/SuggestBox/SuggestBox.tsx | Extracts interface for styled component props |
src/variant-analysis/shared/variant-analysis.ts | Replaces Object.values reduce with explicit property access |
src/extension.ts | Adds void operator to dispose calls |
src/common/mock-gh-api/recorder.ts | Adds type assertion for JSON parsing |
src/codeql-cli/cli.ts | Adds type assertion for query resolution result |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
||
return data.message; | ||
if (typeof data.message === "string") { | ||
return data.message as 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.
The type assertion as string
is redundant since the condition already confirms data.message
is a string type. Remove the type assertion and return data.message
directly.
return data.message as string; | |
return data.message; |
Copilot uses AI. Check for mistakes.
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.
Unfortunately not according to eslint / typescript. This is needed.
width: 100%; | ||
font-family: var(--vscode-editor-font-family); | ||
|
||
${(props) => | ||
${(props: InputProps) => |
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.
The explicit type annotation props: InputProps
is unnecessary since TypeScript can infer the type from the generic parameter. Remove the type annotation for cleaner code.
${(props: InputProps) => | |
${(props) => |
Copilot uses AI. Check for mistakes.
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.
Unfortunately not according to eslint / typescript. This is needed.
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.
LGTM, just one optional suggestion for getSkippedRepoCount
to keep exactly the same runtime code
return ( | ||
(skippedRepos.accessMismatchRepos?.repositoryCount ?? 0) + | ||
(skippedRepos.notFoundRepos?.repositoryCount ?? 0) + | ||
(skippedRepos.noCodeqlDbRepos?.repositoryCount ?? 0) + | ||
(skippedRepos.overLimitRepos?.repositoryCount ?? 0) |
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 think this can be fixed while retaining the same method by using some type parameters:
return ( | |
(skippedRepos.accessMismatchRepos?.repositoryCount ?? 0) + | |
(skippedRepos.notFoundRepos?.repositoryCount ?? 0) + | |
(skippedRepos.noCodeqlDbRepos?.repositoryCount ?? 0) + | |
(skippedRepos.overLimitRepos?.repositoryCount ?? 0) | |
return Object.values(skippedRepos).reduce<number>( | |
(acc, group: VariantAnalysisSkippedRepositoryGroup) => | |
acc + group.repositoryCount, | |
0, |
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.
Hmm, I thought I tried that and it didn't like it, but I'll give it another go.
This PR enables the
@typescript-eslint/no-unsafe-return
rule that was disabled in the eslint 9 migration.Each case is a little bit different. A lot were fixed by using
as
, though some needed other changes. I think the only production code change isgetSkippedRepoCount
and everything else is either just compile-time or is in test code.