Breaking bug-fixes in CFE for Dart 2.1

[askeksa-google]

A number of situations that are defined to be compile-time errors by the spec were not adequately checked in Dart 2.0. Most known cases have been, or will be, implemented for Dart 2.1. Any such newly implemented compile-time error that was detected by neither the analyzer nor the CFE in 2.0 is a breaking bug-fix.

This issue aims to track our overall process of integrating and rolling out the known breaking bug-fixes.

Some breaking bug-fixes (e.g. #33022) have already been re-classified as warnings, as specified in #34349. Our known breaking bug-fixes which have not been re-classified are:

• Interface checks and mixin overrides (Fasta allows unsound abstract override #32014 and Fasta: Missing override checks for mixins #34235). These checks are necessary for soundness, so they can probably not be re-classified. At the same time, the risk of existing code breaking because of these checks is high, as these override relationships can sometimes be quite subtle. A violation has already been found (and fixed) in dartdoc.
• Relaxed generic interface conflict check (Fasta accepts conflicting interface type arguments when contravariant #34574). This was a work-around for existing super mixin code in Flutter. The flutter code has been fixed. This change is probably safe.
• Type arguments after named constructor with implicit new (Analyzer/Frontend inconsistency - type arguments after named constructor #34403). Making this a warning would be sound (as it is just unsupported syntax), but doing so will hinder future implementation of the feature, as discussed in the issue.
• Redirection from const to non-const constructor (CFE should not allow redirection from const to non-const constructor #34161). Unlikely to occur in existing code, as such a situation would probably fail either in the back-end or at runtime.
• FutureOr in an implements clause (Fasta: FutureOr in a class header can be an error #33744). Very unlikely to occur in existing code.
• Setter with the same name as the enclosing class (It is an error to have a setter with the same name as the enclosing class #34225). Unlikely, but also harmless to re-classify.
• Import prefix hidden by class member (CFE fails to report error for import prefix hidden by class member (when used to name a type) #34498).

Action items:

• Decide which (if any) of these errors should be re-classified as warnings, and add them to the list in Change errors to warnings #34349.
• Decide how to go about rolling these changes (in particular the first in the list) into internal code. Maybe we want to test it before landing the change (compiling all the code should suffice). Maybe we want to land the checks as warnings first, then up them to errors after fixing existing code.
• Fix all existing internal code.
• Assess impact on external code.
• Land the remaining fixes.
• Describe all of these changes in the release notes for 2.1.
• Mail flutter-dev, dart-misc, and dart-announce with breaking fix announcement.

[dgrove]

@kmillikin given our timeline, is this still viable?

[kmillikin]

As far as I can see, these are all implemented as errors in CFE except #34403 and #34498. It looks like we have a plan for the first of those but I'm not sure it will make it in time for Dart 2.1. The second is not caught by the front end but it is caught by the Analyzer so it seems pretty safe to consider it a bug in the CFE and we will likely ship with it for 2.1. (I don't know if it was signaled by the Dart 2.0 analyzer. If it was, then it doesn't even belong on this list.)

The rest of the tasks (decide if we will reclassify these as warnings, assess impact on external code, write them into the release notes, and anounce them) are all tasks for the language team which should be viable for Dart 2.1.

[kmillikin]

I'm sorry, I didn't understand the status of #34498. It's not caught by the CFE or the Analyzer, for different reasons.

It looks like we'll ship Dart 2.1 without fixing the issue in the front end unless the language team wants to elevate the priority for it.

[askeksa-google]

The fix for #34403 is done and reviewed and can be landed as soon as we decide that existing code is ready for it: https://dart-review.googlesource.com/c/sdk/+/78080

#34498 is neither caught by the 2.0 analyzer nor the current analyzer.

There are considerations to not reclassify #33022 as a warning after all (the CFE has reported it as an error since August 17 and nobody has complained), in which case this also goes on the list of breaking bug-fixes. Some discussion about this in #34612.

The fix for #33308 has some (legitimately erroneous) corner cases that were not caught by the 2.0 analyzer (the current analyzer correctly rejects these, plus rejects some more that should be accepted; see #34663). These cases would also be breaking changes.

[askeksa-google]

Correction: #33022 will not be breaking whatever we do, since it was detected by the analyzer (as a warning) in 2.0.

[dgrove]

Any further updates here?

[askeksa-google]

Fixes have landed for all 8 issues mentioned in the list. Only communication remains.

I can write a description of each change with examples. That could then go in the release notes and on the mailing lists with an appropriate introduction.

As mentioned, there is a corner case of #33308 (related to generic typedefs) which is also breaking. I will include that on the list of descriptions.

[askeksa-google]

"It is a compile-time error if a class named C declares a member with basename C." (10)

Dart 2.0 lacked this check for setters, allowing this code:

class A {
  set A(int x) {}
}

Issue link: #34225

---

"When const? is const, it is a compile-time error if the redirectee is not a constant constructor." (10.6.1)

Dart 2.0 allowed a constant constructor to redirect to a non-constant constructor, like this:

class A {
  const A.foo() : this(); // Redirecting to A()
  A() {}
}

Issue link: #34161

---

"Let C be a concrete class whose interface has an accessible member m named id. It is a compile-time error if C does not have a concrete member m0 named id which is a correct override of m, unless C has a concrete method named noSuchMethod which is different from the one in the built-in class Object, and C does not have a concrete member named id." (10.8.1)

Dart 2.0 missed this check in some cases. So for instance, this code would be allowed:

class A {
  num get thing => 2.0;
}

abstract class B implements A {
  int get thing;
}

class C extends A with B {} // 'thing' from 'A' is not a valid override of 'thing' from 'B'.

main() {
  print(new C().thing.isEven); // Expects an int but gets a double.
}

Issue link: #32014

---

"It is a compile-time error if an element in the type list of the implements clause of a class C is type FutureOr<T> for any T." (10.9)

Dart 2.0 did not perform this check, allowing code like:

class A implements FutureOr<Object> {}

Issue link: #33744

---

"It is a compile-time error if a class C has two superinterfaces that are different instantiations of the same generic class." (10.9)

Dart 2.0 contained a temporary relaxation of this rule that would allow two instantiations of the same generic class if one was a subtype of the other. For instance, this code would be accepted:

class I<T> {}

class A {}

class B extends A {}

class X implements I<B> {}

class Y extends X implements I<A> {} // Y implements both I<A> and I<B>

Issue link: #34574

---

"Let K be a class declaration with the same constructors, superclass and interfaces as C, and the instance members declared by M (respectively M1, . . . , Mk). It is a compile-time error if the
declaration of K would cause a compile-time error." (12.1)

Dart 2.0 did not check whether members of a mixin were correct overrides of the corresponding member of the superclass in a mixin application. This would allow code such as:

class A {
  int get thing => 2;
}

class B {
  num get thing => 2.0;
}

class C extends A with B {} // 'thing' from 'B' is not a valid override of 'thing' from 'A'.

main() {
  A a = new C();
  print(a.thing.isEven); // Expects an int but gets a double.
}

Issue link: #34235

---

"Consider a generic type alias declaration G named F with formal type parameter declarations X1 extends B1, . . . , Xm extends Bm, and right hand side T, and the parameterized type S of the form F<T1, . . . , Tl>. It is a compile-time error if S is not well-bounded." (14)

Dart 2.0 was missing this well-bounded check, allowing this code:

class A<X extends int> {}

typedef F<Y extends int> = A<Y> Function();

F<num> f = null;

Issue link: #33308

---

Dart 2.0 allowed the type arguments for an instance creation of a generic class using a named constructor to be placed after the constructor name when the optional new keyword was omitted.

class A<T> {
  A.foo() {}
}

main() {
  A.foo<String>(); // Incorrect syntax, was accepted in 2.0
  A<String>.foo(); // Correct syntax
}

Issue link: #34403

---

Dart 2.0 allowed a type to use a prefix even if that prefix was shadowed by a declaration.

import 'dart:core';
import 'dart:core' as core;

class A {
  core.List get core => null; // core refers to the field, not the prefix
}

Issue link: #34498

[askeksa-google]

I don't know where the right place is to put this (should it be reviewed, for instance?) or whether Markdown is an appropriate form for it. But here's a draft of the content, at least.