Open
Conversation
🦋 Changeset detectedLatest commit: 8f36d38 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
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 |
ccb0e3c to
00f25c4
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a nursery lint rule Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
00f25c4 to
2d32061
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_restricted_properties.rs (1)
265-277: Minor: Redundantomit_parentheses()call.Line 270 calls
omit_parentheses()on the object, butidentifier_object_name(line 416) calls it again internally. The first call is redundant sinceidentifier_object_namehandles it.Not blocking – the duplication is harmless and keeps
identifier_object_nameself-contained for other call sites (lines 382, 397) that don't pre-strip parentheses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_restricted_properties.rs` around lines 265 - 277, The call to omit_parentheses() in inspect_member_expression is redundant because identifier_object_name already calls omit_parentheses internally; remove the extra omit_parentheses() invocation so inspect_member_expression passes the raw object (the variable named object) into identifier_object_name(node.object().ok()?) and keep matching via match_restriction with identifier_object_name, property_name.text(), and property_name.range() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_restricted_properties.rs`:
- Around line 265-277: The call to omit_parentheses() in
inspect_member_expression is redundant because identifier_object_name already
calls omit_parentheses internally; remove the extra omit_parentheses()
invocation so inspect_member_expression passes the raw object (the variable
named object) into identifier_object_name(node.object().ok()?) and keep matching
via match_restriction with identifier_object_name, property_name.text(), and
property_name.range() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93fccb9e-ea59-420f-aa1c-1840a0abe443
⛔ Files ignored due to path filters (12)
.opencode/package-lock.jsonis excluded by!**/package-lock.jsonand included by**Cargo.lockis excluded by!**/*.lockand included by**crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_cli/tests/snapshots/main_commands_migrate_eslint/migrate_no_restricted_properties_with_options.snapis excluded by!**/*.snapand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalidConfig.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/valid.js.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (14)
.changeset/thick-coins-hide.mdcrates/biome_cli/src/execute/migrate/eslint_eslint.rscrates/biome_cli/src/execute/migrate/eslint_to_biome.rscrates/biome_cli/tests/commands/migrate_eslint.rscrates/biome_js_analyze/src/lint/nursery/no_restricted_properties.rscrates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalid.jscrates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalid.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalidConfig.jscrates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalidConfig.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/valid.jscrates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/valid.options.jsoncrates/biome_rule_options/Cargo.tomlcrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_restricted_properties.rs
✅ Files skipped from review due to trivial changes (9)
- crates/biome_rule_options/src/lib.rs
- crates/biome_rule_options/Cargo.toml
- .changeset/thick-coins-hide.md
- crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/valid.js
- crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalidConfig.js
- crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalid.options.json
- crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalidConfig.options.json
- crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/valid.options.json
- crates/biome_js_analyze/tests/specs/nursery/noRestrictedProperties/invalid.js
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_cli/src/execute/migrate/eslint_eslint.rs
ematipico
reviewed
Apr 17, 2026
Member
ematipico
left a comment
There was a problem hiding this comment.
Code looks fine, but docs need some rework
2d32061 to
67e873c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_cli/tests/commands/migrate_eslint.rs (1)
407-458: Nice test — consider adding the severity-only sibling case.This covers populated entries well; a tiny
"error"-only case would also lock down theentries: Nonebranch.Suggested companion test
+#[test] +fn migrate_no_restricted_properties_no_options() { + let biomejson = r#"{}"#; + let eslintrc = r#"{ "rules": { "no-restricted-properties": "error" } }"#; + + let fs = MemoryFileSystem::default(); + fs.insert(Utf8Path::new("biome.json").into(), biomejson.as_bytes()); + fs.insert(Utf8Path::new(".eslintrc.json").into(), eslintrc.as_bytes()); + + let mut console = BufferConsole::default(); + let (fs, result) = run_cli( + fs, + &mut console, + Args::from( + ["migrate", "eslint", "--include-nursery", "--write"].as_slice(), + ), + ); + + assert!(result.is_ok(), "run_cli returned {result:?}"); + assert_cli_snapshot(SnapshotPayload::new( + module_path!(), + "migrate_no_restricted_properties_no_options", + fs, + console, + result, + )); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/tests/commands/migrate_eslint.rs` around lines 407 - 458, Add a companion test to cover the severity-only branch: create a new test (e.g., migrate_no_restricted_properties_severity_only) that supplies a .eslintrc.json where "no-restricted-properties" is just ["error"] (no entries object/array), mirror the same MemoryFileSystem, BufferConsole and run_cli invocation used in migrate_no_restricted_properties_with_options (Args::from with "migrate","eslint","--include-inspired","--include-nursery","--write"), assert run_cli is Ok and call assert_cli_snapshot with the new test name to lock down the entries: None branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_cli/tests/commands/migrate_eslint.rs`:
- Around line 407-458: Add a companion test to cover the severity-only branch:
create a new test (e.g., migrate_no_restricted_properties_severity_only) that
supplies a .eslintrc.json where "no-restricted-properties" is just ["error"] (no
entries object/array), mirror the same MemoryFileSystem, BufferConsole and
run_cli invocation used in migrate_no_restricted_properties_with_options
(Args::from with
"migrate","eslint","--include-inspired","--include-nursery","--write"), assert
run_cli is Ok and call assert_cli_snapshot with the new test name to lock down
the entries: None branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54eff383-6850-4d7d-8e9b-e3adae95b249
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_cli/tests/snapshots/main_commands_migrate_eslint/migrate_no_restricted_properties_with_options.snapis excluded by!**/*.snapand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (4)
.changeset/thick-coins-hide.mdcrates/biome_cli/src/execute/migrate/eslint_eslint.rscrates/biome_cli/src/execute/migrate/eslint_to_biome.rscrates/biome_cli/tests/commands/migrate_eslint.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/thick-coins-hide.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_cli/src/execute/migrate/eslint_eslint.rs
67e873c to
8f36d38
Compare
Contributor
Author
|
Ok, I've rewritten the options docs by hand. I know that we usually put examples before options, but for this rule specifically: should we have the options first? I think it could make it flow better. We introduce the user to the options and how they work, then provide examples to demonstrate. |
ematipico
requested changes
Apr 18, 2026
Member
There was a problem hiding this comment.
I left some suggestions for the docs. As for rule, it's a rule specially tailored for JavaScript, and that's confirmed by the way the options are treated. Remember that options are shared across languages.
When rules are designed for a language or domain, they should carry their language in their name. For example useGraphqlNamingConvention. This rule should be named noJsRestrictedProperties. I don't see this rule being ported to other languages, because of the way identifiers are treated.
| /// } | ||
| /// ``` | ||
| /// | ||
| /// This restricts the `ensure` property on the `require` object, so this is reported: |
Member
There was a problem hiding this comment.
Suggested change
| /// This restricts the `ensure` property on the `require` object, so this is reported: | |
| /// In this example, the rule reports the access of the `ensure` property on the `require` object, and it emits the message "...": |
Complete the phrase
| /// } | ||
| /// ``` | ||
| /// | ||
| /// This restricts the `__defineGetter__` property on all objects, so this is reported: |
Member
There was a problem hiding this comment.
Suggested change
| /// This restricts the `__defineGetter__` property on all objects, so this is reported: | |
| /// In this example, the rule reports any access to the property `__defineGetter__`, expect for `Object` object, and it emits the message "...": |
Complete the phrase
| /// foo.__defineGetter__ | ||
| /// ``` | ||
| /// | ||
| /// Except for `Object`, which is allowed by the `allowObjects` allowlist: |
Member
There was a problem hiding this comment.
Suggested change
| /// Except for `Object`, which is allowed by the `allowObjects` allowlist: |
Comment on lines
+107
to
+111
| /// An array of restricted object/property combinations. Depending on the provided options, each entry can: | ||
| /// - Restrict a specific property on a specific object. | ||
| /// - Restrict all properties on a specific object except for an allowlist of properties. | ||
| /// - Restrict a specific property everywhere except for an allowlist of objects. | ||
| /// - Provide a custom message to include in the diagnostic when the restriction is violated. |
Member
There was a problem hiding this comment.
Suggested change
| /// An array of restricted object/property combinations. Depending on the provided options, each entry can: | |
| /// - Restrict a specific property on a specific object. | |
| /// - Restrict all properties on a specific object except for an allowlist of properties. | |
| /// - Restrict a specific property everywhere except for an allowlist of objects. | |
| /// - Provide a custom message to include in the diagnostic when the restriction is violated. | |
| /// An array of restricted object/property combinations. |
I removed the list because we're repeating the examples and the description of the properties below.
| /// } | ||
| /// ``` | ||
| /// | ||
| /// This restricts all properties on the `arguments` object, so this is reported: |
Member
There was a problem hiding this comment.
Suggested change
| /// This restricts all properties on the `arguments` object, so this is reported: | |
| /// In the following example, when the rule encounters the object `arguments`, it reports all properties except for `length`with the message "...": |
See if you like this. If so, complete the phrase
| /// arguments.callee | ||
| /// ``` | ||
| /// | ||
| /// Except for `length`, which is allowed by the `allowProperties` allowlist: |
Member
There was a problem hiding this comment.
Suggested change
| /// Except for `length`, which is allowed by the `allowProperties` allowlist: |
Comment on lines
+121
to
+127
| /// ### `entries[].allowObjects` | ||
| /// | ||
| /// When restricting a property, an optional allowlist of objects that are exempt from the restriction. Only applicable when `property` is provided and `object` is not provided. | ||
| /// | ||
| /// ### `entries[].allowProperties` | ||
| /// | ||
| /// When restricting an object, an optional allowlist of properties that are exempt from the restriction. Only applicable when `object` is provided and `property` is not provided. |
Member
There was a problem hiding this comment.
Here's a legitimate question that the docs should explain.
What if the configuration provides both? From what I read, the allow lists work only in certain conditions. If both property and object are provided, what happens?
Contributor
Author
There was a problem hiding this comment.
That's exactly what I was trying to address with the bullet points up here that you want removed: #9806 (comment)
Member
There was a problem hiding this comment.
Oh, I didn't understand that from the bullet list :(
Comment on lines
+49
to
+98
| <Emphasis>"'entries[]'"</Emphasis> | ||
| " must define at least one of 'object' or 'property'." | ||
| }) | ||
| .with_range(range), | ||
| ); | ||
| is_valid = false; | ||
| } | ||
|
|
||
| if entry | ||
| .object | ||
| .as_deref() | ||
| .is_some_and(|name| !is_js_ident(name)) | ||
| { | ||
| ctx.report( | ||
| DeserializationDiagnostic::new(markup! { | ||
| <Emphasis>"'entries[].object'"</Emphasis> | ||
| " must be a valid JavaScript identifier." | ||
| }) | ||
| .with_range(range), | ||
| ); | ||
| is_valid = false; | ||
| } | ||
|
|
||
| if entry.object.is_some() && !entry.allow_objects.is_empty() { | ||
| ctx.report( | ||
| DeserializationDiagnostic::new(markup! { | ||
| <Emphasis>"'entries[].allowObjects'"</Emphasis> | ||
| " cannot be used together with 'entries[].object'." | ||
| }) | ||
| .with_range(range), | ||
| ); | ||
| is_valid = false; | ||
| } | ||
|
|
||
| if entry.allow_objects.iter().any(|name| !is_js_ident(name)) { | ||
| ctx.report( | ||
| DeserializationDiagnostic::new(markup! { | ||
| <Emphasis>"'entries[].allowObjects'"</Emphasis> | ||
| " only accepts valid JavaScript identifiers." | ||
| }) | ||
| .with_range(range), | ||
| ); | ||
| is_valid = false; | ||
| } | ||
|
|
||
| if entry.property.is_some() && !entry.allow_properties.is_empty() { | ||
| ctx.report( | ||
| DeserializationDiagnostic::new(markup! { | ||
| <Emphasis>"'entries[].allowProperties'"</Emphasis> | ||
| " cannot be used together with 'entries[].property'." |
Member
There was a problem hiding this comment.
Generally, messages and errors should be actionable. Where possible, we should tell the user how to solve the error, and I see some opportunity here
| ``` | ||
| invalidConfig.options:8:22 deserialize ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ | ||
|
|
||
| × 'entries[].object' must be a valid JavaScript identifier. |
Member
There was a problem hiding this comment.
Here the error is triggered by the value passed to object, but the message doesn't show it. That's a mistake. It should show "Object.prototype" instead.
Also would flip the message and say "X isn't a valid identifier". I removed JavaScript.
| #[derive(Default, Clone, Debug, Deserialize, Deserializable, Eq, PartialEq, Serialize)] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields, default)] | ||
| #[deserializable(with_validator)] |
Member
There was a problem hiding this comment.
Using the validator here degrades the UX of our diagnostics. If you see the snapshots, the whole options is flagged as error (span), and the user can't understand which entries[] has a real error.
Based on the errors, I believe it's safe to move the validator in the entry type
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds
noRestrictedPropertieswhich is a port of https://eslint.org/docs/latest/rules/no-restricted-propertiesThe main reason to add this rule is that it will be a faster alternative than users writing grit plugins for similar use cases.
generated by gpt 5.4, heavily steered to get it to stop allocating strings
Test Plan
snapshots
Docs