fix: reduce false positives from local variable shadowing and early-return guards#687
fix: reduce false positives from local variable shadowing and early-return guards#687Akshay090 wants to merge 2 commits into
Conversation
…rowser APIs The Identifier visitor that tracks locally declared names was missing ArrowFunctionExpression and FunctionExpression parent types. This caused false positives when callback parameters happened to share a name with a browser API (e.g. `items.map(scheduler => scheduler.name)` flagged as unsupported Window.scheduler usage). Made-with: Cursor
The existing isInsideIfStatement heuristic only suppressed errors for
API usage inside an if block. The common early-return pattern was missed:
if (!('serviceWorker' in navigator)) { return; }
navigator.serviceWorker.register('/sw.js'); // falsely flagged
Now walks up from the flagged node to its containing block and checks
preceding sibling if-statements for early exits (return/throw) whose
test references the same API. Respects ignoreConditionalChecks setting.
Made-with: Cursor
|
These are really useful features, especially early-return @amilajack please, look at this PR 🙏 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83880d3545
ℹ️ 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".
| if (node.type === "Identifier") { | ||
| return node.name === rule.object || node.name === rule.property; |
There was a problem hiding this comment.
Require matching the guarded member before suppressing
For member APIs this treats any mention of the host object as a valid guard, so a preceding unrelated check like if (!navigator.onLine) return; navigator.serviceWorker.register(...) suppresses the Safari 10.1 navigator.serviceWorker error even though the guard never checked serviceWorker. This creates false negatives whenever an early-return condition references the same object but a different property.
Useful? React with 👍 / 👎.
| stmt.type === "IfStatement" && | ||
| containsEarlyExit(stmt.consequent) && | ||
| expressionReferencesApi(stmt.test, failingRule) |
There was a problem hiding this comment.
Check guard polarity before accepting early returns
The early-return suppression only checks that the condition mentions the API, so an inverted guard such as if (navigator.serviceWorker) return; navigator.serviceWorker.register('/sw.js') is considered safe even though the call only runs when serviceWorker is absent. This hides real compatibility errors for positive early exits that skip the supported path rather than guarding the unsupported one.
Useful? React with 👍 / 👎.
| type === "FunctionExpression" || // ex. arr.map(function(Set) {}) | ||
| type === "ArrowFunctionExpression" || // ex. arr.map(Set => Set.id) |
There was a problem hiding this comment.
Scope callback parameter shadowing before dropping errors
These new parent types add callback parameter names to the file-wide identifiers set, so one local parameter suppresses every same-named API error in the file. For example, after items.map(fetch => fetch.id); fetch('/api'), the real unsupported global fetch call is dropped at Program:exit because fetch was recorded from the arrow parameter; this needs scope-aware filtering instead of a global name set.
Useful? React with 👍 / 👎.
Fixes #688
Summary
Two related fixes that reduce false positives in the
compat/compatrule:1. Function parameter names matching browser APIs
The
Identifiervisitor that tracks locally declared names was missingArrowFunctionExpressionandFunctionExpressionparent types. This caused false positives when callback parameters happened to share a name with a browser API.Example — flagged incorrectly before this fix:
The existing heuristic already handled
FunctionDeclarationparams,VariableDeclarator,Property,ClassDeclaration, and import specifiers. This adds the two missing function types for consistency.2. Early-return guard pattern as feature detection
The existing
isInsideIfStatementheuristic only suppressed errors for API usage inside anifblock. The common early-return guard pattern was missed:The new
isGuardedByEarlyReturnhelper walks up from the flagged node to its containing block and checks preceding siblingif-statements for early exits (return/throw) whose test references the same API. Respects theignoreConditionalCheckssetting.Test plan
inoperator, member expression,throw, bare identifier)ignoreConditionalChecksoverrides early-return detection)