Overlap between `FormalParameterList.leftDelimiter` and `FormalParameterList.parameters`

[stereotype441]

A nice property of nearly all analyzer AstNode types is that the set of Token, AstNode, and NodeList objects that are pointed to by the AstNode always covers non-overlapping ranges of tokens.

However, FormalParameterList is an exception, because FormalParameterList.parameters is a NodeList of all the parameters (including required positional, optional positional, and named parameters), and FormalParameterList.leftDelimiter is the { or [ token that precedes the first optional positional or named parameter. Therefore, FormalParameterList.leftDelimiter, when present, is a token that is within the span of tokens pointed to by FormalParameterList.parameters.

An unfortunate consequence of this is that the lists returned by FormalParameterList.childEntities and FormalParameterListImpl.namedChildEntities behave oddly: they expand out the parameters node list into its constituent elements, and insert the leftDelimiter at the appropriate spot in the list.

This seems confusing and bug-prone to me. I think we should consider changing the FormalParameterList AST representation from this:

class FormalParameterList {
  Token get leftParenthesis;
  NodeList<FormalParameter> get parameters;
  Token? get leftDelimiter;
  Token? get rightDelimiter;
  Token get rightParenthesis;
}

To something more like this:

class FormalParameterList {
  Token get leftParenthesis;
  NodeList<FormalParameter> get requiredPositionalParameters;
  OptionalOrNamedFormalParameters? get optionalPositionalParameters;
  OptionalOrNamedFormalParameters? get namedParameters;
  Token get rightParenthesis;
}

class OptionalOrNamedFormalParameters {
  Token get leftDelimiter;
  NodeList<FormalParameter> get parameters;
  Token get rightDelimiter;
}

In addition to avoiding overlap between nodelists and tokens, this would have the advantage of more closely following the grammar of the language spec, and would be potentially more future-proof if we ever decide to allow a single function to have both optional positional and named parameters.

[bwilkerson]

I think that's a cleaner design.

It will probably have a fairly large impact on clients, but without knowing the actual impact I can't say whether the benefit will outweigh the cost.

It would be nice to have a better name than "OptionalOrNamedParameterList". I'm not sure it's better, but perhaps something along the line of "FormalParameterGroup"? (Making it less about the kind of parameters that are children of the node and more about the fact that they're a separate sub-list.)

[stereotype441]

I think that's a cleaner design.

It will probably have a fairly large impact on clients, but without knowing the actual impact I can't say whether the benefit will outweigh the cost.

It would be nice to have a better name than "OptionalOrNamedParameterList". I'm not sure it's better, but perhaps something along the line of "FormalParameterGroup"? (Making it less about the kind of parameters that are children of the node and more about the fact that they're a separate sub-list.)

Oh, haha, I wasn't even consistent in my proposal. In one place I called it OptionalOrNamedFormalParameters and in another place I called it OptionalOrNamedParameterList. I agree that the name isn't great (neither of them is). An advantage of OptionalOrNamedFormalParameters, though, is that it matches the ⟨optionalOrNamedFormalParameters⟩ BNF rule from the spec.

[bwilkerson]

... it matches the ⟨optionalOrNamedFormalParameters⟩ BNF rule from the spec.

True, but the grammar tends to change when the language changes (and this particular production would likely be removed if the language ever allows both optional positional and named parameters in the same signature). Tying the AST structure to the grammar has never been a goal, and I generally consider it to be an anti-pattern because of the impact on forward compatibility.

Almost every time there's a grammar rule that is a list of alternatives we create names for each branch rather than to use the name of the grammar rule in which the alternate is contained. For example, there is no ⟨methodDeclaration⟩ production in the grammar, it's an alternate in the ⟨classMemberDeclaration⟩ rule. While it's reasonable in this case to use a single class to represent the two branches (because the only difference is the kind of delimiters used), I still think that using the name of the grammar production as a class name is a bad idea. (Although it wouldn't be terrible for OptionalFormalParameters and NamedFormalParameters to be two separate classes.)