Initial cut at an NNBD specification

[leafpetersen]

My initial cut at a specification. I've tried to lay out a specific choice for each of the remaining decisions that we have to take.

[leafpetersen]

cc @lrhn @munificent @eernstg @mit-mit

[lrhn]

Not sure what this means. What does "succeed with a warning mean", and what is the result of the test?
Does it succeed without a warning if the test succeeds with non-changed types?

This sounds like we have runtime warnings and each test needs to do two type checks (although we can perhaps merge that into one check algorithm with a ternary result). It sounds expensive. For the warning to be useful, it must at minimum remember the source position of the test, which is an extra overhead on application size.

[leafpetersen]

Yes to all of the above, except that it's not clear to me why you need to remember more information for the warning than you do for the error that would otherwise be thrown.

[lrhn]

This can hold back migration.
Also, if an unmigrated library can export migrated "symbols", this might not even export any unmigrated types.

Maybe:
It is an error for a migrated library to re-export declarations with a static type containing unmigrated types.

[leafpetersen]

Do you really think this is likely? Migrating before your upstream packages migrate is already the less common use case. Exporting symbols is a fringe use case. Exporting symbols that were exported from yet another library seems like a fringe on a fringe case. It seems much simpler to me to say that we just don't support it.

[lrhn]

Why is it that we won't support it? That is, what is the problem we are trying to solve by disallowing the export?

Is it purely methodological problems: Library A is migrated, but exports declarations from unmigrated library B. Library C uses library A and wants to migrate, so they might need to migrate twice, once for A and once more when B is migrated. We don't want that, so we disallow A from exporting declarations from B until B has been migrated.

If so, I'm not sure how that differs from an unmigrated library exporting declarations of a migrated library. If library A exports declarations from library B and library B migrates first, then the user in library C, which imports library A, may need to migrate twice as well, once for the B changes, and once more for the A changes.

They can choose to not migrate library C until both A and B have been migrated, but that applies to both cases.

Or is there a technical problem? I can't see one because you can get into the exact same state by importing library A and B separately, rather than relying on the export of B by A, and we need to be able to handle that.

[leafpetersen]

It seems to me to add a lot of complication if migrated libraries can have subset of their APIs which are unmigrated.

[danrubel]

Question about syntax

[lrhn]

I think late will be fine as a built-in identifier. After all static works as a built-in identifier.

[leafpetersen]

@bwilkerson was advocating for this to be a keyword on the grounds that it makes error recovery much more robust. I don't have strong feelings about this - I see arguments on both sides. Taking away variable names from programmers is painful for them (and for us), but making error recovery less robust is a tax on everything.

[lrhn]

We currently disallow having both optional positional and named parameters.
Will we want to allow optional positional parameters and required named parameters on the same function?

[bwilkerson]

Personally, I'd like to see us allow all four varieties. Whether that happens sooner or later is a separate question, but it makes it much easier to migrate users to a new API if named and optional positional can be mixed, so I'd prefer sooner.

[lrhn]

