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

Add no-invalid-position-var-function #6859

Open
silverwind opened this issue May 23, 2023 · 22 comments
Open

Add no-invalid-position-var-function #6859

silverwind opened this issue May 23, 2023 · 22 comments
Labels
status: ask to implement ask before implementing as may no longer be relevant type: new rule an entirely new rule

Comments

@silverwind
Copy link

silverwind commented May 23, 2023

What is the problem you're trying to solve?

Using the var() function inside a @ rule block is a CSS syntax error that should be detectable:

@font-face {
  font-family: foo;
  font-weight: var(--font-weight-normal); /* Syntax Error */
}

Firefox will raise a Unknown descriptor ‘var(’ in @font-face rule. Skipped to next declaration..

What solution would you like to see?

A rule that detects such cases. I dug through the CSS specs and found:

Block at-rules will typically contain a collection of (generic or at-rule–specific) at-rules, qualified rules, and/or descriptor declarations subject to limitations defined by the at-rule. Descriptors are similar to properties (and are declared with the same syntax) but are associated with a particular type of at-rule rather than with elements and boxes in the tree.

The var() function can be used in place of any part of a value in any property on an element. The var() function can not be used as property names, selectors, or anything else besides property values. (Doing so usually produces invalid syntax, or else a value whose meaning has no connection to the variable.)

I'm sure there are more functions besides var that would be invalid for a descriptor declaration, but a rule that just warns for var would be a good start.

@silverwind
Copy link
Author

silverwind commented May 23, 2023

If stylelint had a proper parsers like proposed in stylelint/css-parser#2, this case wouldn't even need a rule, as the invalid syntax would trip that parser.

@ybiquitous ybiquitous added the status: needs discussion triage needs further discussion label May 24, 2023
@ybiquitous
Copy link
Member

@silverwind Thanks for the proposal. Could you please comment on a link to the CSS spec you quoted?

Also, does this syntax error occur with other at-rules?

@silverwind
Copy link
Author

silverwind commented May 25, 2023

Spec links are:

https://drafts.csswg.org/css-syntax-3/
https://drafts.csswg.org/css-variables/

As for @ rules, it seems it does not affect all of them. @page accepts var, but @font-face and @counter-style does not. So it seems the spec is not entirely true either. Happy to close this off as "too hard" if you like.

Here is what I tested:

@page {
  margin-top: var(--foo);
}
@font-face {
  font-weight: var(--foo);
}
@counter-style foo {
  system: var(--foo);
}

Firefox accepts the first syntax, but not the other two.

@alexander-akait
Copy link
Member

@silverwind yeah, I think it is more about grammar parsing than linting, so I hope in future we resolve problems with our parser and will catch this on the parser side (or will have rule to apply grammar parsing to each things in your CSS)

@silverwind
Copy link
Author

Agree, syntax errors are topics for a parser, not a linter.

@silverwind silverwind closed this as not planned Won't fix, can't repro, duplicate, stale May 25, 2023
@romainmenke
Copy link
Member

@ybiquitous We might want to keep this one open and add a rule for it regardless of anything that might be done for parsing CSS in general?
This seems like a common mistake that can easily be detected.

Thoughts?

@ybiquitous
Copy link
Member

@romainmenke Surely, this rule might help people to notice mistakes of var() etc. in at-rules.

But, I'm concerning about the @silverwind's comment:

So it seems the spec is not entirely true either. Happy to close this off as "too hard" if you like.

Can we solve this problem?


Anyway, I agree with reopening this and keeping it until we find a good solution. 👍🏼

@ybiquitous ybiquitous reopened this May 26, 2023
@romainmenke
Copy link
Member

romainmenke commented May 26, 2023

If I recall correctly this is the difference between descriptors and properties.
@font-face, @property, @counter-style all take a list of descriptors, whereas @page takes a list of declarations (property / value pairs).

https://www.w3.org/TR/css-variables-1/#using-variables

The value of a custom property can be substituted into the value of another property with the var() function. The syntax of var() is:

https://www.w3.org/TR/css-fonts-4/#font-face-rule

Edit :

@page also has a set of descriptors:

  • size
  • page-orientation

@ybiquitous
Copy link
Member

Ah, I see! Some at-rules accept properties, while some at-rules accept descriptors.

If so, we can likely manage at-rules accepting descriptors in reference/atKeywords.js.

@ybiquitous
Copy link
Member

If this rule is limited to at-rule, starting the rule name with at-rule- might be better. 🤔

Ref:

@silverwind silverwind changed the title Rule idea: no-invalid-descriptor-declaration Rule idea: at-rule-no-invalid-descriptor May 26, 2023
@silverwind
Copy link
Author

Agree, renamed title to at-rule-no-invalid-descriptor.

@ybiquitous
Copy link
Member

ybiquitous commented May 26, 2023

Okay, I suggest a blueprint:

  • Name: at-rule-no-invalid-descriptor
  • Description: Disallow invalid descriptors within at-rules.
  • Message: Unexpected invalid descriptor within "@<at-rule>"
  • Primary option: true
  • Secondary options: n/a
  • Category: Avoid errors > Invalid
  • Included in sharable configs?: No. But if it matured, we would include it in the recommended config.

Example:

@font-face {
  font-weight: var(--font-weight-normal);
/*             ^~~~~~~~~~~~~~~~~~~~~~~~~
               Unexpected invalid descriptor within "@font-face"
 */
}
{
  "at-rule-no-invalid-descriptor": true
}

Please feel free to give me any feedback.

@ybiquitous ybiquitous changed the title Rule idea: at-rule-no-invalid-descriptor Add at-rule-no-invalid-descriptor rule May 26, 2023
@silverwind silverwind changed the title Add at-rule-no-invalid-descriptor rule Proposal: at-rule-no-invalid-descriptor rule May 28, 2023
@Mouvedia
Copy link
Member

Mouvedia commented May 29, 2023

no-invalid-descriptor

Not sure if this is the right name because

:root {
  --lol: 6in;
}

@page {
  size : 4in var(--lol); /* size is a descriptor */
}

seems to be valid.

@silverwind
Copy link
Author

Right, it's only refering to the right side, so at-rule-no-invalid-descriptor-value.

@silverwind silverwind changed the title Proposal: at-rule-no-invalid-descriptor rule Proposal: at-rule-no-invalid-descriptor-value rule May 29, 2023
@Mouvedia
Copy link
Member

Mouvedia commented May 29, 2023

What I was trying to say is that the scope of no-invalid-descriptor-value might require to list every single cases in the documentation.
i.e. right now it's only var()—for certain at-rules—that has been identified but there are probably other cases

@silverwind
Copy link
Author

Ah, misunderstood you. If var() is valid in some constallations, it'll make the rule more complicated. I wonder if there is data around somewhere which CSS functions are considered invalid in descriptor values, there's certainly more than just var().

Regarding rule name, I think at-rule-no-invalid-descriptor-value may be a better name because then at-rule-no-invalid-descriptor could be used for a future rule that validates the descriptor names because at least some at-rules like @font-face have a pre-defined list of valid descriptor names.

@Mouvedia
Copy link
Member

We had the inverse problem with function-calc-no-invalid which scope ended up being too small.
We are trying to limit the future maintenance by picking the name precisely.
Ill let @jeddy3 chime in before continuing with the bikeshedding.

@romainmenke
Copy link
Member

Maybe it can be approached from a different perspective?

Part of the issue here is that var() is expected by many to just work everywhere.
Especially for people coming from preprocessors like scss.

A rule that warns on invalid locations of var() functions could cover :

  • used in @font-face descriptor values
  • used in @media feature values
  • used in selectors
  • ...

I think this is also easier to test and verify so that it is correct with the CSS specification.

@ybiquitous
Copy link
Member

Hum, @romainmenke's suggestion sounds good. So, how about function-var-no-invalid-location?

@jeddy3 jeddy3 changed the title Proposal: at-rule-no-invalid-descriptor-value rule Add a rule to disallow var() in descriptors Jun 24, 2023
@jeddy3
Copy link
Member

jeddy3 commented Jun 25, 2023

Part of the issue here is that var() is expected by many to just work everywhere.

That's a good summary.

I think this is also easier to test and verify so that it is correct with the CSS specification.

I agree.

So, how about function-var-no-invalid-location?

SGTM. It'll tie into the "The var() function can not be used as property names, selectors, or anything else besides property values" part of the spec that @silverwind mentioned above.

I had thought env and var were similar and so a rule would apply to them both, but it turns out env can be used in more places:

In addition, unlike custom properties, which cannot be used outside of declarations, the env() function can be used in place of any part of a property value, or any part of a descriptor (e.g. in Media query rules). As the spec evolves, it may also be usable in other places such as selectors.

Our closest existing rule is no-invalid-position-at-import-rule, so called because it's scoped to the whole source like this rule.

Suggested blueprint:

  • Name: no-invalid-position-var-function
  • Description: Disallow invalid position var() functions.
  • Message: Unexpected invalid position var() function
  • Primary option: true
  • Secondary options: n/a
  • Category: Avoid errors > Invalid
  • Included in sharable configs?: Yes, recommended.
  • Expanded description:

The var() function can be used in place of any part of a value in any property on an element. The var() function can not be used as property names, selectors, or anything else besides property values.

This rule checks:

  • at-rule descriptor names and values
  • media queries
  • property names
  • selectors

We'll want to ensure we don't needlessly call any of the constructor parsers when checking any of the above.

According to the CSS tree reference, the following at-rule have descriptors:

  • counter-style
  • font-face
  • page
  • property
  • viewport

Shall I label as ready to implement?

@ybiquitous
Copy link
Member

I agree with @jeddy3's suggestion 👍🏼

@jeddy3 jeddy3 changed the title Add a rule to disallow var() in descriptors Add no-invalid-position-var-function Jun 26, 2023
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new rule an entirely new rule and removed status: needs discussion triage needs further discussion labels Jun 26, 2023
Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ask to implement ask before implementing as may no longer be relevant type: new rule an entirely new rule
Development

No branches or pull requests

6 participants