Skip to content

JS-1389 Fix FP in S6767: props reported unused when component wraps React.forwardRef#6591

Open
sonar-nigel[bot] wants to merge 10 commits intomasterfrom
fix/JS-1389-fix-fp-on-s6767-props-reported-unused-when-component-is-wrapped-with-reactforwardref-sonnet
Open

JS-1389 Fix FP in S6767: props reported unused when component wraps React.forwardRef#6591
sonar-nigel[bot] wants to merge 10 commits intomasterfrom
fix/JS-1389-fix-fp-on-s6767-props-reported-unused-when-component-is-wrapped-with-reactforwardref-sonnet

Conversation

@sonar-nigel
Copy link
Contributor

@sonar-nigel sonar-nigel bot commented Mar 13, 2026

Fixes a false positive in rule S6767 where props are incorrectly reported as unused when an outer function component passes props via closure into a nested React.forwardRef() or forwardRef() callback.

Root Cause

The upstream no-unused-prop-types rule tracks prop usage per component scope boundary. It attributes props.label usage to the inner forwardRef scope rather than the outer component, causing false positives in the S6767 decorator.

Changes

  • Extends the S6767 decorator with a forwardRefCalleePatterns array and a containsForwardRefCall helper
  • After the existing hasPropsCall guard, adds a component-scoped check that suppresses the report when the owning component's subtree contains a forwardRef call
  • Suppression requires a non-null componentNode, ensuring only the affected component is suppressed — sibling components with genuinely unused props are still reported
  • Adds test cases covering: React.forwardRef closure pattern, bare forwardRef closure pattern, component-scoped suppression (sibling TP still fires), and TypeScript interface-based props with React.forwardRef
  • No ruling file changes needed: the closure-into-forwardRef FP pattern does not appear in the ruling test corpus

Relates to JS-1389.

Vibe Bot and others added 3 commits March 13, 2026 18:08
Tests cover the scenario where props are reported as unused when accessed
via closure inside a React.forwardRef() or forwardRef() callback. The outer
component's props are attributed to the inner forwardRef scope by the upstream
rule, causing false positives.

The tests verify:
- React.forwardRef closure pattern is suppressed (FP)
- Bare forwardRef closure pattern is suppressed (FP)
- Suppression is component-scoped, not file-wide (TP in sibling component)
- TypeScript interface-based props with React.forwardRef are suppressed
- Upstream rule sentinel confirms the FP pattern still exists upstream

Relates to JS-1389
When an outer function component passes props via closure into a nested
React.forwardRef() or forwardRef() callback, the upstream no-unused-prop-types
rule tracks prop usage per component scope boundary. It attributes props.label
usage to the inner forwardRef scope rather than the outer component, causing
false positives.

The fix extends the S6767 decorator with a composable forwardRefCalleePatterns
array and a containsForwardRefCall helper. After the existing hasPropsCall guard,
a second component-scoped check suppresses the report when the owning component's
subtree contains a forwardRef call. The suppression requires a non-null
componentNode, ensuring only the affected component is suppressed and sibling
components with genuine unused props are still reported.

Implementation follows the approved proposal algorithm. Relates to JS-1389.
Ticket: JS-1389

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2026

Ruling Report

No changes to ruling expected issues in this PR

@sonar-nigel sonar-nigel bot marked this pull request as ready for review March 13, 2026 18:48
@sonar-nigel sonar-nigel bot requested a review from a team March 13, 2026 18:48
@francois-mora-sonarsource
Copy link
Contributor

francois-mora-sonarsource commented Mar 17, 2026

The current suppression fires for the entire component as soon as any forwardRef call is found in its subtree, regardless of which specific prop was flagged. This can produce false negatives: a component that uses forwardRef internally but also has genuinely unused props declared in propTypes would have all its reports silenced.

A more precise approach: suppress only when the specific reported prop is actually referenced inside the forwardRef callback. Rather than walking the AST, use ESLint's scope analysis — it has already built the reference graph for all variables in the component:

  1. Get the component's scope via context.sourceCode.getScope(componentNode)
  2. Look up the props variable by name (componentNode.params[0] gives the parameter, whose .name is the props identifier)
  3. Iterate variable.references — each gives the props Identifier node at a specific use site
  4. For each reference, check whether its parent is a MemberExpression accessing the flagged prop name (non-computed), and whether that reference is lexically inside a forwardRef callback (walk up ancestors to confirm it sits inside the first argument of a matching forwardRef(...) call)

This way, props genuinely used via closure are suppressed, while props that are declared in propTypes but never referenced inside the callback are still reported. It also naturally handles renamed props parameters without hardcoding "props" as a string.

Comment: The current suppression fires for the entire component as soon as any `forwardRef` call is found in its subtree, regardless of which specific prop was flagged. This can produce false negatives: a component that uses `forwardRef` internally but also has genuinely unused props declared in `propTypes` would have all its reports silenced.

