Explicit parentheses in operations where precedence might not be clear #175
Conversation
Codecov Report
@@ Coverage Diff @@
## master #175 +/- ##
==========================================
+ Coverage 98.55% 98.64% +0.08%
==========================================
Files 74 74
Lines 553 589 +36
Branches 84 87 +3
==========================================
+ Hits 545 581 +36
Misses 8 8
Continue to review full report at Codecov.
|
| e % | ||
| f ** | ||
| g; | ||
| (((c * d) / e) % f ** g); |
There was a problem hiding this comment.
I think (c * d / e) % f ** g might be a little better, but I don't know how hard that would be to do.
There was a problem hiding this comment.
it has parentheses because it's following veryVeryVeryVeryVeryLongVariableCalledB -
if you try prettier veryVeryVeryVeryVeryLongVariableCalledA + veryVeryVeryVeryVeryLongVariableCalledB - c * d / e % f ** g in javascript it will create the same parentheses.
+ and - are well known to have little precedence but modulo % is a tricky one that's why I think prettier js wraps the % between parentheses, you can check this rule in lines 43 and 44 of src/parser.js in this PR.
Anyway, we don't have to follow by the letter each rule of prettier js, I just thought it was a good place to start, we can add or remove rules according to what makes more sense. My main concern is that the parentheses suggestions have to follow the order of precedence of operators, otherwise, the bytecode will not be the same when compiled.
There was a problem hiding this comment.
Sorry, I mean specifically the parentheses around c*d. I think c * d / e is better than (c * d) / e, but that's just me. Besides, if that's how prettierjs does it, I'm more than fine with following its lead.
| @@ -2,6 +2,16 @@ const extract = require('extract-comments'); | |||
| // https://prettier.io/docs/en/plugins.html#parsers | |||
| const parser = require('solidity-parser-antlr'); | |||
|
|
|||
| const tryHug = (node, operators) => { | |||
There was a problem hiding this comment.
Maybe we should document this function.
There was a problem hiding this comment.
yeah, that's not a bad idea actually
| veryVeryVeryVeryVeryLongVariableCalledA && | ||
| veryVeryVeryVeryVeryLongVariableCalledB || | ||
| (veryVeryVeryVeryVeryLongVariableCalledA && | ||
| veryVeryVeryVeryVeryLongVariableCalledB) || |
There was a problem hiding this comment.
interesting indentation, is that on purpose?
There was a problem hiding this comment.
yeah, well the indentation rule here falls into the TupleExpression indentation rules instead of BinaryOperation indentation rules.
Same happens in javascript.
// Before Prettier
if (veryVeryVeryVeryVeryLongVariableCalledA && veryVeryVeryVeryVeryLongVariableCalledB || c);
// After Prettier
if (
(veryVeryVeryVeryVeryLongVariableCalledA &&
veryVeryVeryVeryVeryLongVariableCalledB) ||
c
);| @@ -2,6 +2,16 @@ const extract = require('extract-comments'); | |||
| // https://prettier.io/docs/en/plugins.html#parsers | |||
| const parser = require('solidity-parser-antlr'); | |||
|
|
|||
| const tryHug = (node, operators) => { | |||
There was a problem hiding this comment.
yeah, that's not a bad idea actually
Ok,
So while working on Binary operations and properly printing them across the different scenarios I stumbled on this case
a && b || c && dprettier JS says "ok this one is difficult for a normal developer to easily read" (and I agree) so it formats it like this(a && b) || (c && d).So why doesn't prettier-solidity does this?
Because we have to change the AST.
I wouldn't want to add to the execution stack or have the contract use more gas just because one way of writing the code looks nicer.
So I created a personal repo where I could test these changes and I discovered that as long as the order of precedence is respected, the bytecode is the same, regardless of the parentheses.
With this green light, I proceeded to imitate all of the parentheses rules that prettier is doing for js.
The change is very localised and is easily removed if it causes problems.
I understand this one might need a bit more scrutiny.