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

Should the grammar allow ? in an old-style function-typed formal parameter declaration? #364

Closed
stereotype441 opened this issue May 20, 2019 · 14 comments
Assignees
Labels
nnbd NNBD related issues

Comments

@stereotype441
Copy link
Member

In the review of #293, @lrhn pointed out that:

There is no easy way to make a <functionFormalParameter> nullable. I suggest:

<functionFormalParameter> ::= \gnewline{}
    <metadata> \COVARIANT{}? <type>? <identifier> <formalParameterPart> `?'?

The final optional ? would make the function type of the parameter into a nullable function type, so:

  E firstWhere(bool test(E element), {E orElse()?});

would be valid.

When trying to convert dart:convert and dart:collection, I had a lot of optional function parameters which need to be nullable.

I'm filing this issue to track the decision about whether we want to make this change to the grammar, so that if we do, the analyzer team can find out about it and start work.

@munificent
Copy link
Member

Lasse's suggestion is what I had in mind with the initial NNBD prototype back in the day. It's a little weird but, well, the function-typed parameter syntax is weird.

@leafpetersen
Copy link
Member

I was hoping to avoid this - I think it's really hard to read. My thinking was that we would just push people to the new function type syntax, and the migration tool would just make this easier. But maybe this is too much pain, particular for manual porting of APIs. @lrhn is that your takeaway from your experiments?

@lrhn
Copy link
Member

lrhn commented May 23, 2019

As a migrator, I would very much like to not have to rewrite my parameters.
Migration is easier if you just add ? and ! where necessary, but any place you have to rewrite code, the work increases significantly. I know this can be automated, but some code will be migrated manually.

I found the int foo(args, args)? syntax readable enough, and it's consistent with int Function()? foo in that you always put the question mark after the end parenthesis (and inconsistent in that it doesn't come just before the name, but function parameters were never consistent on the placement of the name).

I also personally prefer int foo() for a parameter over int Function() foo. There are probably dozens of us! I'd be very annoyed if I am pushed to use the less preferred form in order to get NNBD. Either support the old syntax fully, or remove it, a half-hearted support is just an insult.

@leafpetersen
Copy link
Member

OK, I'm basically fine with this. I'll add the change to the PR in my next pass.

@leafpetersen leafpetersen self-assigned this May 23, 2019
@natebosch
Copy link
Member

Either support the old syntax fully, or remove it, a half-hearted support is just an insult.

Omitting support for new features is a good way to urge migration without a breaking change...
Since we already are handing breaking change migrations here though we could consider removing the old syntax so that there is only 1 way to do things.

@munificent
Copy link
Member

I know this can be automated, but some code will be migrated manually.

I'd be happy to add an automated --fix to dartfmt that turns the old syntax into the new.

@bwilkerson
Copy link
Member

There is already an assist in server (and has been since the new syntax was introduced), so we could also easily add it to dartfix. @danrubel

@a14n
Copy link

a14n commented May 24, 2019

And there's also the lint use_function_type_syntax_for_parameter.

@lrhn
Copy link
Member

lrhn commented May 24, 2019

If I actually wanted to move people to the function-type syntax, then yes, this would be a way to "encourage" that move.

But I don't. I actually prefer the "old" syntax for function typed parameters. I find it more readable.
So I, personally, have no wish to deprecate or under-support it. I may be completely alone in this, but I wouldn't bet on it.

@bwilkerson
Copy link
Member

I don't dislike the old syntax, but I agree with Nate that the language would be better if there were only one syntax for expressing a function type, even if that syntax is not ideal in all cases.

@natebosch
Copy link
Member

FWIW I likely would prefer the old syntax if there wasn't such a sharp edge around mistakenly adding implicitly dynamic argument names with the intention of specifying types... New Dart users keep hitting this - I'm not sure the frequency but this comes up in email lists every so often as a point of confusion.

As a general rule I also prefer if every author didn't need to make a nearly arbitrary choice between two syntaxes that achieve the same goal.

@matanlurey
Copy link
Contributor

I'm strongly for deprecating and then removing the old function literal syntax (or changing tools like dartfmt --fix to automatically replace it). Accidentally creating an incorrect function signature is way worse than typing a few characters - in fact I imagine its not too late to change the keyword if we wanted to, for example:

typedef Formatter<T> = String func(T);

@lrhn:

I actually prefer the "old" syntax for function typed parameters. I find it more readable.

I know this isn't surprising at this point, but one person's personal opinion doesn't make a language :-/. I imagine at one point you strongly preferred dynamic/optional types, reflection over code generation, etc, but the language has to evolve to meet its users.

@munificent
Copy link
Member

I don't think @lrhn is saying that their opinion should outweigh all others, just that he is an embodiment of at least one user who does like the old syntax.

Personally, I'm sympathetic because the new function type syntax isn't great either. Function is an awfully long keyword. :-/ Though, in general, I'd be happy to mandate always using the new syntax even though it is longer, just to keep things simpler and more consistent.

@leafpetersen
Copy link
Member

Ok, I added this to the specification. If we choose to deprecate and remove this syntax I think it should be a separate effort, and the meantime it should be a first class part of the language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nnbd NNBD related issues
Projects
None yet
Development

No branches or pull requests

8 participants