Add --fix=lax and --fix=strict

[ybiquitous]

What minimal example or steps are needed to reproduce the bug?

Analyzing a CSS file with an invalid syntax raises a syntax error expectedly, while doing the same specifying --fix succeeds unexpectedly.

test.css:

// comment

---

For reference, this behavior difference depends on postcss-safe-parser:

stylelint/lib/getPostcssResult.mjs

Line 127 in 382961f

const parse = await (fix ? import('postcss-safe-parser').then((m) => m.default) : postcss.parse);

Note: we found this problem in #7494.

What minimal configuration is needed to reproduce the bug?

{"rules": {}}

How did you run Stylelint?

Run the following commands in my console.

$ npx stylelint test.css

test.css
 1:1  ✖  Unknown word  CssSyntaxError

1 problem (1 error, 0 warnings)

$ npx stylelint test.css --fix

$ echo $?
0

Which Stylelint-related dependencies are you using?

package.json:

{
  "dependencies": {
    "stylelint": "16.2.0"
  }
}

What did you expect to happen?

Expect to raise a syntax error. E.g.

$ npx stylelint test.css --fix

test.css
 1:1  ✖  Unknown word  CssSyntaxError

1 problem (1 error, 0 warnings)

What actually happened?

A syntax error does not happen, and then Stylelint exits normally. E.g.

$ npx stylelint test.css --fix

$ echo $?
0

Do you have a proposal to fix the bug?

1. do something similar to https://github.com/noahbrenner/xo/blob/8e4f4355c4b8e62bc28f37e3673bb6872e693bb0/main.js#L119-L133

2. update fix option

true maps to "lax"
false doesn't change
"lax" aliases to true
"strict" added

[Mouvedia]

I've labeled the issue as ready to implement. Please consider contributing if you have time.

[ybiquitous]

I looked into the introduction of postcss-safe-parser. The PR was #2886.

postcss-safe-parser can fix syntax errors like this:

$ echo 'a {' | npx stylelint

<input css 1>
 1:1  ✖  Unclosed block  CssSyntaxError

1 problem (1 error, 0 warnings)


$ echo 'a {' | npx stylelint --fix
a {
}%

Of course, this package cannot fix some syntax errors like // comment:

$ echo '// comment' | npx stylelint

<input css 1>
 1:1  ✖  Unknown word  CssSyntaxError

1 problem (1 error, 0 warnings)


$ echo '// comment' | npx stylelint --fix
// comment

For now, I cannot find a solution, but let us know if anyone has some information.

[ybiquitous]

For reference, an incomplete comment like /* comment is fixable:

$ echo '/* comment' | npx stylelint --fix
/* comment
*/%

[Mouvedia]

What about some heuristics based on the previous iteration that switch between postcss.parse and postcss-safe-parser?
i.e. a partial revert of #7357

[ybiquitous]

That heuristics use file extensions supporting the // comment form (e.g. .scss, .less etc.), right? I think the way is against our standard CSS preferred policy:

stylelint/docs/developer-guide/rules.md

Line 13 in 382961f

- for standard CSS syntax only

[Mouvedia]

The alternative of not using postcss-safe-parser for --fix is worse though.

The worst that can happen with the partial revert is having someone using CSS-compatible code in a scss file that would trigger the warning erroneously. In that case it's just a matter of detecting the absence of CSS syntax error and adding a hint regarding the possibility of changing the extension.

The list maintenance is not a strong argument against it IMHO.
It's either a blacklist—e.g. sss, sass, scss, styl, less—or the previous whitelist plus mcss.
In both cases, it won't probably be touched for years because it covers most use cases.
i.e. the regression of the DX matters more than strictly enforcing our policy

[ybiquitous]

Hum..., I'm still concerned that the file extension list based on heuristics may grow at people's request. In addition, if we'd like to remove the list itself or some extensions from the list, we must care about backward compatibility.

First of all, performing autofix first is an edge case, right? I believe normal use cases are below:

1. Run stylelint test.scss -> raises a syntax error.
2. Then, most users should notice something wrong with their code.

Furthermore, we encourage using the customSyntax option in the "Getting started" guide:

stylelint/docs/user-guide/get-started.md

Line 57 in 382961f

### Using a custom syntax directly

So, I doubt the worth of this heuristic, though ideally, the bug should not exist.

[Mouvedia]

We can wait for the resolution of stylelint/vscode-stylelint#490 and reevaluate then.
IMHO the 3 issues related to this issue that were either closed or transferred won't be the last.
Personally I think that the message was useful and helping.
I disagree about the list maintenance; new extensions are pretty rare.

