Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FunctionTypeAnnotations should not require names for parameters, but do today #2208

Closed
jakemac53 opened this issue Apr 19, 2022 · 6 comments
Closed
Assignees
Labels
static-metaprogramming Issues related to static metaprogramming

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented Apr 19, 2022

Today the ParameterDeclaration class requires a name (it extends from Declaration). This type is then used in the FunctionTypeAnnotation class to represent the parameters, but in that context they don't require a name.

The name is also an Identifier, which probably doesn't make sense in the context of a function type either, it isn't anything you can reference afaik.

Proposal

Make a new class Parameter which does not extend from Declaration, and has all the fields that ParameterDeclaration has. The ParameterDeclaration class would now implement both Declaration and Parameter. And we would add an additional class FunctionTypeParameter which would implement Parameter, and also have a String? get name field, instead of the Identifier get name field on Declaration.

cc @scheglov @johnniwinther

@jakemac53 jakemac53 added the static-metaprogramming Issues related to static metaprogramming label Apr 19, 2022
@scheglov
Copy link
Contributor

Another alternative design could be to have separate FunctionTypePositionalParameter with String? get name and FunctionTypeNamedParameter with String get name. And then we would not need isNamed. Currently we imply that isNamed means also that get name return non-nullable String. But with types we could prove it.

@jakemac53
Copy link
Contributor Author

True, I worry a bit that it doesn't map very directly to ParameterDeclaration which does not have that distinction. But I could also create NamedParameterDeclaration and drop isNamed from that api?

@scheglov
Copy link
Contributor

Yes, in my opinion NamedParameterDeclaration also would be nice, and consistency as well.

In the analyzer it might be slightly less convenient though, because we have another layer of options - default parameters, field formal parameters, super formal parameters, all of which can be [required, optional] * [positional, named].

@johnniwinther
Copy link
Member

I would go for the original proposal.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Apr 21, 2022

I generally like having a simpler class hierarchy, so I think it will just opt for the original proposal. It isn't as ideal for named parameters in function types, but I think that is ok.

@scheglov
Copy link
Contributor

Sounds good.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Apr 22, 2022
Bug: dart-lang/language#2208
Change-Id: I2a9bed6cc243eef2bfa313f7280d1d60c51a7b36
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/242140
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Jake Macdonald <jakemac@google.com>
@jakemac53 jakemac53 self-assigned this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
static-metaprogramming Issues related to static metaprogramming
Projects
Development

No branches or pull requests

3 participants