Skip to content

fix(noExtraNonNullAssertion): do not flag valid non-null assertions in assignment expressions #8325

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.

Sign up for GitHub

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

Jump to bottom
Open
hirokiokada77 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(noExtraNonNullAssertion): do not flag valid non-null assertions in assignment expressions #8325

hirokiokada77 wants to merge 1 commit into from

Conversation

@hirokiokada77
Copy link
Contributor

@hirokiokada77 hirokiokada77 commented Dec 1, 2025

Summary

This PR removes an apparently redundant match arm in the implementation of the noExtraNonNullAssertion rule, which was incorrectly flagging valid uses of non-null assertions in assignment expressions like a! += b!.

Closes #8314.

Test Plan

A test case was added to:

  • crates/biome_js_analyze/tests/specs/suspicious/noExtraNonNullAssertion/valid.ts

@changeset-bot
Copy link

changeset-bot bot commented Dec 1, 2025

🦋 Changeset detected

Latest commit: 47e5426

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@biomejs/biome Patch
@biomejs/cli-win32-x64 Patch
@biomejs/cli-win32-arm64 Patch
@biomejs/cli-darwin-x64 Patch
@biomejs/cli-darwin-arm64 Patch
@biomejs/cli-linux-x64 Patch
@biomejs/cli-linux-arm64 Patch
@biomejs/cli-linux-x64-musl Patch
@biomejs/cli-linux-arm64-musl Patch
@biomejs/wasm-web Patch
@biomejs/wasm-bundler Patch
@biomejs/wasm-nodejs Patch
@biomejs/backend-jsonrpc Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Dec 1, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 1, 2025

Walkthrough

This PR addresses a false positive in the noExtraNonNullAssertion lint rule. The rule previously flagged non-null assertions in compound assignment expressions (such as a! += b!) as invalid even when the assertions applied to different variables. The fix removes the parent validation for TsNonNullAssertionAssignment contexts, expanding the scenarios considered valid. A corresponding test case validates the corrected behaviour.

Suggested labels

A-Linter, L-JavaScript

Suggested reviewers

  • dyc3

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main fix: preventing the noExtraNonNullAssertion rule from flagging valid non-null assertions in assignment expressions.
Description check ✅ Passed The description clearly explains the problem (redundant match arm causing false positives), the solution (removing it), and references the test case added and issue closed.
Linked Issues check ✅ Passed The PR successfully addresses issue #8314 by removing the faulty match arm that was causing false positives when two non-null assertions appeared on different operands in assignment expressions [#8314].
Out of Scope Changes check ✅ Passed All changes are in-scope: the rule implementation fix, related test additions to valid.ts, and the changeset entry directly address the requirements from issue #8314.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1da899b and 47e5426.

⛔ Files ignored due to path filters (1)
  • crates/biome_js_analyze/tests/specs/suspicious/noExtraNonNullAssertion/valid.ts.snap is excluded by !**/*.snap and included by **
📒 Files selected for processing (3)
  • .changeset/wise-parents-hang.md (1 hunks)
  • crates/biome_js_analyze/src/lint/suspicious/no_extra_non_null_assertion.rs (0 hunks)
  • crates/biome_js_analyze/tests/specs/suspicious/noExtraNonNullAssertion/valid.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • crates/biome_js_analyze/src/lint/suspicious/no_extra_non_null_assertion.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts

📄 CodeRabbit inference engine (CONTRIBUTING.md)

For Node.js package development, build WebAssembly bindings and JSON-RPC bindings; run tests against compiled files after implementing features or bug fixes

Files:

  • crates/biome_js_analyze/tests/specs/suspicious/noExtraNonNullAssertion/valid.ts
.changeset/**/*.md

📄 CodeRabbit inference engine (CONTRIBUTING.md)

.changeset/**/*.md: Create changesets for user-facing changes using just new-changeset; use headers with #### or ##### only; keep descriptions concise (1-3 sentences) and focus on user-facing changes
Use past tense when describing what was done ('Added new feature'), present tense when describing Biome behavior ('Biome now supports'); end sentences with a full stop
For new lint rules, show an example of an invalid case in an inline code snippet or code block; for rule changes, demonstrate what is now invalid or valid; for formatter changes, use a diff code block

Files:

  • .changeset/wise-parents-hang.md
🧠 Learnings (5)
📚 Learning: 2025-11-27T23:04:02.022Z 
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/tests/specs/**/*.{js,ts,tsx,jsx,json,css} : Test rules using snapshot tests via the `insta` library with test cases in `tests/specs/<group>/<rule_name>/` directories prefixed by `invalid` or `valid`

Applied to files:

  • crates/biome_js_analyze/tests/specs/suspicious/noExtraNonNullAssertion/valid.ts
📚 Learning: 2025-11-27T23:04:02.022Z 
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/tests/specs/**/*.jsonc : Use `.jsonc` files in test specs with code snippets as array of strings to test rules in script environment (no import/export syntax)

Applied to files:

  • crates/biome_js_analyze/tests/specs/suspicious/noExtraNonNullAssertion/valid.ts
📚 Learning: 2025-11-27T23:04:02.022Z 
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noUnsafe` prefix for rules that report code leading to runtime failures (e.g., `noUnsafeOptionalChaining`)

Applied to files:

  • .changeset/wise-parents-hang.md
📚 Learning: 2025-11-27T23:04:02.022Z 
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noConstant` prefix for rules that report computations always evaluated to the same value (e.g., `noConstantMathMinMaxClamp`)

Applied to files:

  • .changeset/wise-parents-hang.md
📚 Learning: 2025-11-27T23:04:02.022Z 
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noInvalid` prefix for rules that report runtime errors from mistyping (e.g., `noInvalidConstructorSuper`)

Applied to files:

  • .changeset/wise-parents-hang.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: Test (depot-ubuntu-24.04-arm-16)
  • GitHub Check: Documentation
  • GitHub Check: End-to-end tests
  • GitHub Check: Lint project (depot-windows-2022)
  • GitHub Check: Test (depot-windows-2022-16)
  • GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
  • GitHub Check: Check Dependencies
  • GitHub Check: Bench (biome_js_analyze)
  • GitHub Check: Test Node.js API
  • GitHub Check: Bench (biome_js_parser)
  • GitHub Check: autofix
  • GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (2)
.changeset/wise-parents-hang.md (1)

1-5: Clear, user‑facing changeset description

This reads well and matches the guidelines: concise, correct tense, links the issue, and shows the now‑valid example inline. No tweaks needed.

crates/biome_js_analyze/tests/specs/suspicious/noExtraNonNullAssertion/valid.ts (1)

23-26: Good regression test covering the reported pattern

Nice addition: the a! += b! case with separate operands mirrors the issue report and rightly lives in valid.ts, so the rule won’t regress here again.

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
Copy link

codspeed-hq bot commented Dec 1, 2025

CodSpeed Performance Report

Merging #8325 will not alter performance

Comparing hirokiokada77:fix/no-extra-non-null-assertion (47e5426) with main (8de2774)1

Summary

✅ 58 untouched
⏩ 95 skipped2

Footnotes

  1. No successful run was found on main (1da899b) during the generation of this report, so 8de2774 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

  2. 95 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 L-JavaScript Language: JavaScript and super languages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

💅 noExtraNonNullAssertion false positive with two ! on different variables

1 participant

@hirokiokada77