fix: noDelete should not suggest undefined assignment for process.env by myx0m0p · Pull Request #9760 · biomejs/biome

myx0m0p · 2026-04-01T02:58:49Z

Summary

The noDelete rule currently suggests replacing delete process.env.FOO with process.env.FOO = undefined. This is incorrect because:

delete process.env.FOO is the official documented way to remove env vars
Assigning undefined has different behavior from delete for process.env properties
The suggestion can break functionality (e.g., AWS Lambda env reset patterns in unit tests)

Changes

Modified crates/biome_js_analyze/src/lint/performance/no_delete.rs:
- Added check in fn action() to return None when the deleted target is a process.env property
- Handles both process.env.FOO and destructured patterns like const { env } = process; delete env.FOO
Updated test fixtures in tests/specs/performance/noDelete/invalid.jsonc:
- Added test cases for delete process.env.FOO and destructured env variants

Testing

The diagnostic is still reported for delete process.env.FOO (it is still a use of the delete operator), but the unsafe code action (assigning undefined) is no longer offered.

⚠️ Note: Could not run cargo test locally as Rust toolchain is not available in the current environment. CI will validate.

Fixes #4093

The noDelete rule currently suggests replacing delete process.env.FOO with process.env.FOO = undefined. This is incorrect because: 1. delete process.env.FOO is the official documented way to remove env vars 2. Assigning undefined has different behavior from delete for process.env 3. The suggestion can break functionality (e.g., AWS Lambda env reset patterns) The rule still reports the diagnostic for process.env deletions, but no longer offers the unsafe code action. Fixes biomejs#4093

changeset-bot · 2026-04-01T02:58:55Z

🦋 Changeset detected

Latest commit: 6c0e8ad

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

coderabbitai · 2026-04-01T03:08:19Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

The NoDelete lint rule was updated to ignore delete expressions that target Node.js environment variables: when the operand is a static member matching process.env.<name>, the rule returns None and no violation is reported. A helper is_process_env was added to detect the process → env identifier chain. Tests were extended with delete process.env.FOO as a valid case and a changeset was added.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: preventing the noDelete rule from suggesting undefined assignment for process.env properties.
Description check	✅ Passed	The description clearly explains why the change is needed and what was modified, though it mentions updating invalid.jsonc when the summary shows changes only to valid.jsonc.
Linked Issues check	✅ Passed	The PR addresses `#4093` by preventing the unsafe code action. However, based on the PR objectives, the implementation evolved from suppressing the code action to fully suppressing the diagnostic for process.env deletions, which aligns with the issue's expectation that the rule should not suggest the unsafe alternative.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing `#4093`: the rule logic, test fixtures, and changeset entry are all in scope for this fix.

_{✏️ 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.}

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (1)

crates/biome_js_analyze/src/lint/performance/no_delete.rs (1)
135-145: This branch appears to be dead code for the intended use case

Lines 135-145 check if static_obj.object() is a static member expression with member "process". This would match patterns like delete a.process.env.FOO — where process is itself a property of another object.

For the common case delete process.env.FOO, static_obj.object() is the identifier process, so this branch is never entered. You may want to either:

Remove this branch if it's not needed

