feat(lint/js): add noRestrictedProperties#9806
Conversation
🦋 Changeset detectedLatest commit: bf82af3 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
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ESLint Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
00f25c4 to
2d32061
Compare
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
left a comment
There was a problem hiding this comment.
Code looks fine, but docs need some rework
2d32061 to
67e873c
Compare
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
|
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. |
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.
| /// 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. |
There was a problem hiding this comment.
| /// 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.
| /// ### `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. |
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?
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)
There was a problem hiding this comment.
Oh, I didn't understand that from the bullet list :(
|
Friendly bump @dyc3 |
8f36d38 to
da0e956
Compare
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