Suggestion: directly replace all expressions by their deduced bool

[lue-bird]

original motivation

While converting all lists of errors to maybes in this branch, I found the following simplification which reports 2 errors that have the same error range, same message and details, only a different fix:

if a || b then
    1
else if a && b then
    2
else
    3
-->
if a || b then
    1
else if a then
    2
else
    3
-- also provided but not shown in the CLI
-->
if a || b then
    1
else if b then
    2
else
    3

My intuition says the more "correct"/obvious fix here is:

1. replace all a in the else branch with False as inferred
2. replace all b in the else branch with False as inferred
3. now we have False && False which can be evaluated to False
4. Now everything except its else branch there can be removed

questions

First: Do you agree that the suggested (replacing expressions with deduced bools) is nicer?

And second: should we do the same for strings and numbers, or do you think there are cases where one might not want this, like

if fn == "Html.Attributes.classList" then
    error { message = "Using " ++ fn ++ " with an empty list is the same as not using " ++ fn }
else
    ...
-->
if fn == "Html.Attributes.classList" then
    error { message = "Using " ++  "Html.Attributes.classList" ++ " with an empty list is the same as not using " ++  "Html.Attributes.classList" }
else
    ...

[jfmengels]

First: Do you agree that the suggested (replacing expressions with deduced bools) is nicer?

I'm not sure. I agree that it's clearer to the user that replacing if a && b then by if False && b then is clearer, especially with the not-so-descriptive error message that comes with the error. But if we improve the error message, I don't see a reason why we can't directly remove the condition like we do today (if b then).

I don't care too much about the order in this case nor about the fact that we report 2 errors, because in practice we will arrive to the same result when we apply them all. I think replacing them by False (which I think we do in some cases still) when we know we can directly do better is just adding an additional step in fixes, which slows users down more.

should we do the same for strings and numbers

I've thought about this one too, both as a rule but also simply when I'm myself writing code like the above. I would say we probably shouldn't do it, because it can mess with how maintainable a piece of code is.

Taking you example, if we continue applying fixes, we'll end up with

if fn == "Html.Attributes.classList" then
    error { message = "Using Html.Attributes.classList with an empty list is the same as not using Html.Attributes.classList" }
else

Now let's say the function name changes to something else (Attr.classList for instance). Instead of changing one location, you need to change 3. I don't think this change would be wrong per se, but I think it could make some code harder to edit or maintain.

It could lead to more simplifications though, but I think the potential pain point is likely bigger. That's my gut feeling at least.

[lue-bird]

Very interesting. Yeah, it might be somewhat doable to list why we know that something can be deduced as a specific value when explaining why we can apply a certain fix. As a user, this all might seem a bit magical and possibly hard to follow, precisely because there are multiple steps involved at once that we, instead of explaining in atomic errors applied one after the other, have in one bulk. To me it seems kind of useful to make users understand every simplification step separately (tiny safe refactors like in the talk you shared) because deducing from branching stuff can quickly get overwhelming.

And as you pointed out, we could change our existing code to always use infer instead of casing on the expressions for example but then we have to basically explain the deduction origin in all these errors.

[lue-bird]

Maybe another point to convince you to replace deduced bool literals at least: not (inferred as bool) is currently replaced by the opposite bool literal.

[jfmengels]

Not sure what you mean :thinking:

[lue-bird]

I was trying to say this simplification already exists

if x then
    not x
else
    ...
-->
if x then
    False
else
    ...

Maybe that's the line you draw but seems very thin

[jfmengels]

It is a pretty thin line yes, trying to balance between simplifying code and not preventing users to write code the way they want it, in what is the easiest to maintain.

In the example for not x, you could in the future change if x then to if True then or if False then and the simplification would still apply (well as much as if False then False else ... makes sense.

I'm not saying I will always be right in the decision making process for this, but I'd rather stay on the conservative side. In this case, I feel like if I wanted to maintain

if fn == "Html.Attributes.classList" then
    error { message = "Using " ++ fn ++ " with an empty list is the same as not using " ++ fn }
else

then I wouldn't want it to be rewritten to

if fn == "Html.Attributes.classList" then
    error { message = "Using Html.Attributes.classList with an empty list is the same as not using Html.Attributes.classList" }
else

because it doesn't make my job as the maintainer of the code simpler (because if the value that is compared to fn changes ). Say the compared value was a multiline string that was 20 lines from a book, then I'd definitely wouldn't want to inline it. It wouldn't be great for maintainability nor for bundle size.

With not x, I discover a bit more about how my code behaves when it gets simplified, and it's not replaced by a large piece of code that I then need to maintain (just to True or False).

[lue-bird]

Took me a while to get behind the benefits of leaving variables in places where you know their actual value but I think I now agree for string/(char/)number. For bools, my original stance still holds. But if you have decided the current trade-off between convenience and clarity for the user is the right one, this is just a subjective disagreement.