less / less.js

Less. The dynamic stylesheet language.
http://lesscss.org
Apache License 2.0
17.02k stars 3.41k forks source link

Fix #3616 IfStatement requires double parentheses when dividing #3626

Closed iChenLei closed 3 years ago

iChenLei commented 3 years ago

PR Target

Try to fix https://github.com/less/less.js/issues/3616

Debug analysis

If you write less (v4 parens-division default) code like follow:

.parens-issues-3616 {
  bar: if(false, 666, 888 / 444);
  bar2: if(false, 666, (666 / 333));
  bar3: if(false, 666, ((444 / 222)));
}

VSCode debug screen snapshot

2021-06-11 21 24 47

Call Stack

ParseTree -> ruleset eval -> declaration eval -> value eval -> expression eval -> call eval -> functionCaller.call -> If -> operation eval

Input Expected Result
bar2: if(false, 666, (666 / 333)); bar2: 2; bar2: 666 / 333;

In our expacted logic the Operation's eval output should be a Unit which value is 2.

(666 / 333) compile to a Expression which contains a Operation

2021-06-11 21 43 39

The Operation's eval logic

   //  packages/less/src/less/tree/operation.js
    eval(context) {
        let a = this.operands[0].eval(context), b = this.operands[1].eval(context), op;

        if (context.isMathOn(this.op)) {   //  <- we get false in this case
             /**** omit unimportant code  ****/
            return a.operate(context, op, b);  // <-  so we can't get expected output from here
        } else {
            return new Operation(this.op, [a, b], this.isSpaced);
        }
    },

Our context lose parens !!!

   //  packages/less/src/less/functions/function-caller.js:29 
        args = args
            .filter(commentFilter)
            .map(item => {
                if (item.type === 'Expression') {
                    const subNodes = item.value.filter(commentFilter);
                    if (subNodes.length === 1) {
                        return subNodes[0];    //  <-  lose Expression and get a Opreation
                    } else {
                        return new Expression(subNodes);
                    }
                }
                return item;
            });

So the context.isMathOn(this.op) is failed. I know my analysis is not very clear, but I think maintainers know what i say. Hope this PR is useful, maintainers can modify my code directly cause i am not a less source code expert. Any help welcome.

/cc @matthew-dean

iChenLei commented 3 years ago

BTW: I noticed that less still use deprecated travis-ci(travis-ci.org), please migrate to travis-ci.com(migrate to Github Action is also a good choice)

2021-06-11 22 17 10

matthew-dean commented 3 years ago

@iChenLei Thanks!