[ybiquitous]

For reference, we previously removed an inaccurate syntax inference (heuristic) with PR #6645.

[Mouvedia]

For reference, we previously removed an inaccurate syntax inference (heuristic) with PR #6645.

That one looks more like something I wouldn't recommend we maintain, I agree for that list.

[ybiquitous]

We can wait for the resolution of stylelint/vscode-stylelint#490 and reevaluate then.

Agree. We can revisit this issue if many people complain.

[ybiquitous]

Also, we can revisit if a better solution is found.

[Mouvedia]

Also, we can revisit if a better solution is found.

What about a new flag?
i.e. --validate-syntax or --force-validation which defaults to false (i.e. not being passed) when using --fix

We could use a different validator for that flag or simply postcss.parse.
e.g. csstree has a validator plugin
In that regard the only criterium should be spec compliancy, not speed.
i.e. I would expect that passing that new flag without fix: true could switch to a stricter validator
@romainmenke which one is the best in terms of strictness?

[ybiquitous]

If most people don't care about this bug, I guess adding a new flag is too much.

[Mouvedia]

Any thoughts?

It's elegant because it doesn't require a new flag.
I would support

• --fix=lax as you described, uses postcss-safe-parser
• --fix=strict uses postcss (as previously)
• --fix=stricter uses a stricter parser (to be chosen in a dedicated issue)

• Make --fix=strict default in the next major version
• Remove --fix=lax in the future

I don't have an opinion. As long as it's configurable I am satisfied.

[ybiquitous]

At this time, what is "a stricter parser"?

[Mouvedia]

what is "a stricter parser"?

Let's say that as a prerequisite, it should take into account the values.
e.g. it throws for a { border-style: foo; border-width: -1px; } but validates a { -webkit-line-clamp: none; }

It's a can of worms if the dependency is not well maintained but if stricter is considered experimental having edge cases would be acceptable.
i.e. if it's that strict it might throw perfectly valid CSS erroneously

[romainmenke]

what is "a stricter parser"?

it throws for a { border-style: foo; border-width: -1px; } but validates a { -webkit-line-clamp: none; }

I would narrow the scope as much as possible so that it doesn't overlap with any other rule.

[Mouvedia]

I would narrow the scope as much as possible so that it doesn't overlap with any other rule.

That would either

1. require the parser to have many option that we can disable or
2. to filter the errors in a switch-case (i.e. the parser provides a good granularity for the errors)

I would prefer 1. but I am pretty sure we won't find one, so 2. will be a hard requirement if we want to avoid overlaps.
FYI biome has the same problem: biomejs/biome#2422
i.e. its parser covers the job of existing rules

[romainmenke]

I see what you mean, but I fear that this stricter parser would in a way become an entirely new Stylelint :D

I am sure that there will be some overlap and that is fine, but I would still like to minimize this as much as possible.

You could see it as a layer cake:

1. does it tokenize without producing errors?
2. does it parse according to css-syntax-3 without producing parse errors?
3. do individual features parse correctly? (e.g. media-query-no-invalid)
4. are the value types correct?
5. does the stylesheet adhere to user preferences and conventions?

For me a strict parser would only check layers 1 and 2.

[ybiquitous]

I also want to keep things simple. And, if the current --fix (lax) can produce invalid results, --fix should be strict. I think we should keep only --fix after making strict default in the future.

[Mouvedia]

Let's split this proposal into 2:

1. add "strict"/"lax" support to the flag/option --fix/fix
2. define "stricter" precisely and then eventually add it if we reach a consensus

Concerning 2. I think that it's simpler to handle value position/type/enum validation when it matters (i.e. before --fix) than to add the validation for all affected rules one by one.

[ybiquitous]

Please read #7497 (comment)
I think it's better to provide only --fix finally.

[Mouvedia]

I am dropping my "stricter" proposal so that we can move on with the issue at hand.
For reference here's the issue that proposed to change the parser: #7116
I have updated the Do you have a proposal to fix the bug? section.

related: sindresorhus/meow#44
We want --fix to map to --fix="lax" so that existing code remains backward-compatible.

[ybiquitous]

@Mouvedia Thanks for sharing the Meow issue. Is there any workaround for the problem if Meow doesn't address it?

[Mouvedia]

There is, if you switch to ready to implement I will show you.
i.e. --fix=foo doesn't throw so you can retrieve the value even if the type remains "boolean"

[ybiquitous]

Alright. I've labeled the issue as ready to implement. Please consider contributing if you have time.