fix(linter): Display correct plugin prefix for vitest rules and support vitest-specific behavior#17886
fix(linter): Display correct plugin prefix for vitest rules and support vitest-specific behavior#17886pmmm114 wants to merge 4 commits intooxc-project:mainfrom
Conversation
…guration Add `ConfiguredNamespace` type alias (`Option<&'static str>`) to track the original namespace under which a rule was configured. This is needed because vitest rules are implemented by jest rules, but we need to preserve the original namespace for correct error prefix display. - Add `ConfiguredNamespace` type in config/rules.rs - Add `to_static_str()` helper function for plugin name conversion - Update `RuleSet` type to store `(AllowWarnDeny, ConfiguredNamespace)` - Capture original namespace in `override_rules()` when plugin name transforms - Export `ConfiguredNamespace` from config/mod.rs
…lder and store Update ConfigStoreBuilder and ConfigStore to carry namespace information from configuration parsing to runtime rule resolution. - Change `ConfigStoreBuilder.rules` HashMap value type to include namespace - Add `with_rule_and_namespace()` test helper method - Update `ResolvedLinterState.rules` to 3-tuple `(RuleEnum, AllowWarnDeny, ConfiguredNamespace)` - Update `Config::new()`, `apply_overrides()`, and all related methods - Update all test assertions to use 3-tuple pattern matching
… integration points Enable rules to determine if they were configured under a specific namespace (e.g., vitest vs jest) by passing ConfiguredNamespace through the context. - Add `configured_namespace` field to `LintContext` - Add `is_vitest_context()` method to check if rule was configured under vitest - Update `ContextHost::spawn()` signature to accept namespace parameter - Update 3-tuple pattern matching in lib.rs, tsgolint.rs, lint.rs - Add `with_configured_namespace()` to Tester for testing namespace-specific behavior
There was a problem hiding this comment.
Pull request overview
This PR fixes the display of plugin prefixes for vitest rules and adds support for vitest-specific behavior. When a user configures a rule under the vitest namespace (e.g., vitest/valid-describe-callback), the linter now correctly displays eslint-plugin-vitest instead of eslint-plugin-jest in diagnostic messages, even though the rule implementation is shared with jest. Additionally, the PR adds support for vitest-specific features: async describe callbacks and the 3-argument syntax describe('name', { timeout }, callback).
Changes:
- Extended the rules data structure from
(RuleEnum, AllowWarnDeny)to(RuleEnum, AllowWarnDeny, ConfiguredNamespace)to track the original namespace - Added
is_vitest_context()method toLintContextto allow rules to detect vitest configuration - Updated
valid_describe_callbackrule to allow async describe callbacks when configured under vitest namespace - Added support for vitest's 3-argument describe syntax with options object
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_linter/src/config/rules.rs | Added ConfiguredNamespace type and to_static_str function; updated override_rules to track configured namespace |
| crates/oxc_linter/src/config/mod.rs | Exported ConfiguredNamespace type |
| crates/oxc_linter/src/config/config_store.rs | Updated all rule tuple types to include ConfiguredNamespace |
| crates/oxc_linter/src/config/config_builder.rs | Updated builder methods and test utilities to handle 3-tuple format |
| crates/oxc_linter/src/context/mod.rs | Added configured_namespace field and is_vitest_context() method |
| crates/oxc_linter/src/context/host.rs | Updated spawn method to accept and use configured_namespace |
| crates/oxc_linter/src/rules/jest/valid_describe_callback.rs | Added vitest-specific behavior: async describe support and 3-argument syntax; split tests into jest and vitest variants |
| crates/oxc_linter/src/tester.rs | Added configured_namespace field and with_configured_namespace method |
| crates/oxc_linter/src/tsgolint.rs | Updated tuple destructuring patterns |
| crates/oxc_linter/src/lib.rs | Updated tuple destructuring and context spawning |
| apps/oxlint/src/lint.rs | Updated tuple destructuring |
| crates/oxc_linter/src/snapshots/*.snap | New snapshot files showing correct plugin prefix for jest and vitest contexts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ---------- | ||
|
|
||
| x ]8;;https://oxc.rs/docs/guide/usage/linter/rules/eslint/no-magic-numbers.html\eslint(no-magic-numbers)]8;;\: No magic number: 0.19 | ||
| x ]8;;https://oxc.rs/docs/guide/usage/linter/rules/typescript/no-magic-numbers.html\typescript-eslint(no-magic-numbers)]8;;\: No magic number: 0.19 |
There was a problem hiding this comment.
I'm confused why this change occurred? The no_magic_numbers.rs rule is definitely in the eslint namespace and this link will not work.
There was a problem hiding this comment.
Ah it's because of the TYPESCRIPT_COMPATIBLE_ESLINT_RULES array
| // Vitest supports describe('name', options, callback) syntax where the second argument | ||
| // is an options object (e.g., { timeout: 10_000 }) and the third is the callback. | ||
| // In this case, we need to validate the callback which is at index 2. | ||
| let callback_index = if ctx.is_vitest_context() | ||
| && arg_len == 3 | ||
| && matches!(&call_expr.arguments[1], Argument::ObjectExpression(_)) | ||
| { | ||
| 2 // Third argument is the callback in vitest options syntax | ||
| } else { | ||
| 1 // Second argument is the callback (standard jest/vitest syntax) | ||
| }; | ||
|
|
||
| // Ensure the callback index is valid | ||
| if callback_index >= arg_len { | ||
| diagnostic(ctx, call_expr.arguments[1].span(), Message::SecondArgumentMustBeFunction); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This change should definitely be split into its own PR, please :)
There was a problem hiding this comment.
Thank you for the feedback.
I've removed the "Supports vitest's 3-argument syntax" feature from the current PR. Once this gets merged, I'll submit it as a follow-up PR :)
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
…e in vitest context When `vitest/valid-describe-callback` is configured, the error message now correctly shows `eslint-plugin-vitest` prefix instead of `eslint-plugin-jest`. Additionally, async describe callbacks are now allowed when configured under vitest namespace, matching vitest's behavior. Changes: - Add `ConfiguredNamespace` type to track jest/vitest namespace configuration - Skip async describe callback check when `is_vitest_context()` returns true - Split tests into `test_jest()`, `test_jest_async_describe()`, `test_vitest()` - Update snapshots to reflect correct vitest prefix Closes oxc-project#9060
311858c to
3c6f5fe
Compare
|
I've addressed your feedback. :) |
|
If you happen to have time, I'd love to get your feedback on the direction of this PR and whether it aligns with the intended design of the project. I've outlined some discussion points in the description. Additionally, I'd appreciate any feedback on potential issues or improvements in the implementation itself—things I might have missed. Thanks for your time! |
What This PR Does
closes #9060
eslint-plugin-vitest(...)instead ofeslint-plugin-jest(...)when rules are configured under vitest namespacedescribe('name', { timeout }, callback)Approach
Added
ConfiguredNamespace = Option<&'static str>to track the original namespace. Extended the rules tuple from(RuleEnum, AllowWarnDeny)to(RuleEnum, AllowWarnDeny, ConfiguredNamespace).Used a 3-tuple instead of a struct, following #11051's direction.
Disclosure
Questions for Reviewers
Tuple vs Struct: With 3 fields now, should we introduce a struct instead of the 3-tuple?
Auto-detection: Currently auto-detects vitest context when vitest plugin is enabled and a jest rule runs. Is this appropriate, or require explicit
vitest/rule-name?3. Scope: Onlyvalid-describe-callbackusesis_vitest_context(). Should other shared rules be updated in this PR or follow-up?