Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(biome_css_analyzer): implement noInvalidPositionAtImportRule #2717

Merged

Conversation

t-shiratori
Copy link
Contributor

@t-shiratori t-shiratori commented May 5, 2024

Summary

close #2709

Implement no-invalid-position-at-import-rule)

Please note that the following is not yet addressed:

  • Implementation of options
  • Handling extended CSS language cases such as sass and less
  • Custom function

Test Plan

added tests and snapshots

@github-actions github-actions bot added A-Project Area: project L-CSS Language: CSS A-Diagnostic Area: diagnostocis labels May 5, 2024
@t-shiratori t-shiratori marked this pull request as draft May 5, 2024 01:19
Merge branch 'main' into feature/noInvalidPositionAtImportRule and
 `just gen-lint`
Copy link

codspeed-hq bot commented May 5, 2024

CodSpeed Performance Report

Merging #2717 will not alter performance

Comparing t-shiratori:feature/noInvalidPositionAtImportRule (e690c55) with main (5088667)

Summary

✅ 97 untouched benchmarks

@t-shiratori t-shiratori marked this pull request as ready for review May 5, 2024 02:43
.note(markup! {
"Consider moving import position."
}).note(markup! {
"Any @import rules must precede all other valid at-rules and style rules in a stylesheet (ignoring @charset and @layer), or else the @import rule is invalid."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use <Emphasis></Emphasis> to highlight @import?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: 7495e81

},
)
.note(markup! {
"Consider moving import position."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this at the end? Reminder to check our rule pillars https://biomejs.dev/linter/#rule-pillars

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: c6f45ab

continue;
}

let import_rule = any_css_at_rule.as_css_import_at_rule().cloned();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't clone here. Clone when you actually need it, which is when you call return

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: 9ca1793

Comment on lines +19 to +20
> 2 │ @import 'foo.css';
│ ^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you noticed, but the diagnostic isn't correct because it doesn't highlight the @.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review.
Sorry, I didn't know how to do it 🙇‍♂️
Is there a sample somewhere?

Comment on lines 39 to 40
type State = CssImportAtRule;
type Signals = Option<Self::State>;
Copy link
Member

@ematipico ematipico May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be a bit better. Instead of highlighting only the first incorrect at-rule, we should highlight ALL incorrect at-rules. For example:

a {}
@import "foo";
@import "bar";
@import "foo";
@import "bar";

Here we should flag all import all together.

This means we should return a Vec<Self::State>. And State should be a TextRange, we don't need the whole node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't know how to do it 🙇‍♂️
RuleDiagnostic only accepts a single TextRange.
How should I handle vector format state in RuleDiagnostic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The infrastructure does that for you, you don't need to do much more.

You just need to return a vector from the run function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your advice!
I am trying to fix it locally as below.
We add invalid_import_list and return it at the end.
But I get a TypeError in the state of RuleDiagnostic::new().

crates/biome_css_analyze/src/lint/nursery/no_invalid_position_at_import_rule.rs

impl Rule for NoInvalidPositionAtImportRule {
    type Query = Ast<CssRuleList>;
    type State = Vec<TextRange>;   //  ← update
    type Signals = Option<Self::State>;
    type Options = ();

    fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
        let node = ctx.query();
        let mut is_invalid_position = false;
        let mut invalid_import_list = Vec::new();  //  ← update

        for rule in node {
            let any_css_at_rule = match rule {
                AnyCssRule::CssAtRule(item) => item.rule().ok(),
                _ => None,
            };

            if let Some(any_css_at_rule) = any_css_at_rule {
                // Ignore @charset, @layer
                if any_css_at_rule.as_css_charset_at_rule().is_some() {
                    continue;
                }
                if any_css_at_rule.as_css_layer_at_rule().is_some() {
                    continue;
                }

                let import_rule = any_css_at_rule.as_css_import_at_rule();
                if let Some(import_rule) = import_rule {
                    if is_invalid_position {
                        invalid_import_list.push(import_rule.range());  //  ← update
                    }
                } else {
                    is_invalid_position = true;
                }
            } else {
                is_invalid_position = true;
            }
        }

        if invalid_import_list.len() > 0 {
            return Some(invalid_import_list);  //  ← update
        }

        None
    }

    fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
        Some(
            RuleDiagnostic::new(
                rule_category!(),
                state,  //  ← TypeError 
                markup! {
                    "This "<Emphasis>"@import"</Emphasis>" is in the wrong position."
                },
            )
            .note(markup! {
                "Any "<Emphasis>"@import"</Emphasis>" rules must precede all other valid at-rules and style rules in a stylesheet (ignoring @charset and @layer), or else the "<Emphasis>"@import"</Emphasis>" rule is invalid."
            }).note(markup! {
                "Consider moving import position."
            })
        )
    }
}

Copy link
Member

@ematipico ematipico May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type Signals must return a Vec<Self::State> instead of Option<Self::State>.

Basically, you must always return a vector.

State must stay a TextRange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico

Thank you for your advice!
6f752ed

Please also check this 🙇‍♂️
#2717 (comment)

Comment on lines 35 to 37
pub struct RuleState {
span: TextRange,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have struct with one field, the struct is superfluous. Just TextRange as state is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: f536251

- Merge branch 'main' into feature/noInvalidPositionAtImportRule
- fix conflicts (rules.rs)
@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser A-Tooling Area: internal tools A-LSP Area: language server protocol L-JavaScript Language: JavaScript and super languages A-Changelog Area: changelog labels May 7, 2024
@github-actions github-actions bot removed A-Linter Area: linter A-Parser Area: parser A-Tooling Area: internal tools A-LSP Area: language server protocol A-Changelog Area: changelog labels May 7, 2024
Comment on lines 1 to 2
@charset 'utf-8';
@import 'foo.css';
Copy link

@Mouvedia Mouvedia May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose @charset "utf-8"; @import 'foo.css'; passes as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review.
fix: eb6210b

Comment on lines +1 to +2
/* some comment */
@import 'foo.css';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Maybe add a test case for

@charset "utf-8";
/* some comment */
@import 'foo.css';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: aa879c1

---
# Input
```css
@CHARSET 'utf-8';
Copy link

@Mouvedia Mouvedia May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

major

This is invalid.
i.e. @charset string must be double-quoted.
It's not really a at-rule per se.

If you want to be pedantic, Firefox used to support the non-standard @charset utf-8; almost 20 years ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: 27b6ab5

---
# Input
```css
@charset 'utf-8';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@charset 'utf-8';
@charset "utf-8";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: eb6210b

@@ -0,0 +1,3 @@
@CHARSET 'utf-8';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: 27b6ab5

@t-shiratori t-shiratori requested a review from ematipico May 9, 2024 11:40
@ematipico ematipico merged commit 4213527 into biomejs:main May 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Diagnostic Area: diagnostocis A-Project Area: project L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Implement no-invalid-position-at-import-rule
3 participants