@typescript-eslint/no-deprecated#491
Conversation
ScriptedAlchemy
changed the title
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new TypeScript linting rule, Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the @typescript-eslint/no-deprecated rule, a substantial feature that includes the rule's implementation, documentation, and tests. The changes are well-structured. My review focuses on a couple of opportunities to improve code clarity and remove redundancy. Specifically, I've pointed out a duplicated code block in the API handler and a potential simplification in an AST traversal helper function. Overall, great work on adding this complex rule.
There was a problem hiding this comment.
Pull request overview
Adds support for the @typescript-eslint/no-deprecated rule to the TypeScript plugin, including parity tests against the upstream typescript-eslint test suite and wiring the rule into the registry/test runner.
Changes:
- Implement
no-deprecatedrule in Go (TypeScript plugin) with allow-list support and deprecation-reason extraction. - Add/enable TypeScript-eslint compatibility tests + snapshot for
no-deprecated. - Register the rule in the global rule registry and adjust IPC lint defaults.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/rslint-test-tools/tests/typescript-eslint/rules/no-deprecated.test.ts | Updates rule-tester cases (JSX languageOptions/noFormat) and skips a few React-type-dependent cases. |
| packages/rslint-test-tools/tests/typescript-eslint/rules/snapshots/no-deprecated.test.ts.snap | Adds snapshot output for invalid cases for the new rule. |
| packages/rslint-test-tools/rstest.config.mts | Enables the no-deprecated test in rstest config. |
| internal/plugins/typescript/rules/no_deprecated/no_deprecated.go | New rule implementation for detecting deprecated symbol usage. |
| internal/plugins/typescript/rules/no_deprecated/no_deprecated_test.go | Adds Go unit tests for allow-list import matching and basic rule behavior. |
| internal/plugins/typescript/rules/no_deprecated/no_deprecated.md | Adds minimal rule documentation + upstream reference. |
| internal/config/config.go | Registers @typescript-eslint/no-deprecated in the TypeScript-eslint plugin registry. |
| cmd/rslint/api.go | Changes IPC lint behavior to default-enable all rules when none are specified (now redundantly implemented). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6364c5745
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…precated-92ca # Conflicts: # packages/rslint-test-tools/rstest.config.mts
Deploying rslint with
|
| Latest commit: |
ad15bbe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a3346a70.rslint.pages.dev |
| Branch Preview URL: | https://codex-ts-eslint-no-deprecate.rslint.pages.dev |
fansenze
left a comment
fansenze
left a comment
There was a problem hiding this comment.
Review of the no-deprecated port (P0/P1 findings). Several conclusions are backed by ablation/probe experiments, noted inline. Not blocking — happy to discuss.
| return taggedTemplate != nil && taggedTemplate.Tag == node | ||
| } | ||
|
|
||
| func isDeprecatedNodeAssertFailOverload(ctx rule.RuleContext, node *ast.Node, name string) (bool, string) { |
There was a problem hiding this comment.
These two helpers (isDeprecatedNodeAssertFailOverload here, isDeprecatedFsExistsImport at L1880) hard-code the API name, package, and the full deprecation reason string including version numbers.
I ablated both (forced return false) and reran the suite: import assert from 'node:assert'; assert.fail({}, {}) fails immediately — this case is supported only by the hard-coding (fs.exists is structurally identical). The same root cause appears elsewhere: the skipped <div aria-grabbed /> test also yields no diagnostic because the deprecation lives in lib.dom.d.ts. So @deprecated from an external .d.ts (lib / node_modules) isn't detected, and instead two specific APIs are patched in by hand — everything else from node_modules goes undetected. The root cause is worth fixing rather than per-API patching.
| return found, reason | ||
| } | ||
|
|
||
| func deprecatedVariableInfoByNameInSource(ctx rule.RuleContext, name string) (bool, string) { |
There was a problem hiding this comment.
This *ByNameInSource family walks the whole SourceFile and matches declarations by name string, detached from symbol resolution (used as a fallback in checkIdentifier, L2085).
This produces false positives when a local variable/parameter shadows a deprecated declaration of the same name — the reported identifier resolves to the shadowing symbol, which is not deprecated, yet gets flagged. Verified with two probes:
/** @deprecated */ const foo = 1;
function bar(foo: number) { return foo; } // FALSE POSITIVE on `foo`/** @deprecated */ const value = 1;
function bar() { const value = 2; return value; } // FALSE POSITIVE on `value`Detection should rely on the resolved symbol, not name matching.
| // Skipped: React JSX deprecation requires React types (tsgolint parity) | ||
| { | ||
| code: ` | ||
| skip: true, |
There was a problem hiding this comment.
4 invalid JSX cases are skipped (here + L2956, L2971, L3000) with the note "requires React types". I removed the skips and ran each:
- AProps (here),
foo-bar:baz-bam(L2971),Tab.List(L3000) all PASS — the rule reports the deprecated prop with the correct messageId + line/column. Their@deprecatedcomes from local declarations (a plaininterface), so "requires React types" doesn't apply; skipping them just drops working coverage. - Only
const a = <div aria-grabbed />(L2956) genuinely fails (no diagnostic) — the deprecation is inlib.dom.d.ts(external), same root cause as the hard-coded node_modules helpers.
Please un-skip the 3 that pass, and track the external-lib case (aria-grabbed) explicitly rather than skipping silently.
| ctx.ReportRange(diagnosticRange, message) | ||
| } | ||
| if sourceFile != nil { | ||
| diagnostics := ctx.TypeChecker.GetSuggestionDiagnostics(context.Background(), sourceFile) |
There was a problem hiding this comment.
Two parallel detection paths — GetSuggestionDiagnostics (here) + the hand-written AST listeners (L2155+) — deduped by text range via reportedRanges (L1996). By inspection (not reproduced) this looks fragile: if the two paths compute slightly different ranges, dedup fails and you get duplicate reports. There are also text-based heuristics: isInImportStatementRange (L1364) classifies "inside an import" by parsing the source line as text, and hasDeprecatedTagInSource (L205) scans characters backward for /**. Upstream is purely symbol/type driven; these heuristics are a smell.
| sourceFiles[filePath] = sourceFile | ||
| sourceFilesLock.Unlock() | ||
|
|
||
| if len(req.RuleOptions) == 0 { |
There was a problem hiding this comment.
Unrelated infra changes, with an empty PR description:
- This
len(req.RuleOptions) == 0branch changes global behavior (runs config-enabled rules instead of nothing when no options are passed). RuleCount(L291) changes from "configured" to "executed".RequiresTypeInfo(L259): by code reading this appears to be a no-op on the IPC path (noTypeInfoFilesis passed and IPC never callsFilterNonTypeAwareRules) — if it's actually needed, add a test; if not, drop it.
Please split infra changes into a separate PR and document them.
|
|
||
| // Remove length bytes from buffer | ||
| this.chunks = [combined.slice(4)]; | ||
| this.chunks = [combined.subarray(4)]; |
There was a problem hiding this comment.
Unrelated to the rule. On the Node side (node.ts) Buffer.slice→subarray is fine (slice is deprecated). But here combined is a Uint8Array: .slice() copies while .subarray() returns a shared view — different memory semantics on the IPC message boundary, with no test coverage. I haven't reproduced a concrete bug, but it's an aliasing risk worth isolating and justifying.
No description provided.