Fix declaration-block-no-redundant-longhand-properties autofix conflicts

[Mouvedia]

Which issue, if any, is this issue related to?

Closes #7610

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

[changeset-bot]

🦋 Changeset detected

Latest commit: 093fd05

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
stylelint	Patch

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

[ybiquitous]

I notice that this problem is about border-*-style false positive, rather than generic autofix. See the demo:

border-*-style props are reported too much, comparing border-*-width and border-*-color.

[Mouvedia]

Apart from

Expected shorthand property "border" [(declaration-block-no-redundant-longhand-properties)](https://stylelint.io/user-guide/rules/declaration-block-no-redundant-longhand-properties) [13:2-19]

which might be covered by #7609 in a particular case:

a {
	border-top-width: foo;
	border-right-width: foo;
	border-bottom-width: foo;
	border-left-width: foo;
	border-top-color: bar;
	border-right-color: bar;
	border-bottom-color: bar;
	border-left-color: bar;
	border-top-style: qux;
	border-right-style: qux;
	border-bottom-style: qux;
	border-left-style: qux;
}

the rest looks OK.
i.e. I expect 7 warnings but only 3 fixes attempted.

are reported too much

Just think about it, the detection of the shorthand possibilities for border-top, border-right, border-left and border-bottom happen once you reach the third match. Just check the implementation, it's logical. It has nothing to do with border-*-style and everything to do with the position of the block. Switch them up you will understand.
e.g. demo

[ybiquitous]

I understood this PR's fix cannot resolve the false positives. Honestly, I think we should reconsider the rule's implementation, not adding a workaround patch, because this rule has been fixed at many times recently.

[Mouvedia]

@ybiquitous I have added an if around the forEach so that it doesn't loop for other shorthands.
I feel that this is a major bug: the autofix is removing code.
This PR is fine as is; it is scoped and I took care of the perf aspect as well.
If you have suggestions for other tests Ill welcome them.
Concerning a refactor, it will have to wait.
I can wait for further review to come in if you are not confident.
I hope it will be part of 16.4.1.

[ybiquitous]

@Mouvedia I'm against adding this workaround, and again we should reconsider the implementation even if it would be a big change. Honestly, I don't want to review PRs so many times only for one rule, and this workaround may occur other problems.

In addition, this bug has been not found for a long time. You say this is "major", but I guess many people are not in trouble.

I think it's a good timing to detect the rule's better implementation design.

[Mouvedia]

Ill just explain what this fix does then, maybe that will change your mind.
The rule creates an array that it populates with longhands that it encounters inside a declaration block, for each shorthand supported. Once the array matches it can move on to the fixer.
Before the fix, once the replacement of the longhands by the shorthand was done it didn't remove those longhands from the other arrays that were being populated at the same time. Inevitably it would cause useless autofixes to be attempted because the arrays weren't cleaned up. The first iteration of the fix was doing the cleanup without any regard of whether the shorthands were affected by the bug. The last iteration only does it for a shortlist of shorthands, it's less future proof and requires to maintain a blacklist.

I hope that's clearer and I managed to convince you.

[ybiquitous]

Okay, I agree with merging this since it resolves at lease autofix problem. But I still have the opinion to reconsider this rule's implementation as soon as possible.

If you fix the changelog, you can merge. LGTM.

[Mouvedia]

But I still have the opinion to reconsider this rule's implementation as soon as possible.

This is a legitimate worry. Having 2 rules depending on the sets and having to jungle them both every time they are changed can be considered daunting. If we were to take a perf approach for the refactor I think @romainmenke might be interested by it based on his work on #6869.

If you fix the changelog, you can merge. LGTM.

I will fix the changelog.
Let's wait for several days; like you said we are not in a hurry.

[ybiquitous]

The discussions about this rule have been opened in #7630 and #7631. They indicates there is some problem in the rule implementation.

[Mouvedia]

…you can merge…

I have added more tests to cover more permutations.

The discussions about this rule have been opened in #7630 and #7631. They indicates there is some problem in the rule implementation.

We can continue the discussion around maintenance and refactors there.