Skip to content

Lint that warns about comparing boolean expressions against literals #58533

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

Closed
Pante opened this issue Oct 6, 2021 · 9 comments · Fixed by dart-archive/linter#3973
Closed
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-lint-request type-enhancement A request for a change that isn't a bug

Comments

@Pante
Copy link

Pante commented Oct 6, 2021

Describe the rule you'd like to see implemented
Comparing boolean expressions against a boolean literal is almost always a code smell. Less experienced developers are more likely to perform such comparisons. Therefore, I think it will be beneficial to have a lint that warns against such comparisons.

In my opinion, the lint should be applied inside all conditional expressions and maybe even all comparisons.

if (isDigit() == true) {
   ...
}
while (isDigit() == true) {
   ...
}
final foo = isDigit()== true ? a : b;

The lint would also need to take into account if the boolean literal is on the LHS of the comparison.

@Pante Pante added type-enhancement A request for a change that isn't a bug linter-lint-request labels Oct 6, 2021
@bwilkerson
Copy link
Member

Except in cases where the non-literal operand has the type bool?.

@srawlins
Copy link
Member

srawlins commented Oct 6, 2021

This makes sense to me, specifically if isDigit() returns a non-nullable bool. If it returns dynamic, Object, bool?, etc, then I like the == true code.

@pq
Copy link
Member

pq commented Oct 6, 2021

If it returns dynamic, Object, bool?, etc, then I like the == true code.

For the bool case, I prefer ?? false. See also:

https://dart.dev/guides/language/effective-dart/usage#prefer-using--to-convert-null-to-a-boolean-value

EDIT: and related lint use_if_null_to_convert_nulls_to_bools

(Sorry for the derail!)

@munificent: any additional thoughts?

@incendial
Copy link
Contributor

If you willing to try a custom analyzer plugin called Dart Code Metrics, we have this rule implemented https://dartcodemetrics.dev/docs/rules/common/no-boolean-literal-compare among with other useful lint rules.

@Pante
Copy link
Author

Pante commented Oct 10, 2021

If you willing to try a custom analyzer plugin called Dart Code Metrics, we have this rule implemented https://dartcodemetrics.dev/docs/rules/common/no-boolean-literal-compare among with other useful lint rules.

That's really neat! However, I still think it will be far more beneficial if this rule was included inside the base analyzer since not everyone can use a custom analyzer plugin.

@incendial
Copy link
Contributor

since not everyone can use a custom analyzer plugin.

Could you elaborate on that? It's really important for us to eliminate all blockers from our side.

@munificent
Copy link
Member

I'm 100% on board with this lint. I'm actually surprised we don't have it already.

@Pante
Copy link
Author

Pante commented Oct 13, 2021

since not everyone can use a custom analyzer plugin.

Could you elaborate on that? It's really important for us to eliminate all blockers from our side.

Just my two cents, please take it with a grain of salt:

In my opinion, most of the reasons aren't technical. Some organizations may be reluctant to adopt it simply due to internal policies. (Libraries must be audited etc.). Another issue is that in my opinion, most people may not be even aware that dart-code-metrics exist. They can't really use something if they aren't even aware that it exists. Personally I didn't even know that the dart-code-metrics existed before you mentioned it in this issue haha.

Sorry for being off-topic.

@incendial
Copy link
Contributor

Another issue is that in my opinion, most people may not be even aware that dart-code-metrics exist. They can't really use something if they aren't even aware that it exists.

Thank you very much for the answer! I completely agree with that and it's what we're trying to solve right now.

Sorry for the off-topic too.

@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-lint-request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants