Make `extends C implements C` and `extends C with C` a warning

[eernstg]

[Cf. the discussion in https://github.com/dart-lang/sdk/issues/42256].

It is currently an error to have the same type as a superclass and also as one of the types in the implements clause:

class A {}
class B extends A implements A {} // Compile-time error.

This error is solely motivated by the fact that this is a confusing construct that provides no affordances at all (hence also: no useful affordances).

This issue is a proposal that we should change that from being a compile-time error to being a non-error. At this time, the language specification documents do not have an opinion about warnings (except for a few, for historical reasons), but the language team could recommend that it is somehow reported, e.g., as a warning from the analyzer, or as a lint.

This would make the mixin application C with C a warning/lint when C is a mixin class (because the specified meaning of that mixin application is a class where C is the superclass as well as an element in the implements list).

We could also have a lint for the case where the same mixin or mixin class occurs two or more times in a with clause (like class D = Object with M, M;). This would be consistent with the warning for C with C mentioned above, and it makes sense from a software engineering perspective because repeated occurrences of a mixin is a construct which is very unlikely to be useful (and very likely to be a mistake).

Anyone who really needs repeated occurrences of the same mixin in the same class would just need to ignore that lint.

@dart-lang/language-team, WDYT?

[natebosch]

Anyone who really needs repeated occurrences of the same mixin in the same class would just need to ignore that lint.

What does this let you accomplish that can't be accomplished without duplicating mixins in a single class?

[lrhn]

What does this let you accomplish that can't be accomplished without duplicating mixins in a single class?

Who knows, but the semantics are well-defined, so the language doesn't need to have an opinion on that.

It's something much better suited as an analyzer warning than a specified language error.

(And the vague phrasing of the error specification is making people disagree on whether class C = M with M; is an error or not. My reading: it's not. Analyzer gives error, CFE does not.)

[eernstg]

@natebosch wrote:

What does this let you accomplish ..?

The main point is side effects. You could have multiple allocations of storage for "the same" instance variable (like _dummy below), and you could presumably use super in twisted ways to access that memory (or it might just be a sort of space leak), but the duplication of side effects is easy to get, easy to detect, and probably a mistake.

int counter = 0;

mixin S {
  // ignore:unused_field
  final _dummy = ++counter; // We can have side effects.
}

class C = Object with S, S; // Same thing as `S with S`, but definitely allowed now.

main() {
  new C();
  print(counter); // '2' because the `C` has two mixins `S`.
}

[natebosch]

I think making this a non-universal could be a disservice to users. Even if we add the lint to our core set we're still opening up room for potentially very confusing code if some project ends up relying on those side effects. I think if an author wants to rely on effects this subtle they should need to do more than ignore (or especially fail to enable) a lint.

[lrhn]

We happen to disallow extends S implements S and implements S, S today, making them spec errors. I don't think that level of error is warranted for something so I'm for making them non-errors. They're useless, so I'd be fine with the analyzer giving a warning instead.

I'll maintain that we haven't made class C = S with S; an error, and the analyzer currently reporting it is wrong.

We have no current error for doing with S, S.

And you can do it too:

class A {
  int get foo => 0;
  int get bar => 0;
}
mixin F on A {
  int get foo => super.foo + 1;
}
mixin S on A {
  int get foo => super.bar;
  int get bar => super.foo;
}
class D extends A with S, F, F, F, S {}
void main() {
  print(D().bar); // 3
}

Should you? Probably not, but it's well-defined behavior, so there is no need to disallow it. And we don't today.

The only two errors we actually specify is extends S implements S and implements S, S. Those are the only two we should consider changing to (non-required) warnings.

[eernstg]

The only two errors we actually specify is extends S implements S and implements S, S. Those are the only two we should consider changing to (non-required) warnings.

It makes sense to weaken those two to tool specific warnings rather than language specified errors: They cannot cause any harm technically because they have no effect, they are only a potential source of confusion.

Next, we can recommend that the analyzer stops reporting S with S, but I don't see why anybody would thank us for that.

Note that we rely on errors arising from the desugaring of mixin applications to classes, specified here, in many ways. For instance, there is no other reason why it should be an error for a mixin application to introduce an override relationship which is not a correct override:

class A { void m() {}}
mixin M { int get m => 1; }
class B = A with M;

In the class that is the result of desugaring A with M, we have a compile-time error because it declares an m getter that overrides the m method in the superclass A, but if we refuse to recognize that the desugared class is a source of errors then we have no reason for saying that there is an error, or even that there is an override relationship between two members named m.

So in order to say that S with S is not an error (according to the current rules), we need to introduce some special exceptions saying that "ah, by the way, it's not an error when the desugared class has this particular error". We can make special exceptions like that, but I'm still not convinced that we're helping anybody by doing that.

It would actually be a more strict approach than what we have previously had if the analyzer starts warning in case of duplicate mixins (SomeClass with M, M). As mentioned, this does make a difference semantically, and I'd expect that it is done intentionally only in exceedingly rare cases. To me that looks like a reasonable candidate for a lint, and probably a recommended one.

[lrhn]

I'd rather remove the error entirely, than have to explain why

class C = S with S;

is an error, but

mixin N {}
class C = S with N, S;

or

class C = Object with S, S;

is not.

It's an unnecessary error, based only on a single declaration not needing to mention an interface twice, and then generalized to a desugaring that nobody would write anyway.

Or put differently: It was originally a warning. The warning was there as a help, not a restriction. It told you that you had written something unnecessary, and the solution was to remove that unnecessary implements clause.

When we then defined mixin application in terms of desugaring (and yep, even if I was the one doing that), it was never intended to introduce an error which we wouldn't have given for the mixin application if we had not defined it through desugaring. The error (supposedly) comes because the desugaring says that the desugared mixin application class implements the interface of the mixin. And if it had been a user written class, they would remove the implements clause and all would be well. Since the user didn't write this code, they also cannot fix it. Giving the user an unfixable error, based on an error that is designed to be easily fixable, is a bug in the specification. The specification should remove the implements clause and avoid the error.

It's a general design rule: If we use desugaring as specification, which we should try to avoid because of unforseen issues like this, it should never introduce a new error, one we wouldn't have wanted to give anyway if the desugaring hadn't produced it. If that happens it's a bug in the desugaring, not in the program.

So the desugaring should only add the implements clause for the mixin type if the superclass doesn't already implement it. That should change nothing ... except getting rid of this then clearly spurious error.

[eernstg]

Very well! Sounds like there is support for removing the two compile-time errors specified here.

If we do that then there's no reason to debate desugaring and errors derived from that: They are gone anyway, as seen from the specification.

I created a lint proposal, https://github.com/dart-lang/linter/issues/4548, in order to address the duplication of mixins (no matter the syntactic form).