Skip to content

fix(biome_js_analyze): mark noThisInStatic fix as unsafe#10588

Open
mvanhorn wants to merge 1 commit into
biomejs:mainfrom
mvanhorn:fix/10278-no-this-in-static-unsafe
Open

fix(biome_js_analyze): mark noThisInStatic fix as unsafe#10588
mvanhorn wants to merge 1 commit into
biomejs:mainfrom
mvanhorn:fix/10278-no-this-in-static-unsafe

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

The noThisInStatic autofix is now marked unsafe, since replacing this in a static context can change behavior.

Why this matters

Reported in #10278: the rule's fix was classified Safe, so it applied automatically. Rewriting this references inside a static member is not always behavior-preserving (for example when this refers to the class via a getter/inheritance chain), so the fix belongs in the unsafe tier where the user opts in.

Changes

  • Change noThisInStatic's fix_kind from Safe to Unsafe and update the rule's test snapshots accordingly.

Testing

Updated invalid.js spec and snapshot reflect the unsafe fix classification.

Fixes #10278

AI was used for assistance.

@changeset-bot

changeset-bot Bot commented Jun 8, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: ed7f682

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

❌ Automation signals

Activity patterns show signs of automation.

View full analysis →

This is an automated analysis by AgentScan

@mvanhorn mvanhorn mentioned this pull request Jun 8, 2026
Open
1 task
@github-actions github-actions Bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Jun 8, 2026
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bcaad0d5-5949-4b64-ac74-af313fd152d8

📥 Commits

Reviewing files that changed from the base of the PR and between 6e24640 and ed7f682.

⛔ Files ignored due to path filters (1)
  • crates/biome_js_analyze/tests/specs/complexity/noThisInStatic/invalid.js.snap is excluded by !**/*.snap and included by **
📒 Files selected for processing (2)
  • crates/biome_js_analyze/src/lint/complexity/no_this_in_static.rs
  • crates/biome_js_analyze/tests/specs/complexity/noThisInStatic/invalid.js

Walkthrough

This PR reclassifies the NoThisInStatic lint rule's automatic fix from Safe to Unsafe and adds test coverage. The rule detects usage of this in static contexts, but its automatic fix can alter program semantics in complex scenarios. A new test case demonstrates this with a custom Symbol.hasInstance implementation that walks the prototype chain and performs type checking—a pattern the safe fix would mishandle.

Possibly related PRs

  • biomejs/biome#10176: Also modifies the NoThisInStatic rule, altering its logic to skip reporting on new this(...) patterns.

Suggested labels

A-Linter, L-JavaScript

Suggested reviewers

  • ematipico
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR changes the fix classification from Safe to Unsafe but doesn't implement the more robust fixer improvements or comprehensive edge-case tests outlined in #10278. Consider whether the fix classification change alone addresses the core objective, or if additional fixer robustness and test coverage for complex patterns (static methods, prototype checks, nested returns) are required per #10278.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change—reclassifying the noThisInStatic fix from Safe to Unsafe.
Description check ✅ Passed The description clearly explains why the fix was reclassified, referencing the behavioural semantics issue and linking to #10278.
Out of Scope Changes check ✅ Passed Changes are narrowly scoped to the rule's fix_kind classification and test snapshot updates, with no unrelated formatting or analyser changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codspeed-hq

codspeed-hq Bot commented Jun 8, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 58 untouched benchmarks
⏩ 196 skipped benchmarks1

Comparing mvanhorn:fix/10278-no-this-in-static-unsafe (ed7f682) with main (80449d6)

Open in CodSpeed

Footnotes

  1. 196 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

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

Reviewers

No reviews

Assignees

No one assigned

Labels

A-Linter Area: linter agentscan:automated-account L-JavaScript Language: JavaScript and super languages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

💅 noThisInStatic safe fix is still not safe in more complex scenarios

1 participant

@mvanhorn