It is top of my list for improvements going on 6.5 years now (https://github.com/dart-lang/sdk/issues/7056, sadly only the nr 27 most wanted feature based on 👍 count).

[leafpetersen]

Will we want to allow optional positional parameters and required named parameters on the same function?

This seems to me raise the same kind of calling convention questions that other combinations raise.

class A {
  void foo({required int x}) {}
}

class B extends A {
  void foo({int x}) {}
}

class C extends A {
  void foo([int y], {required int x}) {}
}

A call site that looks like f(A x) => x.foo(x : 3) doesn't know whether the receiver expects an optional named parameter, or a required named parameter, or a required named parameter and one or more optional parameters.

I'm not entirely unsympathetic to broadening our calling conventions, but I think that's a separate change that we need to make with full buy in from the various teams.

[lrhn]

I don't think x! should be assignable. Put it into <selector> instead.
It might just be a bad name. You can write garbage like x..foo() = 42 with the existing grammar too, it'll be disallowed by the equivalence to x.foo() = 42, which is not valid.

Maybe we can use a grammar like:

<cascadeSequence> ::= (`?..' \alt `..') <cascadeSection> (`..' <cascadeSection>)*

<cascadeSection> ::= <cascadeSelector> (<cascadeAssignment> | <selector>* (<assignableSelector> <cascadeAssignment>)?)

<selector> ::= `!' 
  \alt <argumentPart>
  \alt <assignableSelector>

<assignableSelector> ::= (`?.' \alt `.') <identifier>
  \alt `?.'? `[' expression `]'

<cascadeAssignment> ::= <assignmentOperator> <expressionWithoutCascade>

(should we allow <incrementOperator> in <cascadeAssignment> too, so you can do foo..bar++..baz = 42 and not just foo..bar += 1..baz = 42?)

[leafpetersen]

I don't think x! should be assignable. Put it into <selector> instead.

If you do this, then you can't write x!.y = 3, which I think we want to be able to write. Unless I'm missing something? But I think the x!.y can't be derived as a selector, you have to go through assignableSelector. So I was planning on adding this here, and then disallowing things like x! = 3 semantically.

You can write garbage like x..foo() = 42 with the existing grammar too,

How does this derive? I don't see how x..foo() gets derived as an <assignableExpression>.

[lrhn]

If you change selector to:

<selector> ::= `!'
 \alt <argumentPart>
 \alt <assignableSelector>

and either assignableSelectorPart to:

<assignableSelectorPart> ::= (<argumentPart> | `!')* <assignableSelector>

(from <assignableSelectorPart> ::= <argumentPart>* <assignableSelector>)
or change that and assignableExpression to:

<assignableeExpression> ::= <primary> <assignableSelectorPart> 
  \alt \SUPER{} <unconditionalAssignableSelector>
  \alt <constructorInvocation> <assignableSelectorPart>
  \alt <identifier>

<assignableSelectorPart> ::= <selector>* <assignableSelector>

then you can derive x!.y = 3.

You are correct that just changing <selector> is not enough because assignableSelectorPart doesn't use selector.

---

About ..foo() = 42, the grammar for cascadeSection is

<cascadeSection> ::= `..' (<cascadeSelector> <argumentPart>*)
  \gnewline{} (<assignableSelector> <argumentPart>*)*
  \gnewline{} (<assignmentOperator> <expressionWithoutCascade>)?

and with:

• <cascadeSelector> ↦ <identifier> ↦ foo
• <argumentPart> ↦ <arguments> ↦ ( )
• <assignmentOperator> ↦ =
• <expressionWithoutCascade> ↦^* 42

you can derive x .. foo () = 42 by having one <argumentPart>, zero (<assignableSelector> <argumentPart>*) and one (<assignmentOperator> <expressionWthoutCascade>) as allowed by the multiplicities.
There is no <assignableExpression> involved.

The problem is that <argumentPart> is a selector and not an assignable selector, but the cascade grammar allows inserting it before the assignment. The issue is the cascade grammar, and as you say the <assignableExpression> doesn't have that issue.
I think my suggestion above is better (but would obviously like someone else to check that it still allows all the right things).

The code is rejected by our tools anyway, even though it satisfies the grammar. It is then treated as equivalent to x.foo() = 42, which is itself invalid. Example:

main() {
  42..toString() = 4;
} 

gives the CFE error:

casc.dart:2:18: Error: Can't assign to this.
  42..toString() = 4;

and the analyzer error:

Analyzing casc.dart...
  error • Missing selector such as '.<identifier>' or '[0]' at casc.dart:2:5 • missing_assignable_selector
  error • A value of type 'int' can't be assigned to a variable of type 'String' at casc.dart:2:20 • invalid_assignment
2 errors found.

(The second error is spurious—there is no variable of type string in the program—which suggests that the analyzer isn't entirely comfortable with this misuse of syntax).

[leafpetersen]

@lrhn If I do as you suggest, then I don't think you can have ! in a cascade section anymore, right?

a..foo()!.bar=3;

I think this cascade parses with my grammar as:

cascadeSelector -> identifier -> foo
argumentPart* -> argumentPart -> ()
assignableSelector -> !
assignableSelector -> .bar
assignmentOperator -> =
expressionWithoutCascade -> conditionalExpression -> ifNullExpression -> logicalOrExpression -> logicalAndExpression -> equalityExpression -> relationExpression -> bitwiseOrExpression -> bitwiseXOrExpression -> bitwiseAndExpression -> shiftExpression -> additiveExpression -> multiplicativeExpression -> unaryExpression -> StayWithMeAlmostThere -> primary -> literal -> numericLiteral

How does it parse with yours?

[lrhn]

Let's pick the first grammar I proposed:

<cascadeSequence> ::= (`?..' \alt `..') <cascadeSection> (`..' <cascadeSection>)*

<cascadeSection> ::= <cascadeSelector> (<cascadeAssignment> | <selector>* (<assignableSelector> <cascadeAssignment>)?)

<selector> ::= `!'     /// <--- the ! is here
  \alt <argumentPart>
  \alt <assignableSelector>

<assignableSelector> ::= (`?.' \alt `.') <identifier>
  \alt `?.'? `[' expression `]'

<cascadeAssignment> ::= <assignmentOperator> <expressionWithoutCascade>

Then the cascade section of

a...foo()!.bar = 3

parses as

• cascadeSection ↦ cascadeSelector selector* assignableSelector cascadeAssignment
• cascadeSelector ↦ identifier (foo)
• selector* ↦ selector selector
• selector ↦ argumentPart ↦* ( )
• selector ↦ !
• assignableSelector ↦ . identifier (bar)
• cascadeAssignment ↦ = expressionWithoutCascade
• expressionWithoutCascade ↦* integerLiteral (3)

We only want the actually assignable selectors to be assignable, and ! does not introduce a LHS, so it should not be an assignable selector. Only .id, [e] and ?.[e] are assignable because it's the only suffixes that can denote a setter (counting operator[]= as a settter).

[leafpetersen]

Okay, I've taken your suggestion, PTAL.

[eernstg]

I commented on the grammar discussions, in relation to dartLangSpec.tex.

In order to check that the grammar will actually work, I've created a CL which makes adjustments to the spec_parser such that it parses language_2 (including nnbd/...) with the proposed changes, plus a number of smaller adjustments needed in order to make it work.

I made the experiment in this CL to use the ordering which was proposed by @lrhn, that is, putting late after final. I had to change a single test in order to allow this, 'late_modifier_error_test', which may or may not serve as evidence that it's an easy change. ;-)

I made adjustments to 'unsigned_right_shift_test', because it seems to have several mistakes.

All in all, this means that we now have a grammar which is known to be able to parse the current language_2 tests with the proposed NNBD updates (except a couple of files which are intended to be opt-out tests, and one test which uses a syntax which is not covered by the discussion here: 'nnbd/syntax/null_assertion_test').

The CL is https://dart-review.googlesource.com/c/sdk/+/101500.

[lrhn]

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.

[bwilkerson]

Would it be better to change it to something like the following?

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

[lrhn]

@bwilkerson
Please define "better" :)

Unless we intend to remove the old-style function parameter format (I don't), we should fully support it.
It is highly annoying if you can write foo(int compare(int a, int b)), but have to rewrite it to foo(int Function(int a, int b)? compare) when you want it to be nullable. The non-orthogonality is jarring.

As a person migrating a large library (I'm working on a proof-of-concept migration of the core platform libraries), I found it very convenient to just put a ? after the ), independently of the parameter format. I actually first wrote foo(int compare(int a, int b)?), then started worrying whether it was allowed. That suggests to me that it is a reasonable syntax, and that it is needed.

I personally like the old-style function parameter syntax, and prefer it over writing the type separately. I will be sad if I am forced to the "worse" syntax just because the type must be nullable.

So, to answer your question from a personal perspective: Emphatic no!

[bwilkerson]

Ok. The consistency argument is valid and persuasive. If the language team agrees to update the syntax to allow this, please let us know explicitly so that we can update the parser and other code paths.

What I meant by "better" is that I thought we were actively encouraging users to switch to the newer syntax and that it would therefore be better in the sense that the SDK would follow our own advice. I might be wrong about any of number of things in that line of reasoning. :-)

[stereotype441]

Ok. The consistency argument is valid and persuasive. If the language team agrees to update the syntax to allow this, please let us know explicitly so that we can update the parser and other code paths.

I went ahead and filed #364 to track this question.

[a14n]

Shouldn't it be X extends Object instead of X extends Object??

[leafpetersen]

No. Note that Object is not nullable.

X extends Object is potentially non-nullable, since it is not nullable, but it is also non-nullable, since it is a subtype of Object.

X extends Object? on the other hand is potentially non-nullable, since it is not nullable (Null is not a subtype of it) , but it is not known to be non-nullable, since it could be instantiated with a nullable type.

That may all seem confusing, but basically:

• nullable type -> definitely could get Null
  • e.g. int?
• non-nullable type -> definitely will never get Null
  • e.g. int
• potentially nullable type -> might or might not get "instantiated" such that it becomes nullable
  • e.g. X extends Object?
  • e.g. int*
• potentially non-nullable type -> might or might not get "instantiated such it becomes non-nullable.
  • e.g. X extends Object?
  • e.g. int*

[a14n]

Thanks for the explanation.

[leafpetersen]

I'm going to go ahead and land this, so that we have a baseline for further changes to be diff'ed against. I think we have all of the major discussion threads either resolved or captured in separate issues.