Tracking `literal_only_boolean_expressions` Fixes

[RohitSaily]

This issue is a thread for discussing and tracking planned fixes for literal_only_boolean_expressions. The goal of this is to identify and resolve all relevant cases that require updating with a single patch.

I will continually edit this post to keep it updated with any key information related to the fix. I will also link the in-progress patch here once I have started it.

These fixes follow a prior minor fix.

---

Current cases to handle - provided by @bwilkerson:

• adjacent strings when some of the strings aren't constants (false positive)
• list, map, and set literals whose elements are not all constants (false positive)
• dot shorthand expressions that reference constants (false negative)

[RohitSaily]

Question 1

Should it be ensured that this lint flags any conditions that are compile-time computable? For example

const a = 0, b = 1;
if (a == b); // Currently not linted.

In the rare case one is working with very complex/composed compile-time data then they can suppress the lint, clearly indicating and choosing to have it checked.

---

Question 2

Should expressions comparing collection literals always be flagged? They are either const and known; not const and will always create distinct instances and never be identical; or a combination.

---

Would appreciate any input @bwilkerson @srawlins @FMorschel

[FMorschel]

I'd say so, these are good points. If we can process those, at least. I'm not sure if we have something else for those, but I guess not (@lrhn, do you know?).

Would e also be triggered then? Currently, this whole example triggers nothing from this lint.

const a = 0;
const b = 0;
const c = 1 == 2 ? 3 : 4;
const d = a == b ? c : 5;
const e = '' == '';

I'm not sure what you prefer, but if you have the time and prefer the fixes to be split by different CLs, feel free to do so. Doing them all in one is probably fine too, but it may take longer for reviewers to get to it.

[RohitSaily]

Thanks for your input.

The lint currently only checks control flow conditions. I would also like to generalize it to any boolean expressions. So with that, yes I would also lint e.

Once I decide the approach I'm taking I will try to divide it into multiple changes as you suggested if it ends up not being a simple general solution.

[srawlins]

I think the reason it's implemented the way it is so far (with much of @FMorschel 's help, if I remember), is getting the most bang for your buck.

For example, if I proposed a lint rule that warned you about comparing x.toLowerCase() == y.toUppercase(), that might sound like a very silly rule. Who would accidentally do that. But, well, there is some value there, for the occasional code that accidentally gets written this way, by a human or an LLM. So if it has any merest hint of value, shouldn't we implement it? Well we have to consider the costs:

• Every first-party lint rule is code that the Dash team must maintain; when new language features come out (like patterns, dot-shorthands) we must look at rules like this and add fixes and tests.
• Also the cost to the user running the lint rule. Each time you press a key in your IDE, and this lint rule is enabled, we must compute any lints. Adding constant evaluation could be expensive. Checking on every expression vs just conditions like if-conditions adds (maybe just a tiny bit) of expense.
• And there is also the cognitive load of a team knowing all of the lint rules that are enabled. That doesn't play much of a role when making a rule stricter. But it does come into play when adding a new rule, comparing_an_uppercase_string_with_a_lower_case_string.

I think that checking constant values of all expressions (in conditions) is beyond the scope of a rule that checks for literal only expressions. We used to have a rule called invariant_booleans I believe which tried to track invariants of variable expressions, like checking if (b && b) or if (b) if (!b) ... but it was too ambitious a rule, and had too many bugs for us to maintain.

I think we could probably allow checking const e = '' == '';. While I don't think it has high value, I think it would fit the theme of the rule.

[FMorschel]

with much of @FMorschel's help, if I remember

Just to be clear, (I don't want to take any credit for things I didn't do hahaha) I think you are confusing this with: no_literal_bool_comparisons and avoid_bool_literals_in_conditional_expressions (#60614). I've never looked at the specific lint in question here 😅

From the git history I could find: you two, Paul Berry, Phil Quitslund, Parker Lougheed and Bill Hesse have worked on this up until now.

[srawlins]

Haha thanks @FMorschel ; I was confusing that other rule.

[RohitSaily]

• Every first-party lint rule is code that the Dash team must maintain; when new language features come out (like patterns, dot-shorthands) we must look at rules like this and add fixes and tests.
• Also the cost to the user running the lint rule. Each time you press a key in your IDE, and this lint rule is enabled, we must compute any lints. Adding constant evaluation could be expensive. Checking on every expression vs just conditions like if-conditions adds (maybe just a tiny bit) of expense.
• And there is also the cognitive load of a team knowing all of the lint rules that are enabled. That doesn't play much of a role when making a rule stricter. But it does come into play when adding a new rule, comparing_an_uppercase_string_with_a_lower_case_string.

I think that checking constant values of all expressions (in conditions) is beyond the scope of a rule that checks for literal only expressions. We used to have a rule called invariant_booleans I believe which tried to track invariants of variable expressions, like checking if (b && b) or if (b) if (!b) ... but it was too ambitious a rule, and had too many bugs for us to maintain.

Thanks @srawlins, your reply helps clarify how I should approach this.

To keep what seems to be a good balance of utility, performance, and maintainability - I will stick to only checking conditions of control flow constructs. I will also include the condition of a ternary expression in these checks since that is effectively a short-hand if-statement used to choose a value. I also noticed is and as operations are not checked and will include them. For example, 1 is String && ('' as int == 'weird check').

---

The last things I would like to decide is whether or not (1) it is worth checking const instance creations eg const SomeObject() in addition to literals (2) checking if an operand is an identifier declared as const (then treating it like the literals are).

I would avoid performing constant evaluations - in case it is expensive - and simply check for the const creation or identifier usage.

Thoughts (anyone)?

[srawlins]

Since this rule is named with the word 'literal', and it might be expensive to calculate const values, I don't think we should check const expressions in this rule. It could be an analyzer plugin though.

[bwilkerson]

Since this rule is named with the word 'literal' ... I don't think we should check const expressions in this rule.

I agree. As Sam mentioned, we've tried having more complex rules in the past, but it's too hard to get them right without support for a general symbolic execution mechanism (which we don't have). Checking for literals is a lexical operation, which is inexpensive; checking for constant values is probably too expensive in the general case.

It could be an analyzer plugin though.

Putting it in a plugin does remove the first cost (that the Dash team has to maintain the code), but it doesn't change any of the other arguments. I wouldn't recommend putting a more general version of this rule in a plugin because of all the same costs as discussed above.

[RohitSaily]

I will keep it focused on literals.

For the sake of exploration - regarding the nonliteral constant checking - isn't it sufficient to check if the const keyword is applied on a constructor invocation or if an identifier is used directly that is declared as const? We wouldn't have to ever compute the value then.

[bwilkerson]

... isn't it sufficient to ... ?

I don't remember, off hand, what all of the corner cases were, but the invariant_booleans rule ran into lots of them. It's possible that it was trying to do way more than you're proposing, though, so your proposal might be sufficient.