unnecessary_parenthesis sometimes not reported for doubled parentheses

[oprypin]

Some examples of what it currently misses but should lint:

Int64((12 * 60 * 60))   // ArgumentList(BinaryExpression)

`a.compareTo((b.bar))`  // ArgumentList(MethodInvocation)

foo.addAll((await fut)) // ArgumentList(AwaitExpression)

`'${(foo == bar.baz)}'  // InterpolationExpression(BinaryExpression)

So I would like to introduce a rule that

• ${( )} and (( )) are never OK, drop the inner ones.

It's just that it's a bit tricky to find in the language what exactly constitutes these doubled parentheses. For example:

• ArgumentList with length of 1 is always an extra set of parentheses
  (which already covers almost all bad cases for this issue).
• So is an if statement, but somehow I can't trigger any false negative with it.

I will immediately open a pull request that would address this issue.

[bwilkerson]

I would assume that any pair of parentheses that doesn't change the semantics of the code is 'unnecessary' and that that's what this lint should flag. There is some evidence, however, that that might be too strict. I think we need to start by defining what this lint is expected to do, and then fix the implementation to match.

[srawlins]

I would assume that any pair of parentheses that doesn't change the semantics of the code is 'unnecessary' and that that's what this lint should flag. There is some evidence, however, that that might be too strict.

I think there is a lot of evidence that that is too strict. The general consensus is that it would be asking too much to expect every reader of code to know the precedence table. I'm seeing parallels in our other internal linters for JavaScript and Java, and in previous discussions for Dart. In particular there are surprises, like some of the test cases for this rule. Here's one example:

var x = (1==1 ? [] : [])..add(7);
var y = 1==1 ? [] : []..add(7);

Do the parentheses in the first statement change the semantics? Does x have 7? Does y?

I think we need to start by defining what this lint is expected to do, and then fix the implementation to match.

That expectation is spelled out here: https://dart-lang.github.io/linter/lints/unnecessary_parenthesis.html

I believe the "double parentheses" examples above represent a mismatch with this expectation, and are thus bugs.

[bwilkerson]

That expectation is spelled out here ...

The problem I have with that expectation statement is that there's not definition of what's covered by "improve the readability of the code". While I don't necessarily object to some of the requested relaxations, I have no idea, other than my own personal taste, of whether to say yes or no to a given relaxation. (That isn't what this issue is about, but still.)

I believe the "double parentheses" examples above represent a mismatch with this expectation, and are thus bugs.

Yes, because the parentheses don't "change the meaning of the code". And I agree that they don't improve readability either. Given that I don't disagree that these are reasonable to catch, I probably shouldn't have said anything. I was too influenced by the related issue (#3886) and trying to answer the question of how far to relax the rule.

[oprypin]

@bwilkerson I think your [first] comment is a call to completely stall this lint unless someone is willing to take it up as their main job. The comment also doesn't seem to talk about this particular issue but rather something general.

But I am not talking about something general or controversial. If one writes ((foo)) and that's somehow not unnecessary parentheses, then what is?

I think we need to start by defining what this lint is expected to do, and then fix the implementation to match.

I am defining the clearest and highest-priority rule that this lint has ever had, even if it's just 1 rule.

[oprypin]

To clarify, by "highest priority" I mean that the rule could be applied before any other rules and short-circuit the following logic

[bwilkerson]

Like I said, "I probably shouldn't have said anything".

[srawlins]

The problem I have with that expectation statement is that there's not definition of what's covered by "improve the readability of the code". While I don't necessarily object to some of the requested relaxations, I have no idea, other than my own personal taste, of whether to say yes or no to a given relaxation.

Yeah that's really fair. The devil is in the details, and even with a rule in my hand about operators-with-whitespace, or something similar, if you presented me with 10 different cases, I might not give a consistent answer about whether I think the whitespace is unnecessary.