fix(css_analyze): ignore at-rules supporting descriptors#8819
fix(css_analyze): ignore at-rules supporting descriptors#8819Netail merged 3 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 7dfd40f The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
Merging this PR will degrade performance by 15.64%
Performance Changes
Comparing Footnotes
|
WalkthroughBroads the noUnknownProperty lint to detect any AnyCssAtRule ancestor and, unless that ancestor is one of the explicitly enumerated descriptor-supporting at-rules, treat the context as supporting descriptors and skip reporting unknown properties. Adds a new public node union Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_css_analyze/src/lint/correctness/no_unknown_property.rs`:
- Around line 82-101: The current ancestor check for
is_at_rule_supporting_descriptors uses an exclusion-style test (AnyCssAtRule
with a list of denied kinds) and thus incorrectly treats most at-rules as
descriptor-supporting; change it to an inclusive allow-list that returns true
only when an ancestor.kind() is one of the known descriptor-supporting at-rules
(e.g. match AnyCssAtRule::can_cast(ancestor.kind()) and then explicitly check
for the allowed kinds such as the enum variants representing `@font-face`,
`@font-feature-values`, `@property`, `@counter-style`, and `@viewport`), otherwise
return false; update the is_at_rule_supporting_descriptors computation (and any
references to it) so validation is skipped only for those allowed at-rules.
crates/biome_css_analyze/src/lint/correctness/no_unknown_property.rs
Outdated
Show resolved
Hide resolved
d7fe7c9 to
6a8ba12
Compare
ematipico
left a comment
There was a problem hiding this comment.
Should address the parsing diagnostics
| let is_at_rule_supporting_descriptors = node.syntax().ancestors().any(|ancestor| { | ||
| if AnyCssAtRule::can_cast(ancestor.kind()) | ||
| && !(TwApplyAtRule::can_cast(ancestor.kind()) | ||
| || CssContainerAtRule::can_cast(ancestor.kind()) | ||
| || CssLayerAtRule::can_cast(ancestor.kind()) | ||
| || CssMediaAtRule::can_cast(ancestor.kind()) | ||
| || CssScopeAtRule::can_cast(ancestor.kind()) | ||
| || CssStartingStyleAtRule::can_cast(ancestor.kind()) | ||
| || CssSupportsAtRule::can_cast(ancestor.kind())) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| false |
There was a problem hiding this comment.
Two things:
- why did we remove skip()?
- in these cases, it's best to create a new node via
declare_node_union. The code becomes more readable
|
|
||
| ``` | ||
|
|
||
| _Note: The parser emitted 2 diagnostics which are not shown here._ |
There was a problem hiding this comment.
This seems important. There are parsing errors, and I believe you don't want to test broken syntax
There was a problem hiding this comment.
Oops, didn't notice. Some syntax that would be valid with scss 😅
ematipico
left a comment
There was a problem hiding this comment.
Looks good. A couple of things to address
| } | ||
|
|
||
| declare_node_union! { | ||
| pub DescriptorSupportingAtRules = TwApplyAtRule | CssContainerAtRule |
There was a problem hiding this comment.
| pub DescriptorSupportingAtRules = TwApplyAtRule | CssContainerAtRule | |
| pub AnyDescriptorSupportingAtRules = TwApplyAtRule | CssContainerAtRule |
As per internal coding guidelines, all unions must start with Any*
.changeset/tough-pans-greet.md
Outdated
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fixed [#6567](https://github.com/biomejs/biome/issues/6567). Ignore unknown properties in at-rules which support descriptors. |
There was a problem hiding this comment.
You might want to mention the rule. Without clicking the issue, there's little context, and it gets difficult to understand what's been fixed and where
Summary
Closes #6567, ignore unknown properties if located in at-rules supporting descriptors
Test Plan
Unit test
Docs