Make more elements non empty

[jfmengels]

• Expression.LetBlock declarations
• Expression.FunctionCall arguments
• Expression.FunctionImplementation arguments
• ModuleName (use Maybe ModuleName in things like FunctionOrValue)?
• More to go and figure out???

[lue-bird]

Maybe ModuleName sounds great. What I've always asked myself: Can't ModuleName just be a String / What benefits do we get by splitting the name?

Because if we decide to make ModuleName = String we can probably say "" for no qualification as well.

[MartinSStewart]

I'd rather not have ModuleName be a string because it makes it possible for the Writer module to generate Elm code that won't parse into a valid AST (yes this is already possible but I don't want to make it more possible).

I'm on board with having Maybe ModuleName though

[jfmengels]

@MartinSStewart do you mean if someone manually creates FunctionOrValue (Just "$$$") "name"? It feels like it's already very possible to break this, so I'm not sure that this will improve things.

Having ModuleName be a string will probably be much better for performance though, as List is a much more expensive piece of data to create and to (de-)serialize (caching for encoded elm-syntax and for internal elm-review cache).

---

A downside is that it will be easier to confuse it with other strings, as users will likely have functions like:

someFn : ModuleName -> String -> X
someFn moduleName name = ...
-->
someFn : String -> String -> X
someFn moduleName name = ...

and now this is easier to mess up. We could keep an alias (type alias ModuleName = String) but it won't be more type-safe. We could instead do an custom type with exposed variants type ModuleName = ModuleName String but that might be (too?) painful for pattern matching.

[MartinSStewart]

Hmm, hadn't considered the performance angle. Do you know how much of a different it makes using List String vs String?

[jfmengels]

I don't know the details. But let's say we have a Maybe ModuleName, then in JS that would be (I could get some details wrong but this should be about right):

// Nothing
{ $: 0 }
// Just "Module.Name" using a String
{ $: 1, a: "Module.Name" } // 1 object
// Just [ "Module", "Name" ] using a List
{ $: 1, a: { $: 1, a: "Module", b: { $1, a: "Name", b: { $: 0 } } // 4 objects

In the elm-syntax encoder/decoder, we could probably say Nothing translates to "" to avoid an object.

For elm-review, we dump the JS memory, but because there are so many nested lists and records and what not, the data is huge (and IIRC can lead to stack overflows, not sure). So for performance, I had to actually use a replacer in JSON.stringify (and JSON.parse) to turn Elm Lists into JS Arrays.

Module names are not the longest lists so it's fine, but they are pretty much everywhere though 😅

[jfmengels]

I guess if we want to go for performance, we could skip the Maybe and just have it be "" when there isn't one like @lue-bird suggested. I don't know if the UX for that will be better or worse.

case value of
  -- No module name
  Expression.FunctionOrValue "" name -> x

  -- We don't know if there is a module name (unless we have the previous branch present).
  Expression.FunctionOrValue moduleName name ->
    if String.isEmpty moduleName then
       x
    else
       y

Whereas with Maybe we could pattern matching this more easily:

case value of
  Expression.FunctionOrValue Nothing name ->
     x

  Expression.FunctionOrValue (Just moduleName) name ->
     y

[MartinSStewart]

Gotcha. I'm still in favor of Maybe ModuleName where type alias ModuleName = NonEmptyList String.

I think it's fine encoding and decoding converts it into a string but I think it's best to go with Maybe ModuleName for the user facing API since it's more self documenting (it's clear that a function can be unqualified) and it's less likely the user will accidentally generate invalid Elm code (since the . in the module name path are provided for you).

[lue-bird]

Random edge case: The type List has no module origin, so [] or "" should be the "correct" full qualification. I guess you could say NonEmptyList.cons "" [] is the "right" qualification then but idk.

I feel like the best we have is Maybe String for optional qualification, so Nothing for e.g. a module-local List variant and Just "" for the implicitly imported List type. Frankly I feel like we should just not introduce the primitive type alias to make it clear users should use fields for passing these around:

someFn : Elm.Syntax.ModuleName.ModuleName -> String -> X
someFn moduleName name = ...
-->
someFn : { moduleName : String, name : String } -> X
someFn fullyQualifiedReference = ...