Enumerate all variants in list

[MartinSStewart]

What the rule should do:
This rule checks that list literals the user has chosen, contain every possible variant of a chosen type.

What problems does it solve:
Those times where you add a new variant to a type and forget to update a list that should contain every variant

Example of things the rule would report:

Example 1:

type IceCreamFlavor = Vanilla | Chocolate | Strawberry

iceCreamFlavors : List IceCreamFlavor
iceCreamFlavors =
    [ Vanilla
    , Chocolate
    ]

gets fixed to this (assuming the user has configured the rule to check iceCreamFlavors)

type IceCreamFlavor = Vanilla | Chocolate | Strawberry

iceCreamFlavors : List IceCreamFlavor
iceCreamFlavors =
    [ Vanilla
    , Chocolate
    , Strawberry
    ]

Example 2:

type IceCreamFlavor = Vanilla | Chocolate | Strawberry | Custom String

iceCreamFlavors : List IceCreamFlavor
iceCreamFlavors =
    [ Vanilla
    , Chocolate
    , Strawberry
    ]

This would cause a configuration error because Custom can't be enumerated in a list (assuming the rule was configured to check iceCreamFlavors).

Example 3:

type IceCreamFlavor = Vanilla | Chocolate | Strawberry

This would cause a configuration error because iceCreamFlavor is missing (again, assuming that the rule is configured to check for it).

When (not) to enable this rule:

You don't have lists that need to enumerate a type

I am looking for:

• Would you use this rule?
• An alternative to having rule configuration for determining which functions should be check is to have some naming convention. For example, checking top level functions that start with all and have a type annotation of List <type name>. I personally don't think this is the way to go since things might silently break if the function gets renamed.

[lydell]

What if you have LegacyBubblegum in there that you need to support but don’t want to appear in a list of selectable flavors?

[MartinSStewart]

Something like this could work I suppose

bubbleGumFlavorsInternal = [ ... ] -- This is the one the rule checks

bubbleGumFlavors = bubbleGumFlavorsInternal |> List.filter ((/=) LegacyBubblegum)

[lydell]

Ah, I see. I was thinking that LegacyBubblegum was a previous IceCreamFlavor, but I get your point.

[jfmengels]

Would you use this rule?

Yes! We actually have this rule set up at my workplace.
I wrote a gist containing it https://gist.github.com/jfmengels/d143009a984ced40dffffaa7a8d70089
and I wrote about it in this Discourse reply (https://discourse.elm-lang.org/t/covering-all-constructors/5310/2) (the gist linked there is for the v1 API)

I wrote it when v1 was the latest version, so it is limited to the knowledge of the current module. This should be turned into a project rule before it is published somewhere.

Also, I didn't account for example 2, and at the moment it would just error as if the variant with an argument was missing from the list (which is not ideal)

An alternative to having rule configuration for determining which functions should be check is to have some naming convention. For example, checking top level functions that start with all and have a type annotation of List . I personally don't think this is the way to go since things might silently break if the function gets renamed.

That is actually what I went with, but I agree that it is brittle and that bugged me. I hadn't thought of having the list in the configuration, that sounds like a more stable solution. I wonder whether there is worth in having both the allXyz solution and the configuration, but I feel like configuration-only would be the way to go.

I imagine that the configuration would look somewhat like this?

RuleName.rule [ "Some.Module.Name.iceCreamFlavors" ]

If so, what you should probably do then is to report when that function could not be found in the project (or if does not look like a list of enum), like I described in here (under "Making sure the target function exists").

[MartinSStewart]

Yeah, something like RuleName.rule [ "Some.Module.Name.iceCreamFlavors" ]. For a different rule I've written it as RuleName.rule [ (["Some", "Module", "Name"], "iceCreamFlavors") ] to reduce the number of invalid ways you could write the function path. Maybe it's not worth the extra syntax though?

Also I had a look at how project/configuration errors are reported in the link you provided.

Rule.errorForElmJson
                    elmJsonKey
                    (\_ ->
                        { message = "Could not find Helpers.Regex.fromLiteral."
                        , details =
                            [ "I want to provide guarantees on the use of this function, but I can't find it. It is likely that it was renamed, which prevents me from giving you these guarantees."
                            , "You should rename it back or update this rule to the new name. If you do not use the function anymore, remove the rule."
                            ]
                        -- Dummy location. Not recommended though :(
                        , range =
                            { start = { row = 1, column = 1 }
                            , end = { row = 1, column = 2 }
                            }
                        }

In this code there's this comment, -- Dummy location. Not recommended though :(. What is recommended then?

[sparksp]

Yeah, something like RuleName.rule [ "Some.Module.Name.iceCreamFlavors" ]. For a different rule I've written it as RuleName.rule [ (["Some", "Module", "Name"], "iceCreamFlavors") ] to reduce the number of invalid ways you could write the function path. Maybe it's not worth the extra syntax though?

I've got a couple of rules that take module names as config and have discussed String vs List String with @jfmengels a few times. We settled on Strings as these are more like what users would understand so they need less documentation to explain, and it also means that ( ["Some.Module", "Name"], "allFlavors" ) isn't going to trip you up.

[jfmengels]

I've written it as RuleName.rule [ (["Some", "Module", "Name"], "iceCreamFlavors") ] to reduce the number of invalid ways you could write the function path.

I feel like that can make it harder for a user to understand, especially when they don't know that elm-syntax uses a List for module names (and I don't think they should have to know). I think there are enough other ways that this configuration can go wrong (especially if it needs to be split, because now you can't copy/paste it), that you need to validate the configuration or ignore the configuration anyway.
I think [ ("Some.Module.Name", "iceCreamFlavors") ] is fine though, as users are more likely to understand why the module name and the function name are split. (like @sparksp just mentioned)

In this code there's this comment, -- Dummy location. Not recommended though :(. What is recommended then?

The recommended way would be to throw some kind of global/configuration error, which elm-review doesn't support at the moment. One of the reasons why I haven't added it yet is because I heard that tooling like the Elm language-server-protocol need to have a file associated to an error, so a global error not-related to a file doesn't make much sense. There are global others I throw in elm-review, for which I end up assigning elm.json ("good enough" solution probably), so this shouldn't be an issue.

That said, there are other concerns around creating global/configuration errors. I'll try and create an issue about that (soon-ish) in the elm-review repo, where we can talk about this some more (and I'll link it here)

[jfmengels]

Update:

1. global and configuration errors will be available in the next elm-review version
2. I have a more up-to-date version of the rule available here https://gist.github.com/jfmengels/e1fd40af3d0e7bde707c0241bf46920f

@MartinSStewart I think you had more comments about this rule on Slack but I forgot what they were :s

[MartinSStewart]

Oops, forgot to respond but I guess it's not too bad that I forgot because I don't remember what those comments were either.

[jfmengels]

@Arkham published this under https://package.elm-lang.org/packages/Arkham/elm-review-no-missing-type-constructor/latest/
and wrote about it in https://juliu.is/there-and-back-again/