Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #3616 IfStatement requires double parentheses when dividing #3626

Merged
merged 2 commits into from Jun 12, 2021
Merged

Fix #3616 IfStatement requires double parentheses when dividing #3626

merged 2 commits into from Jun 12, 2021

Conversation

iChenLei
Copy link
Member

PR Target

Try to fix #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
Copy link
Member Author

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
Copy link
Member

@iChenLei Thanks!

@matthew-dean matthew-dean merged commit c518180 into less:master Jun 12, 2021
@iChenLei iChenLei deleted the fix-parens-bug branch June 12, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if() requires double parentheses when dividing
2 participants