Add a test case for nested patterns like delete globalThis.process.env.FOO if that's the intended coverage
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/biome_js_analyze/src/lint/performance/no_delete.rs` around lines 135 -
145, The branch that checks static_obj.object().ok().and_then(|o|
o.as_js_static_member_expression()) and then inspects
outer_object.member()/outer_member.as_js_name() for "process" (which would match
nested patterns like delete a.process.env.FOO) appears to be dead for the common
case delete process.env.FOO where static_obj.object() is an identifier; remove
this entire nested block (the if let Some(outer_object) { ... } section that
returns None) to eliminate unreachable code, or if you intended to support
nested targets, instead keep the block and add a unit test exercising delete
globalThis.process.env.FOO (or similar nested pattern) to validate the behavior;
update tests accordingly and ensure the code paths around static_obj.object(),
as_js_static_member_expression(), outer_object.member(), and
outer_member.as_js_name() reflect the chosen approach.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/biome_js_analyze/src/lint/performance/no_delete.rs`:
- Around line 146-156: The current check incorrectly calls
object.as_js_identifier_expression() for member expressions like delete
process.env.FOO, causing the guard to miss when the base is a static member;
change the check to use static_obj.object().as_js_identifier_expression() (i.e.,
call .object() on the static member expression `static_obj` instead of using
`object`) so the subsequent name extraction (ident.name(),
name_ref.as_js_reference_identifier(), and the comparison
name.to_trimmed_text().text() == "process") correctly detects `process.env`
cases and prevents suggesting the fix for delete process.env.FOO.
- Around line 130-161: The current check using
argument.as_js_static_member_expression() cannot detect destructured bindings
like `const { env } = process; delete env.FOO;`; either remove those
destructured test cases and restrict the rule to only match direct static
members (i.e., only rely on argument.as_js_static_member_expression() /
static_obj.member() / name.to_trimmed_text()) or implement semantic binding
resolution: use the rule's context model (ctx.model()) to resolve identifier
origins when object.as_js_identifier_expression() yields an identifier (follow
the identifier's binding to see if it ultimately points to process.env) and only
then treat it as exempt; update the matching logic around
argument/as_js_static_member_expression(), object.as_js_identifier_expression(),
and the name checks to use the model-based binding lookup if you choose the
semantic route.

In `@crates/biome_js_analyze/tests/specs/performance/noDelete/invalid.jsonc`:
- Around line 14-16: The test cases reveal two issues: the rule that flags
"delete process.env.FOO" is checking the wrong identifier so it wrongly offers a
fixer; update the checker in the rule implementation that inspects
MemberExpression nodes (the logic around process.env detection) to ensure it
explicitly verifies the object is the Identifier "process" and the property
chain includes "env" before suggesting a fix; for the destructured patterns
("const { env } = process; delete env.FOO;" and "const { env } =
require('process'); delete env.TEST;") do not attempt a syntactic fixer—either
add semantic resolution to trace that env originates from process or
conservatively disable fix suggestions for Identifier-based deletes (i.e., only
auto-fix when the MemberExpression target is directly process.env), and after
fixing, regenerate the spec snapshots with the provided cargo test command to
confirm diagnostics no longer include fix suggestions.

---

Nitpick comments:
In `@crates/biome_js_analyze/src/lint/performance/no_delete.rs`:
- Around line 135-145: The branch that checks
static_obj.object().ok().and_then(|o| o.as_js_static_member_expression()) and
then inspects outer_object.member()/outer_member.as_js_name() for "process"
(which would match nested patterns like delete a.process.env.FOO) appears to be
dead for the common case delete process.env.FOO where static_obj.object() is an
identifier; remove this entire nested block (the if let Some(outer_object) { ...
} section that returns None) to eliminate unreachable code, or if you intended
to support nested targets, instead keep the block and add a unit test exercising
delete globalThis.process.env.FOO (or similar nested pattern) to validate the
behavior; update tests accordingly and ensure the code paths around
static_obj.object(), as_js_static_member_expression(), outer_object.member(),
and outer_member.as_js_name() reflect the chosen approach.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d7bcd120-db5a-4606-bd2f-575345d007fc

📥 Commits

Reviewing files that changed from the base of the PR and between 716e4e1 and 6167d26.

📒 Files selected for processing (2)

crates/biome_js_analyze/src/lint/performance/no_delete.rs
crates/biome_js_analyze/tests/specs/performance/noDelete/invalid.jsonc

coderabbitai · 2026-04-01T03:08:23Z

+	"delete process.env.FOO;",
+	"const { env } = process; delete env.FOO;",
+	"const { env } = require('process'); delete env.TEST;"


⚠️ Potential issue | 🟠 Major

Test cases may not behave as intended given implementation bugs

Good additions for coverage. However:

Line 14 (delete process.env.FOO) — the rule has a bug (wrong variable checked), so this will incorrectly receive a fix suggestion until the implementation is corrected.

Lines 15-16 (destructured patterns) — these cannot be handled by the current syntactic implementation. The rule would need semantic analysis to track that env originated from process.

Once the implementation issues are resolved, please regenerate snapshots with cargo test -p biome_js_analyze --test spec_tests -- --update to confirm the expected behaviour (diagnostics without fix suggestions). As per coding guidelines, all code changes must include appropriate tests that validate the fix.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/performance/noDelete/invalid.jsonc` around lines 14 - 16, The test cases reveal two issues: the rule that flags "delete process.env.FOO" is checking the wrong identifier so it wrongly offers a fixer; update the checker in the rule implementation that inspects MemberExpression nodes (the logic around process.env detection) to ensure it explicitly verifies the object is the Identifier "process" and the property chain includes "env" before suggesting a fix; for the destructured patterns ("const { env } = process; delete env.FOO;" and "const { env } = require('process'); delete env.TEST;") do not attempt a syntactic fixer—either add semantic resolution to trace that env originates from process or conservatively disable fix suggestions for Identifier-based deletes (i.e., only auto-fix when the MemberExpression target is directly process.env), and after fixing, regenerate the spec snapshots with the provided cargo test command to confirm diagnostics no longer include fix suggestions.

…ess.env

ematipico · 2026-04-01T09:09:16Z

@myx0m0p you must run the tests so you can commit the snapshots.

Without it, we can't merge the PR. Can't you find a solution? If not, let us know. We can help but it will take some time.

As for the rule, we shouldn't trigger the rule at all. The whole point was to comply with Node.js suggestion, where they document to suggest the use of delete. If we keep the diagnostic, but we don't show the action, we still don't tell the user how to fix the issue, which also breaks our rule pillars.

Instead, the rule must not trigger

myx0m0p · 2026-04-03T06:23:49Z

Thanks for the feedback! I've updated the PR:

The rule no longer triggers at all for delete process.env.FOO — the check is now in fn run() instead of fn action(), so the diagnostic is completely skipped for process.env.* properties.
Tests updated — moved delete process.env.FOO from invalid.jsonc to valid.jsonc and accepted the updated snapshot.
Extracted the detection into a separate is_process_env() helper function.

ematipico · 2026-04-03T06:48:29Z

+"@biomejs/biome": "patch"
+---
+
+fix: noDelete should not suggest undefined assignment for process.env


We guidelines for how to write a changeset, can you follow them?

codspeed-hq · 2026-04-03T07:05:15Z

Merging this PR will not alter performance

✅ 58 untouched benchmarks
⏩ 196 skipped benchmarks¹

_{Comparing myx0m0p:fix/issue-4093 (6c0e8ad) with main (716e4e1)}

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. ↩

ematipico · 2026-04-03T08:38:20Z

@myx0m0p CI failed, please address the failures. Make sure to run the jobs locally so that you can catch things early

myx0m0p · 2026-04-03T10:23:39Z

all green, @ematipico thanks for onboarding!

ematipico · 2026-04-03T10:29:36Z

Thank you @myx0m0p, sorry to ask you something again, but would you mind also updating the docs of the rule? That's something really important worth mentioning

myx0m0p · 2026-04-03T15:15:16Z

Pushed follow-up commit 6c0e8ad addressing the latest review feedback:

linked the Node.js process.env docs in the rule rustdoc and next to the exemption comment
removed the remaining allocating name checks in is_process_env

Local verification:

cargo format
cargo lint
cargo test -p biome_js_analyze --test spec_tests specs::performance::no_delete::valid_jsonc -- --exact
cargo test -p biome_js_analyze --test spec_tests specs::performance::no_delete::invalid_jsonc -- --exact

github-actions Bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Apr 1, 2026

coderabbitai Bot reviewed Apr 1, 2026

View reviewed changes

Andre Perepelitsa added 2 commits April 1, 2026 03:20

chore: add changeset for noDelete process.env fix

be6c7e5

fix: remove untestable destructured test cases, keep only direct proc…

e7d82d3

…ess.env

fix: do not trigger noDelete rule for process.env properties

22692f1

ematipico reviewed Apr 3, 2026

View reviewed changes

chore: update changeset to follow contribution guidelines

4316a00

ematipico reviewed Apr 3, 2026

View reviewed changes

Comment thread crates/biome_js_analyze/src/lint/performance/no_delete.rs Outdated

refactor: use Option<bool> in is_process_env per review feedback

bdcc8f6

ematipico reviewed Apr 3, 2026

View reviewed changes

Comment thread crates/biome_js_analyze/src/lint/performance/no_delete.rs Outdated

Apply suggestion from @ematipico

f747d4c

fix: remove needless borrow in is_process_env call

4edd3a4

docs: add process.env example to noDelete rule documentation

a48a1b3

dyc3 requested changes Apr 3, 2026

View reviewed changes

Comment thread crates/biome_js_analyze/src/lint/performance/no_delete.rs Outdated

Comment thread crates/biome_js_analyze/src/lint/performance/no_delete.rs Outdated

Comment thread crates/biome_js_analyze/src/lint/performance/no_delete.rs Outdated

fix(lint): refine noDelete process.env docs

6c0e8ad

dyc3 approved these changes Apr 3, 2026

View reviewed changes

ematipico approved these changes Apr 4, 2026

View reviewed changes

ematipico merged commit 5b16d18 into biomejs:main Apr 4, 2026
18 checks passed

github-actions Bot mentioned this pull request Apr 4, 2026

ci: release #9733

Merged

myx0m0p deleted the fix/issue-4093 branch April 4, 2026 14:01

Uh oh!

Conversation

myx0m0p commented Apr 1, 2026

Summary

Changes

Testing

Uh oh!

changeset-bot Bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

coderabbitai Bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ematipico commented Apr 1, 2026

Uh oh!

myx0m0p commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ematipico Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

codspeed-hq Bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Uh oh!

Uh oh!

ematipico commented Apr 3, 2026

Uh oh!

myx0m0p commented Apr 3, 2026

Uh oh!

ematipico commented Apr 3, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

myx0m0p commented Apr 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot Bot commented Apr 1, 2026 •

edited

Loading

coderabbitai Bot commented Apr 1, 2026 •

edited

Loading

myx0m0p commented Apr 3, 2026 •

edited

Loading

codspeed-hq Bot commented Apr 3, 2026 •

edited

Loading