fix(linter): Fix empty-tags rule logic.#16873
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the empty-tags rule to correctly ignore the ignorePrivate setting, as documented in the JSDoc settings. The rule was incorrectly filtering JSDoc comments based on the ignorePrivate setting, which should not apply to this rule according to the documented behavior.
Key Changes:
- Removed the
should_ignore_as_privateutility import and its usage - Updated iteration logic to process all JSDoc comments without filtering
- Added explanatory comments documenting why
ignorePrivatedoesn't apply - Fixed test configuration structure to use proper
settingswrapper
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
The core logic change (stop honoring ignorePrivate) matches the PR intent, but there’s a small performance footgun in is_empty_tag_kind due to repeated to_string() allocations. The updated test config structure is good, but the test suite should include an explicit regression test proving that ignorePrivate: true does not suppress empty-tags diagnostics on @private blocks. Addressing these will make the rule both faster and less likely to regress.
Additional notes (1)
- Performance |
crates/oxc_linter/src/rules/jsdoc/empty_tags.rs:108-108
The current implementation allocates on every tag check viatag_name.to_string()inside the hot path:self.0.tags.contains(&tag_name.to_string()). For large files / many tags this can be unnecessarily expensive. Consider storing configured tags as a set (e.g.,FxHashSet<String>) or normalizing toVec<String>but comparing without allocating (e.g., iterate and compare&str).
Summary of changes
What changed
- Removed the private-filtering logic from
jsdoc/empty-tagsby droppingutils::should_ignore_as_privateand iterating over all JSDoc blocks. - Added inline documentation clarifying that the
ignorePrivateJSDoc setting does not apply to this rule. - Updated the related test configuration shape to nest
ignorePrivateundersettings.jsdoc(i.e.,{"settings": {"jsdoc": {"ignorePrivate": true}}}) instead of a top-leveljsdockey.
Key diff points
crates/oxc_linter/src/rules/jsdoc/empty_tags.rs: removedfilter(|jsdoc| !should_ignore_as_private(...))and its settings dependency; updated test config JSON.
CodSpeed Performance ReportMerging #16873 will not alter performanceComparing Summary
Footnotes
|
|
Oh wait ignore me, I though copilot made this PR 🙂 |
Merge activity
|
This corrects the implementation of the empty-tags rule to not care about the `ignorePrivate` setting. Fixes #16871. The doc comment for this setting actually explicitly says it should not be considered: https://github.com/oxc-project/oxc/blob/230e34c6eb36c4265fa8ae421375884182fd6926/crates/oxc_linter/src/config/settings/jsdoc.rs#L12-L14 This was missed when porting the tests because the settings shape was incorrect and silently failed. I only noticed it while attempting to enforce a stricter config schema.
02fa857 to
bb86e0e
Compare
… file (#16874) This adds a `$schema` field to the JSON Schema, but otherwise disallows any fields not explicitly defined in the Oxlintrc struct. This ensures that we can warn the user if they have invalid config fields: <img width="695" height="417" alt="Screenshot 2025-12-14 at 11 22 02 PM" src="https://github.com/user-attachments/assets/8dc2252e-58ad-4bb5-a84e-cd468ad35941" /> The improved enforcement ensures the correct usage of the configuration file, and this change found a bug in another rule (see #16873).
… file (oxc-project#16874) This adds a `$schema` field to the JSON Schema, but otherwise disallows any fields not explicitly defined in the Oxlintrc struct. This ensures that we can warn the user if they have invalid config fields: <img width="695" height="417" alt="Screenshot 2025-12-14 at 11 22 02 PM" src="https://github.com/user-attachments/assets/8dc2252e-58ad-4bb5-a84e-cd468ad35941" /> The improved enforcement ensures the correct usage of the configuration file, and this change found a bug in another rule (see oxc-project#16873).
This corrects the implementation of the empty-tags rule to not care about the
ignorePrivatesetting.Fixes #16871.
The doc comment for this setting actually explicitly says it should not be considered:
oxc/crates/oxc_linter/src/config/settings/jsdoc.rs
Lines 12 to 14 in 230e34c
This was missed when porting the tests because the settings shape was incorrect and silently failed. I only noticed it while attempting to enforce a stricter config schema.