A more precise approach would be to suppress only when the specific reported prop name is actually referenced inside the `forwardRef` callback body (the lambda or function expression passed as the first argument). That way, props used via closure are suppressed, but props that are not referenced inside the callback are still reported correctly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 17, 2026

Good catch. The broad suppression was indeed a false-negative risk.

The fix replaces containsForwardRefCall with isPropReferencedInForwardRefCallback, which now:

  1. Finds each forwardRef call in the component's subtree
  2. Checks the first argument (the callback body) for a props.<propName> member expression matching the specific reported prop
  3. Suppresses only if that prop name is actually referenced inside the callback — props that are genuinely unused pass through to context.report

The prop name is extracted from descriptor.data.name, which is what eslint-plugin-react sets when reporting no-unused-prop-types.

A new invalid test case was added to cover the scenario described in the comment: a component with React.forwardRef where label is used inside the callback (suppressed) but color is not (still reported).

@github-actions
Copy link
Contributor

README Freshness Check

The rules README is out of date.

A fix PR has been created: #6618

Please review and merge it into your branch.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@francois-mora-sonarsource
Copy link
Contributor

The new hasPropMemberReference helper hardcodes 'props' as the parameter name — it will silently miss the FP when the outer component names its parameter differently (e.g. ownProps, p). It also re-traverses the component subtree on every reported prop, duplicating work ESLint has already done.

A more robust and efficient approach is to use ESLint's pre-built scope data instead:

  1. Get the component's scope: context.sourceCode.getScope(componentNode)
  2. Read the props parameter name from componentNode.params[0] (if it's an Identifier) — no hardcoded string
  3. Look up the variable: scope.set.get(propsParamName)
  4. Iterate variable.references — ESLint has already collected every props reference site; no traversal needed
  5. For each reference, check whether its parent is a non-computed MemberExpression accessing propName, and whether it sits inside a forwardRef callback (short ancestor walk up to confirm)

This resolves both issues: the props parameter name is taken from the actual binding rather than assumed, and the reference list is O(1) to obtain since ESLint built it during its own traversal.

Vibe Bot and others added 2 commits March 18, 2026 14:41
Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

Conclusion: Solid fix. The scope-based reference walk is precise (suppresses only the specific reported prop, not all props), conservative (rejects destructured params and computed member accesses), and correctly bounded (ancestor walk only matches when the member access is nested inside a forwardRef call, not just anywhere in the component). The sentinel test that verifies the upstream rule still raises the FP is a particularly good safety net.

🗣️ Give feedback

s6767-props-reported-unused-when-component-is-wrapped-with-reactforwardref-sonnet
Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

🗣️ Give feedback

@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 18, 2026

Both issues have been addressed in the latest commit (Use scope references for forwardRef prop suppression).

The updated hasPropMemberReference implementation now:

  1. Reads the actual parameter name from componentNode.params[0] (an Identifier node) — no hardcoded 'props' string
  2. Calls sourceCode.getScope(componentNode).set.get(propsParam.name) to obtain the scope variable
  3. Iterates variable.references directly — ESLint's pre-built reference list; no subtree re-traversal

The ancestor walk is now a short upward climb (isPropReferenceInForwardRefCallback walks parent nodes from the MemberExpression up to find the enclosing forwardRef call), not a downward subtree scan.

A test case with the renamed 'ownProps' parameter was also added to confirm the fix handles non-default prop parameter names correctly.

@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 18, 2026

Thank you for the thorough review!

@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 18, 2026

Thank you for the review!

@francois-mora-sonarsource
Copy link
Contributor

LGTM - fixes S6767 false positives when outer function component props are accessed via closure inside a nested React.forwardRef() or forwardRef() callback — #6591

@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 18, 2026

Thank you for the review and approval!

The ancestor walk in isPropReferenceInForwardRefCallback now tracks
the previous node to confirm the reference sits specifically inside
call.arguments[0] (the render callback), not in the callee position
or a later argument of the forwardRef call.

Also fixes a stale comment in unit.test.ts that still referenced the
old containsForwardRefCall approach.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 19, 2026

Good catch — the ancestor walk was finding any forwardRef CallExpression in the ancestry chain without verifying the reference sits inside its first argument specifically. I've updated isPropReferenceInForwardRefCallback to track the previous node as we walk up, and added call.arguments[0] === prev to the check. This ensures suppression only fires when the prop member access is lexically inside the render callback (arguments[0]), not in the callee position or a later argument.

Also fixed a stale comment in the TypeScript test that still referenced containsForwardRefCall (the old approach).

@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 19, 2026

Thank you for the review!

s6767-props-reported-unused-when-component-is-wrapped-with-reactforwardref-sonnet
@sonarqube-next
Copy link

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

🗣️ Give feedback

@sonar-nigel
Copy link
Contributor Author

sonar-nigel bot commented Mar 19, 2026

Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant