Initial support for the NNBD proposal

Initial support for the NNBD proposal

R=scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/7eb7573dc452a62c514f462460e2544569ce01be

[Brian Wilkerson, 2016-09-16 15:12:54]

brianwilkerson@google.com changed reviewers:
+ rnystrom@google.com, scheglov@google.com

[Brian Wilkerson, 2016-09-16 15:12:54]

I suspect that with some careful examination of the grammar we can make this
more efficient [1], but committing it behind a flag before that work is done
means folks can start experimenting with it.

[1] When trying to determine whether a question mark is part of a conditional
expression.

[scheglov, 2016-09-16 15:29:39]

LGTM

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:2103: void parseFunction(Parser
parser) {
The name looks like this function will parse a function :-)

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:2111: token = _skip(token.next,
parseFunction);
Out of curiosity - what is an example when just seeing COLON after an expression
is not enough?

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_test.dart (right):

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:10482:
expect(typeName.typeArguments, isNotNull);
Should we check for "question"?

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:10495:
expect(typeName.typeArguments, isNull);
Should we check for "question"?

Do we need a test for parsing TypeName in a more complex expression?

[Brian Wilkerson, 2016-09-16 15:47:35]

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:2103: void parseFunction(Parser
parser) {
> The name looks like this function will parse a function :-)

Yes it does. :-) I've renamed it to 'parseOperation' in leu of a better name.

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:2111: token = _skip(token.next,
parseFunction);
> Out of curiosity - what is an example when just seeing COLON after an
expression
> is not enough?

I'm concerned about cases like

  if (foo is Bar? && flag ? m1() : m2())

where we need to ensure that the colon isn't part of some other conditional
expression.

I'm not sure, but it might be possible to put together a list of tokens that can
legally follow the '?' when it isn't a conditional expression ('&&', '||', ')',
';', etc.) and reverse the sense of the condition, thus making this require only
a small number of tokens to be looked at, but I need to spend some time with the
grammar to be sure.

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_test.dart (right):

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:10482:
expect(typeName.typeArguments, isNotNull);
Done

https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:10495:
expect(typeName.typeArguments, isNull);
> Should we check for "question"?

Done

> Do we need a test for parsing TypeName in a more complex expression?

I added test_conditionalExpression_precedence_nullableType, but I'm happy to add
more. Any suggestions?

[Brian Wilkerson, 2016-09-16 17:25:44]

ptal

[scheglov, 2016-09-16 17:32:31]

LGTM

The potential performance hit is even bigger now, as we clone all tokens after
every "?" after a TypeName :-(

[Brian Wilkerson, 2016-09-16 17:50:45]

Yes. I think we really need to find a more efficient way to perform the test
before enabling this feature.

[Brian Wilkerson, 2016-09-16 17:51:22]

Description was changed from

==========
Initial support for the NNBD proposal
==========

to

==========
Initial support for the NNBD proposal

R=scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7eb7573dc452a62c514f462460e2544569ce01be
==========

[Brian Wilkerson, 2016-09-16 17:51:24]

Committed patchset #2 (id:20001) manually as
7eb7573dc452a62c514f462460e2544569ce01be (presubmit successful).

[Bob Nystrom, 2016-09-16 21:46:11]

On 2016/09/16 15:29:39, scheglov wrote:
> LGTM
> 
>
https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/parser.dart (right):
> 
>
https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/parser.dart:2103: void parseFunction(Parser
> parser) {
> The name looks like this function will parse a function :-)
> 
>
https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/parser.dart:2111: token = _skip(token.next,
> parseFunction);
> Out of curiosity - what is an example when just seeing COLON after an
expression
> is not enough?
> 
>
https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
> File pkg/analyzer/test/generated/parser_test.dart (right):
> 
>
https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
> pkg/analyzer/test/generated/parser_test.dart:10482:
> expect(typeName.typeArguments, isNotNull);
> Should we check for "question"?
> 
>
https://codereview.chromium.org/2342383002/diff/1/pkg/analyzer/test/generated...
> pkg/analyzer/test/generated/parser_test.dart:10495:
> expect(typeName.typeArguments, isNull);
> Should we check for "question"?
> 
> Do we need a test for parsing TypeName in a more complex expression?

Some other tests that would be interesting:

class A extends B? {} // Error.
class A implements B? {} // Error.
class A extends Object with B? {} // Error.

class A<T?> {} // Error.

class A<T extends B?> {} // OK.
new Foo<T?>(); // OK.

Also, doing the same tests for "as" that you have for "is" might be good.