stil4m/elm-syntax

Idea: Rename Expression.FunctionOrValue and/or split it

Opened this issue · 3 comments

We currently have a Expression.FunctionOrValue node, and the name is both long and a bit confusing. It is also not completely true, because the node is not a function, but a reference to a function or to a variable/argument/... .

I propose to rename this node. I was going to suggest Reference but that is somewhat confusing still. ESLint uses the term Identifier which I think fits quite well.

Maybe we also want to split this into 2 different nodes: one for variables/argument (add) and one for type constructors (True, SomeRecordAlias). I have found cases where this was important to the rule and I had to differentiate between the two (which I did by checking whether the first character was capitalized).

Any opinions on this?

I agree FunctionOrValue isn't a great name. But one advantage it has over Identifier is that it's clear it's not talking about type definition identifiers. Maybe that's sufficiently obvious to someone who knows you can't reference a type in an expression but I think making it extra clear is a good thing.

I agree distinguishing between ordinary functions and type constructors would be nice. What about Function and TypeConstructor?

Edit: On second though I don't like those names either since they don't make it clear it's a reference. FunctionIdentifier and TypeConstructorIdentifier maybe? Though TypeConstructorIdentifier is kind of long.

It looks like in the TypeScript AST the A in type A = ... is also an Identifier node.

I don't think we need to cater too much to people who don't know Elm that well, especially as I think it's quite clear that Expression.Identifier would not refer to the identifier in a type declaration. Also, if they're using in a context like elm-review, it's unlikely that they will be expecting to find a type declaration identifier while visiting expressions.

I don't like having Function in the name because it doesn't always reference a function (a when a = 1 for instance), and that can be confusing. Also, a type constructor identifier can also be a function.

We could name references to a ValueIdentifier, but again, even functions are technically values (at least in my definition 🤔 ).

Another solution is to only have Identifier and have a function that indicates whether the name would be a "type identifier" or not (so that my rules don't have to re-implement that logic in a way that could be flawed compared to the parser). I'm not sure where this function would go and how we would prevent it from running on strings that don't know from the Identifier node though.

Throwing in one more idea: Expression.Variable for values and functions
- maybe unclear it includes module-level things

Splitting off a new variant for record type alias and type variant constructors might be overkill since many will mostly match both variables and constructors as one case.