NoConfusingPrefixOperator

[lydell]

What the rule should do:

Report for example (<) 0, such as in List.filter ((<) 0). My brain automatically reads that as “keep all negative numbers”, but it actually means List.filter (\x -> 0 < x) which does the opposite – keeping all positive numbers.

Example of things the rule would report:

(<) 0

Example of things the rule would not report:

(==) 0

More comprehensive thoughts:

The following operators are OK to partially apply, because the order of the operands does not matter:

(+) (*) (==) (/=)

The following are not OK:

(-) (/) (//) (^) (<) (>) (<=) (>=) (++) (|>) (<|) (>>) (<<)
(|.) (|=) (</>) (<?>)

I think these are technically OK, but it’s pretty weird partially applying them?

(&&) (||)

Trivia:

In Haskell you can also do (<) 0 and it means the same thing as in Elm. But Haskell has additional syntax:

• (0 <) means \x -> 0 < x (just like (<) 0)
• (< 0) means \x -> x < 0

When (not) to enable this rule:

I think this rule always makes sense!

I am looking for:

Is this a good fit for https://github.com/jfmengels/review-simplification? That package already has NoFullyAppliedPrefixOperator which is a bit similar.

I’d love to implement this myself, but I already have lots of other fun open source stuff to do so I might not get around to it.

[jfmengels]

Would this report (<) used on its own, or only if it is passed at least one argument?

As you said, I think the rule makes sense for non-commutative operators (i.e. where a <operator> b does not equal b <operator> a). I think it's okay not to report (&&) because they are commutative and therefore not really confusing IMO. FYI, it can be used like this List.foldl (&&) True listOfBooleans (which is equal to List.all).

Is this a good fit for jfmengels/review-simplification?

As I mentioned (recently) in the repository, I don't intend to maintain that package myself. But I think it makes sense for someone to do it.

I was originally thinking that it would be a repository for simplifying code (like what is proposed in #52), not for simplifying the readability. But that might work too.

I think that people who will want to enable NoFullyAppliedPrefixOperator might likely also want to enable this rule, so maybe it makes sense to merge them together into a NoConfusingOperator rule? Since (<) 0 1 is confusing too, the name of the rule does make sense I think.

In that sense I think it also makes sense to report (<) without arguments, since that makes people (me at least) rack their brain to figure what it will do.

[jfmengels]

I feel like this rule could be grouped in the same package as @lxierita's https://github.com/lxierita/no-typealias-constructor's rule, which forbids creating a value with a type alias constructor type alias Position = { x : Int, y : Int } ; pos = Position 0 1.

[xie-lang]

it does look relevant. i'll try it out sometimes this week too

[xie-lang]

sorry, just saw that it's been picked up

[infiniteregrets]

Hey! Just saw your tweet and I think I might be able to work on it. But I might need some help.
So from the Elm syntax package, are we looking at https://package.elm-lang.org/packages/stil4m/elm-syntax/latest/Elm-Syntax-Expression to check if an expression is a lambda expression? And then I am not sure, what would be the way out. I really do need to sit down and read about compiler design I guess ):
Let me know if you have any hints!

[jfmengels]

Hey @infiniteregrets 👋

We want to report the usage of some of the operators used as function. So as you said, we'll (only) be looking at expressions, and in particular the PrefixOperator variant.

I suggest doing the following:

• Clone the jfmengels/elm-review-common project
• Use elm-review new-rule to create a new rule (I think NoConfusingPrefixOperator is a good rule name, as originally suggested)
• Create failing tests in the generated test file
• Add the expression visitor and report errors. I suggest the read the explanation of https://package.elm-lang.org/packages/jfmengels/elm-review/latest/Review-Rule#withSimpleExpressionVisitor which you'll likely have to use (and I suggest also the documentation at the top of that page)

Then when you're done with that, you can think about the documentation and helpful error messages/Details to show the user of the rule.

Let me know if you need more help 😊

[infiniteregrets]

Thank you so much for the explanation! Looks like a fun thing to work on. I think I will be able to take a look at this next weekend (: I'll drop a message here if I am stuck. If someone else wants to try this out, then they really should!

[Janiczek]

Hello! I've been prompted by @jfmengels to weigh in here. I would love this rule to exist 👍 as I've even written down some thoughts about it in the past:

• a blogpost way from 2017: http://martin.janiczek.cz/clanek/being-clever-antipattern-in-elm/
• a bit more explained Twitter thread from just today: https://twitter.com/janiczek/status/1435895064438394886

They both boil down to

⚠️ Don't use non-commutative monoidal operators/fns with partial application.

This is about as succint as I can make it.

Interestingly that coding style has some support from The Man Himself™️: https://twitter.com/evancz/status/856611881728958464 if you want to consider idiomaticness.

[jfmengels]

I'm wondering whether we should make this rule also handle functions as well, n which case the name of the rule should change, and ideally not something that mentions "commutative" (I don't think people will try to enable that rule, it's not a sexy word 😅 ).

I'm in favor, but probably only for the functions defined in elm/core? Or should the rule also allow for configuration so that you can add more functions to report? It sounds useful but also a bit overkill maybe?

[Janiczek]

Yeah I didn't expect the rule name would change to NoNoncommutativeMonoidalPrefixOperator 😂 the terminology is a bit too much. I actually like the NoConfusingPrefixOperator name.

I don't have a clear opinion on the rule handling also functions vs just the operators. Maybe splitting the rule into two might make sense? I mean, there is only a limited number of operators, so the operator rule could know them all and not need configuration. And the function rule (if it existed at all) could have some nice defaults for elm/* but would then allow for configuration to add more?

config : List Rule
config =
    [ NoConfusingPrefixOperator.rule
    , NoConfusingPartiallyAppliedFunction.rule
        [ "Set.diff"
        , "Set.Any.diff"
        , "AssocSet.diff"
        ]
    ]

[Janiczek]

With type inference the rule could even automatically flag such functions without a blacklist 🤯

[jfmengels]

I don't think so. How would type inference help differentiate between Set.union and Set.diff?

[Janiczek]

Oh, right. I totally glossed over the commutativity aspect, got so excited about the a -> a -> a inference. You're right.

[lydell]

It could also be worth discussing if we should ban partial application of all operators. For example, this might look good at first:

userIds |> List.filter ((/=) selectedUserId)

But then gets refactored to:

users |> List.filter (.id >> (/=) selectedUserId)

Both of them look good with lambdas:

userIds |> List.filter (\id -> id /= selectedUserId)

users |> List.filter (\user -> user.id /= selectedUserId)

Also, bonus point if one could get autofixes to the above, rather than:

users |> List.filter (.id >> (\id -> id /= selectedUserId))

---

Note: (.id >> (/=) selectedUserId) reads surprisingly good in English: “where dot id isn’t the selected user id”. But it’s also a bit of a syntax soup.

[Janiczek]

It's a bit of an acquired taste. Back in 2017 (that was before I worked with Elm in my actual dayjob, with actual other people 😱 ) as I wrote the article I felt very very strongly about always using (\id -> id /= selectedUserId) and never ((/=) selectedUserId).

Now, during these four years, I kinda grew to like it, in some cases. I still think the lambda way wouldn't hurt at all even with the "safe" commutative operators, but sometimes I find myself using (.id >> (/=) selectedUserId) etc. It probably depends on team coding conventions.

If this rule banned all operators, would it prevent teams that are comfortable with ((/=) selectedUserId) from using the rule? They might still want to ban the unsafe operators...

[jfmengels]

I don't have any problems with commutative operations or operators, so I think the example is just fine userIds |> List.filter ((/=) selectedUserId). I don't feel like this should be part of the rule, and that that could be a different rule for people who don't like this style.

[lydell]

@jfmengels What do you think about .id >> (/=) selectedUserId?

[jfmengels]

It's a bit ugly but I (personally) read it fluently, and don't find it necessary to report it.

If we did change it, what about .id >> (/=) (someFunction value)? It doesn't read all that nice, that's for sure, but if we change it to \value -> value.id /= someFunction value then potentially if this function is used multiple times like in a List.map call, then we have duplicated the number of times someFunction value will be computed, which is one of the reasons why we choose to use partial application.

I mean, we could then decide to special-case when the argument to the operator is a function call, but we'd have to explore when to stop?

By the way, finding a good variable name and that does not introduce shadowing errors in a fix are both non-trivial tasks.

[lydell]

Good points!

[jfmengels]

@Janiczek just complained about this again on Twitter. Let's get this rule in elm-review-common, which I now think is the most appropriate place?

[Janiczek]

😂 Glad to be of service!

[jfmengels]

Released as part of elm-review-common: https://package.elm-lang.org/packages/jfmengels/elm-review-common/latest/NoConfusingPrefixOperator

(This only includes the part that was about the operators, not the functions that can be confusing like Set.diff)

[Janiczek]

Thank you @jfmengels !