dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.3k stars 1.59k forks source link

Suggestion for a new linter rule for avoiding negative conditionals in Dart style guide. #59235

Open Turskyi opened 1 year ago

Turskyi commented 1 year ago

Negative conditionals are conditional expressions that use the logical negation operator ! to invert the value of a boolean expression. For example, the following code uses a negative conditional:

if (!buffer.shouldNotCompact()) { // do something }

Negative conditionals can sometimes make the code harder to read and understand, especially if they are nested or combined with other logical operators. Therefore, I suggest creating a new linter rule that warns about the use of negative conditionals and suggests using positive conditionals instead. For example, the previous code can be rewritten as:

if (buffer.shouldCompact()) { // do something }

The benefits of using positive conditionals over negative conditionals are:

I think this new linter rule would be useful for improving the clarity and quality of Dart code, as well as following the general guidelines for writing clear and concise code in the official Dart style guide. I would appreciate any feedback or suggestions on this idea. Thank you. 😊

lBasilliskl commented 1 year ago

Thanks for the useful idea. I hope that more people will be interested in this method.

lrhn commented 1 year ago

This does not seem like something that can be easily automated, which may make it a bad match for a lint.

First of all, there might not be any buffer.shouldCompact. If there isn't, and the buffer is not your own code, then there is nothing you can do about it. (If it is your own code, in the same package, then maybe a suggestion to provide a positively named alternative could be given, but we don't know why it isn't here.)

Then there is the issue of deciding whether a name is negative.

Containing Not or Non as CamelCase parts can be a signal, but for example isNonNullable is not the opposite of isNullable in the Dart semantics. (But isNotNullable is. Because things are complicated.) Containing Un or Im prefixes could also be a signal, like isImmutable or isUnmodifable. But it's a flaky signal. An isUnderpant is not the opposite of isDerpant and isImpOrFairy is not the opposite of isPOrFairy 😉

Having any chance here requires either a large lexicon of words, a good heuristic, or, more likely, just a fixed set of known APIs that have both a positive and a negative version of a query. Which is what we have today with prefer_is_not_empty, it's just a very short list.

Turskyi commented 1 year ago

@lrhn, thank you for your reply and your feedback. I understand that this lint rule may not be easy to implement and that there may be many cases where the name of a boolean expression is not clearly negative or positive. I appreciate your explanation of the challenges and limitations of this idea.

Avoiding negative conditionals can improve the readability and clarity of Dart code, and it would be helpful to have a lint rule that can suggest positive alternatives when possible. However, I respect your decision and expertise on this matter, and I will not be very strict about this lint rule if you think it is not feasible or useful.

If you think this issue is impossible to implement or not worth pursuing, then you may close it. Otherwise, I would be happy to hear more suggestions or opinions on how to make this lint rule better or more practical.

Thank you for your time and attention. 😊

lrhn commented 1 year ago

The example here is a double negation.

It could easily be a rule to avoid a single negation, when the conditional has an else branch, so at least rewriting

if (!buffer.shouldNotCompact()) { 
  // do first something 
} else {
  // do second something
}

into

if (buffer.shouldNotCompact()) { 
  // do second something 
} else {
  // do first something
}

Then that author may choose to change that to buffer.shouldCompact instead of changing the order, if they know that they can.

For the more complicated detection and negation of boolean predicates based on names, it's not impossible (double negation, so it's possible), to have a list of known opposite predicates, like the isEmpty/isNotEmpty, and extend it with, fx, isImmutable/isMutable, isClosed/canClose, or even known binary options isPositive/isNegative.

It would be properties that actually exist on at least one commonly used type, and which are known opposites. Maybe even remember the type and only apply to that, or choose to apply it to any type which has both members as booleans.

(But in the end, I'm not going to be the one implementing this, I'm just pointing out the likely implementation issues. If someone else comes up with a way around those, I think it would be a valuable readability lint.)

bwilkerson commented 1 year ago

I don't know whether it would be worthwhile because I don't know how often this comes up in practice, but a more general solution would be to define an annotation that can be used to identify antonyms within an API. For example,

class Buffer {
  @AntonymOf(#shouldCompact)
  bool shouldNotCompact() => ...;

  @AntonymOf(#shouldNotCompact)
  bool shouldCompact() => ...;
}

That would allow any API to self identify the inverse cases, and would allow the analyzer to rewrite !buffer.shouldNotCompact() as buffer.shouldCompact().

We'd need to have some checks to ensure that the annotation was being used correctly, and we'd likely still need a hard-coded list for the SDK libraries (because I doubt that we'd add the annotation to the SDK), but it would be more general.