Page MenuHomePhabricator

[Regression] Mixin copies rules from unrelated block
Closed, ResolvedPublic

Description

Originally reported by fuf13 at https://github.com/wikimedia/less.php/issues/108 on 5 April 2024.

Input file:

input.less
.btn {
   color: blue;
   font-weight: bold;

   &[title] {
      background: lightgreen;
   }
}
.btn[disabled] {
   cursor: not-allowed;
   color: red;
}
.btn[test] {
   background-color: pink;
}

.btn-specific {
   .btn();
}

Expected output, as correctly generated by Less.php 4.1.1 (and earlier), and upstream Less.js 2.5.3 and Less.js 3.13.

output.css
.btn {
  color: blue;
  font-weight: bold;
}
.btn[title] {
  background: lightgreen;
}
.btn[disabled] {
  cursor: not-allowed;
  color: red;
}
.btn[test] {
  background-color: pink;
}
.btn-specific {
  color: blue;
  font-weight: bold;
}
.btn-specific[title] {
  background: lightgreen;
}

Actual output, as of Less.php 4.2.0 and still the case in latest master:

.btn {
  color: blue;
  font-weight: bold;
}
.btn[title] {
  background: lightgreen;
}
.btn[disabled] {
  cursor: not-allowed;
  color: red;
}
.btn[test] {
  background-color: pink;
}
.btn-specific {
  color: blue;
  font-weight: bold;
  cursor: not-allowed;
  color: red;
  background-color: pink;
}
.btn-specific[title] {
  background: lightgreen;
}

Notice how this block includes rules from unrelated "disabled" and "test" blocks:

.btn-specific {
  color: blue;
  font-weight: bold;
  cursor: not-allowed;
  color: red;
  background-color: pink;
}

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
$ git bisect start      
status: waiting for both good and bad commits

$ git bisect good v4.1.1
status: waiting for bad commit, 1 good commit known

$ git bisect bad v4.2.0
Bisecting: 28 revisions left to test after this (roughly 5 steps)
[24540bf92a0be3348e43d8156d3aafed16ac36b4] …
$ bin/lessc input.less

$ git bisect good
Bisecting: 14 revisions left to test after this (roughly 4 steps)
[fdfd34651862e5ea787192d1183952f2cfeaad60] …
$ bin/lessc input.less

$ git bisect good
Bisecting: 7 revisions left to test after this (roughly 3 steps)
[201d390afde84d8c71ac794f56f7d3c31753158b] …
$ bin/lessc input.less

$ git bisect good
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[7f7129101c31b62b83d31f6f691c2dbc97807e86] …
$ bin/lessc input.less

.btn-specific {
  color: blue;
  font-weight: bold;
  cursor: not-allowed;
  color: red;
  background-color: pink;
}

$ git bisect bad 
Bisecting: 1 revision left to test after this (roughly 1 step)
[21e77ca4ffe657fe429d92bc71a07a8c91a3a381] …
$ bin/lessc input.less

.btn-specific {
  color: blue;
  font-weight: bold;
  cursor: not-allowed;
  color: red;
  background-color: pink;
}

$ git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[6cb92a99774b1228f5f1061d3aba34a7d54a7bab] …
$ bin/lessc input.less

$ git bisect good
21e77ca4ffe657fe429d92bc71a07a8c91a3a381 is the first bad commit

Fix interpolated selector & String interpolation
Bug: T353142
Change-Id: I9eae221cbb3fe93b17d79a66e9b3425cbe24163b
https://gerrit.wikimedia.org/r/c/mediawiki/libs/less.php/+/993712

/cc @matmarex @Hokwelum

I tried a few things and looks like this was caused by the removal of the early return here: https://gerrit.wikimedia.org/r/c/mediawiki/libs/less.php/+/993712/10/lib/Less/Tree/Selector.php#b109

But restoring it breaks other test cases, so the issue is more complex.

I suspect something is wrong in Selector::CacheElements() or in Ruleset::find(). These methods are involved with mixins, and the code doesn't match either Less.js 2.5.3 nor 3.13.1.

Change #1023570 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/libs/less.php@master] Fix Mixins duplicating rules

https://gerrit.wikimedia.org/r/1023570

Change #1023570 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Fix unexpected duplicating of uncalled mixin rules

https://gerrit.wikimedia.org/r/1023570