jfmengels / elm-review-simplify

Provides elm-review rules to simplify your Elm code
https://package.elm-lang.org/packages/jfmengels/elm-review-simplify/latest/
BSD 3-Clause "New" or "Revised" License
20 stars 9 forks source link

List.member on Dict.keys/Set.toList -> Dict/Set.member #290

Open morteako opened 4 months ago

morteako commented 4 months ago

What the rule should do: Replace List.member on Set.toList/Dict.keys with Set/Dict.member

What problems does it solve: Simplifying and optimizes unnecessary convertions

Example of things the rule would report:

List.member c (Dict.keys d)
--> Dict.member c d
List.member c (Set.toList d)
--> Set.member c d

Example of things the rule would not report: Since Set/Dict.member requires comparable, but List.member does not, applying this replacement to Set/Dict.empty, could lead to a type error. So therefore we should/could not replace it in that case. (But it should be caught by some other simplification rule)

List.member c (Dict.keys Dict.empty)

The above should be simplified to False anyways, by the callOnEmptyReturnsCheck

Should this be part of the Simplify rule or should it be a new rule?

Part of Simplify

I am looking for:

lue-bird commented 4 months ago

Nice. So basically we can only apply these if the set/dict is constructed right there (fromList, insert, singleton) and is non-empty.

(And I think you mean List.member c (Dict.toList Dict.empty) instead of List.member c (Dict.member Dict.empty) in the "not reported" example)

morteako commented 4 months ago

Ah, yes. That seems (unfortunately) correct.

Those restrictions seems to make this rule pretty niche, so I don't think it is worth to implement it probably. I think it would be a good rule in the general case, but having the possibility to create type errors is of course really bad.

jfmengels commented 4 months ago

Sounds like a good idea :+1: Thank you both for your input :relaxed:

I wonder if there are other conversations (for instance List.isEmpty (Dict.toList dict)) that could be done with the base type's functions, and that we don't already support.

morteako commented 4 months ago

@jfmengels

I think these are not supported.

.singleton

Dict.fromList (List.singleton (k,v)) ---> Dict.singleton k v

List.length ✅ https://github.com/jfmengels/elm-review-simplify/pull/293

List.length (Set.toList set) ---> Set.size set

-- also for Dict.keys and Dict.values List.length (Dict.toList dict) ---> Dict.size dict

List.isEmpty ✅ https://github.com/jfmengels/elm-review-simplify/pull/293

-- also for Dict.keys and Dict.values List.isEmpty (Dict.toList dict) ---> Dict.isEmpty dict

List.isEmpty (Set.toList set) ---> Set.isEmpty set

List.member (both of these have the same problems as the one above :()

List.member k (Dict.keys dict) ---> Dict.member k dict

List.member k (Set.toList set) ---> Set.member k set