Fix shorthand-property-no-redundant-values false negatives for var()

[gregmatys]

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

a { padding: 0 var(--padding) 0; }

What minimal configuration is needed to reproduce the bug?

{ "rules": { "shorthand-property-no-redundant-values": true } }

How did you run Stylelint?

Demo

Which Stylelint-related dependencies are you using?

json { "stylelint": "15.6.0" }

What did you expect to happen?

fix redundant values

What actually happened?

nothing

Do you have a proposal to fix the bug?

--

[ybiquitous]

@gregmatys Thanks for the report with the reproducible demo. This is a bug.

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

There are steps on how to fix a bug in a rule in the Developer guide.

[Mouvedia]

Do we allow the erasion of var(…)?
i.e. do we eval --padding in your example?

e.g.

  /* --padding is 0 */
  padding: 0 var(--padding) 0;

to

  padding: 0;

or without eval

  /* --padding is unknown */
  padding: 0 var(--padding) 0;

to

  padding: 0 var(--padding);

?

The latter is safer because it keeps var(--padding) in the source.
Another possibility is to do the former only if the custom property declaration is within the same file.

[ybiquitous]

"without eval" makes sense to me.

[gregmatys]

without evaluation for sure
because var() can be dynamic!

[ybiquitous]

I just opened PR #7657. It supports not only var(), but also other functions such as calc(). Please feel free to comment on the PR if you have any concerns. 🙏🏼

[romainmenke]

🤔 Is it an issue that with var() we can not know what that actual value will be?

:root {
	--padding: 20px;
}

@media screen {
	:root {
		--padding: 20px 5px;
	}
}

.a {
	padding: 0 var(--padding) 0; /* unknown what --padding actually is */
}

.b {
	padding: 0 20px 5px 0; /* no error */
}

.c {
	padding: 0 20px 0; /* Expected "0 20px 0" to be "0 20px" */
}

https://stylelint.io/demo/#N4Igxg9gJgpiBcCBOEIBcAEwA6A7baAtIQA4CGUUAlrgObwYBMADCQB4DceAvnngAIBbGNTIYAzmCQwYuLHzTwU6efjQFi5SjXpNWbDAFZ2XNb1zm8AOjE41W6nQbMMANzJIAFJoqPaASgxmU0tcKwAjVQIHHWc9diME4J4+MLAotBinIPiDZIs8EAAaEAAzKgAbGAA5MmEEEBg2OpIqqzBxcWLwCFxy2ga7DAxsRrY0WShxUYYAbTxh4dHxNABPKoqaIkg+qlpCFbJcKA8oUYWMAF0ebp3+gDEIJEEyNAaAK3Fe7tgSLsQhiMQCt1jBNrh1AggRVXjAVqMihdlmsNltCHc9gc0EcTkgzlDRjCJvCQDduEA

[Mouvedia]

--padding: 20px 5px;

To me that's a blocker even if it eval the (re)declarations of the current file.
i.e. if the custom property can be reset dynamically the rule shouldn't report

The resetting cannot be ruled out for 2 reasons:

• setProperty
• other files

tl;dr: IMHO it should be ignored
i.e. this issue should be closed as by design

The PR could add support for functions like calc though.

[romainmenke]

this issue should be closed as by design

In that case we should probably make that explicit behavior by adding a test case for it.

I agree that whenever var() is involved it becomes exceedingly hard to give correct and meaningful feedback, let alone do auto fixes.

[ybiquitous]

Thanks for the feedback. I missed cases like --padding: 20px 5px;. I agree var should be ignored for this rule. 👍🏼

I will continue to work on PR #7657 to exclude var support since calc support still makes sense.

I'm closing this issue.

[gregmatys]

Thanks for your work on that issue. You're right it's hard to precisely exclude the var issue here.