Meta issue for Dart 1.5 generic methods, non-strong mode.

[leafpetersen]

This is a tracking bug for generic methods in non-strong mode Dart 1.5 .

• Final decision on dynamic behavior (private issue): https://github.com/dart-lang/dart-lang-evolution/issues/103
• Final decision on static behavior (private issue): https://github.com/dart-lang/dart-lang-evolution/issues/107
• Specification issue: Specification needs to be updated for non-strong mode generic methods. #27639
• VM implementation issue: Support syntactic (only) support for generic methods in the VM. #25869, Add support for treating generic method parameters as dynamic in all contexts #27460
• Dart2js implementation issue: Support generic method syntax in dart2js #25835, final CL
• Analyzer implementation issue: Analyzer support for Dart 1.50 non strong mode generic methods. #27641
• Ensure DartDoc renders generic method syntax correctly: Support generic method syntax dartdoc#1278
• Update changelog with description of this change: Update changelog w/ details about Generic Method syntax #27875
• Handle generic method syntax in dartfmt (Support generic method syntax dart_style#556)

[leafpetersen]

We have an informal specification for this here:

https://docs.google.com/document/d/1nccOSkdSyB9ls7TD8gUqIu6Wl2J_9bTSOoMCEIoAiQI/edit#heading=h.dnanv15dn1me

[leafpetersen]

@eernstg My understanding is that the dart2js implementation is up to date, is that correct? If not I'll file an implementation bug.

[eernstg]

The dart2js implementation currently needs the '--generic-method-syntax' option in order to enable this feature, and it emits a warning (reportWarningMessage). If the latter conflicts with "warning free" code requirements then I'd need to change that to reportHintMessage.

[leafpetersen]

I believe that support is now at a point where we can start the process of moving this out from under a flag in all implementations and testing it out, probably starting with the SDK.

[bwilkerson]

Is there an order in which the flags should be removed, or should I go ahead and remove the flag from analyzer?

[leafpetersen]

@eernstg is managing the flag removal, but I believe the plan is to do this as soon as possible. From my perspective, the only constraint is that all of the flags should be removed atomically with respect to the next roll into google3 (that is, I don't think we should do a roll in which the analyzer supports them without a flag but the VM or dart2js doesn't).

I'd also like to see us do this soon, so we can start converting over the comment syntax and get some more confidence that there aren't bugs.

[eernstg]

We will now get ready to enable this feature in all tools: Every tool team can go ahead and create a CL to enable syntax-only generic methods by default, but keep that CL un-landed for now. We will land this feature at the same time, and put a bullet into CHANGELOG.md about it.

Here's some information about the considerations behind this approach:

It is in fact a breaking change to enable syntax-only generic methods by default: for instance, the argument list in foo(a < b, c > (d)) used to contain two relational expressions, but with this feature it is one invocation of a generic function a with type arguments b, c and value argument d. We expect the number of libraries affected by this problem to be extremely low, but we should still make sure that we introduce this feature in a way that keeps the disruption at the lowest possible level.

It should be noted that there is no way the introduction of this feature could silently change the meaning of a working program: If foo(a < b, c > (d)) is working code before the change then c cannot possibly be a type literal (because Type does not support operator >, and type literals are resolved statically so this would be a static type warning in plain Dart, and an error in strong mode). Even if it is plain Dart and the warning is ignored, it is a runtime error to evaluate c > (d) when c is a type literal, no matter whether d can be evaluated or not. The same line of reasoning is applicable if c is replaced by c<..>, which looks like a class with type arguments after the change, and would have been some relational expressions ending in .. >> (d) (using the shift-right operator) before the change (and >>> won't parse, so it ends here).

The disruption amounts to programs that used to work but now have compile-time errors, or warnings and run-time errors. The diagnostic messages might not be very helpful (because the old code was never intended to call generic methods), so we will need to make sure that the developers will have a chance to discover what is going on, if it hits them. Hence the entry in CHANGELOG.md.

[crelier]

VM changelist: https://codereview.chromium.org/2486943003

[bwilkerson]

Analyzer CL: https://codereview.chromium.org/2488043002/

[leafpetersen]

status: Implementation is almost done, I believe there were some issues with the presubmit. @eernstg can you update?

[eernstg]

Analyzer being debugged (some issues arose in the global pre-submit in google3).

[eernstg]

One update in google3 requested, and then we should have a clean global pre-submit run with the analyzer.

[eernstg]

Current status:

The VM and dart2js CLs went through the global pre-submit with 14 new errors, all of which were unlikely to be caused by this change (for instance, almost all of them had a "recently flaky" flag, and the remaining ones were network errors like 'recycling port still in use', and similar things). Consequently, last week I asked to get these CLs (2486943003 and 2476423005) landed, and that should happen within days (2476423005 was landed last week and reverted because of some status file issues, an update is expected to land tomorrow; I just sent a reminder about landing 2486943003).

The analyzer was a bit more complex to handle; the global pre-submit had 144 new test failures; they turned out to be concerned with generic tear-offs (someFunction<SomeType>) in comment syntax; the google3 code was adjusted (CL 139451169), so google3 is ready now.

One issue remains: The analyzer performs a large number of checks on generic methods, and we haven't clarified the relationship between the checks performed by the analyzer and the checks specified in the informal spec of this feature. Apparently, it is not simple to make the analyzer perform just those (few) checks, and it is not simple to make the analyzer ignore the comment syntax when in standard (non-strong) mode.

[bwilkerson]

It appears that I haven't been very clear in our conversations, for which I apologize.

... it is not simple to make the analyzer perform just those (few) checks ...

That's not accurate. If we know of an error that is being produced when it shouldn't be it's fairly trivial to find where that's happening and either stop reporting it or report a hint instead of an error. Hence my question about tests, because the tests will tell us when we're incorrectly generating an error so that we can fix it.

What isn't trivial is to ensure that we have caught all of the places where an existing error is being generated when it shouldn't be. Basically we'll only be as complete as the test suite used to find such places.

... it is not simple to make the analyzer ignore the comment syntax when in standard (non-strong) mode ...

As far as I know, analyzer does ignore the comment syntax when in standard mode. (I think we were mis-communicating because we were discussing both a general principle and a specific case at the same time.) If you have evidence to the contrary, please let me know.

[crelier]

I just landed the flag change in the VM.

[eernstg]

The language group discussed the situation and agreed that it is OK to proceed to land the analyzer CL and then handle the error-to-hint adjustments afterwards.

[mit-mit]

The language group discussed the situation and agreed that it is OK to proceed to land the analyzer CL and then handle the error-to-hint adjustments afterwards.

@bwilkerson can we go ahead and land the cl so that we can get this complete in 1.21?

[mit-mit]

@munificent I see in dart-lang/dart_style#556 that formatter support landed in that repo. Do we need something to bring that over to 1.21 in the SDK?

[eernstg]

FYI: The analyzer CL was already landed yesterday (and a comment on this action was added to #27641 when it was closed).

[munificent]

Do we need something to bring that over to 1.21 in the SDK?

I just published a new version of dart_style with that fix (and the other half of generic method syntax that I overlooked—parameters), and pulled that version into the master branch of the SDK's DEPS file. As far as I know, we should be good after that.

[eernstg]

Cool! Closing.