dart-lang/code_builder

Unify parameter handling on `Method` and `FunctionType`

natebosch opened this issue · 0 comments

See #353

Currently Method uses a Parameter class, and it separates parameters into few Lists. The Parameter class indicates if it is named, so optionalParameters must have either all named, or all unnamed parameters. Parameter.required is only sensible for named parameters - so requiredParameters will have 0 values with required == true, while optionalParameters can have values with required == true.

In FunctionTypes we don't allow argument names at all for positional parameters, so the optionalParameters here are only the optional positional parameters (type only, no name) while namedParameters have a name and a type, but we have to add a separate collection to track which named parameters are required.

class Method {
  /// Optional parameters.
  BuiltList<Parameter> get optionalParameters;

  /// Required parameters.
  BuiltList<Parameter> get requiredParameters;
}

class FunctionType {
  /// Required positional parameters of this function type.
  BuiltList<Reference> get requiredParameters;

  /// Optional positional parameters of this function type.
  BuiltList<Reference> get optionalParameters;

  /// Named optional parameters of this function type.
  BuiltMap<String, Reference> get namedParameters;

  /// Named required parameters of this function type.
  BuiltMap<String, Reference> get namedRequiredParameters;
}

I considered making both classes have a single BuiltList<Parameter> class and abusing Parameter.required to also have meaning (somewhat different to the current meaning since it doesn't indicate a required keyword, but very related). The biggest downside to this is that it would mean passing an extra ..required = true on nearly all usages because so many parameters are required positional.

@jakemac53 pointed out that there is some nice properties to having a separation between the different flavors of parameters because there isn't any risk of conflating them. We can incrementally migrate if we reuse the existing names though, and the single parameters list has the nice property of having a new name.

Another option we could consider is adding a ParameterList class. Each of Method.parameters and FunctionType.parameters could be added with this new type, and then within we could separate out the flavors of parameters if we wanted.

Another idea @jakemac53 had that I like is deprecating Parameter.covariant and Parameter.required, and instead having a Parameter.keywords which is a BuiltList<String>.