Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Having a mixin implement a private member of another mixin causes a name collision #4244

Open
rrousselGit opened this issue Jan 31, 2025 · 5 comments
Labels
request Requests to resolve a particular developer problem

Comments

@rrousselGit
Copy link

Consider the following:

// mixin.dart
mixin Mixin {
  int _method(int value);

  // Some more methods
}

mixin OtherMixin on Mixin {
  @override
  int _method(int value) => value;
}

Then:

import 'mixin.dart';

class Foo with Mixin, OtherMixin {}

This unexpectedly fails with:

The private name '_method', defined by 'OtherMixin', conflicts with the same name defined by 'Mixin'.
Try removing 'OtherMixin' from the 'with' clause.dart[private_collision_in_mixin_application](https://dart.dev/diagnostics/private_collision_in_mixin_application)

But there's no name conflict. Instead OtherMixin._method is from the same interface as Mixin._method

@rrousselGit rrousselGit added the request Requests to resolve a particular developer problem label Jan 31, 2025
@lrhn
Copy link
Member

lrhn commented Jan 31, 2025

There has been repeated talk about making it an error for a mixin application with private member to override a concrete existing member with the same name.

I don't remember whether it was ever decided to do so. I think it might have been.

The analyzer appears to have implemented this: dart-lang/sdk#45959
The front-end has not.
Probably be cause the spec change has never landed: #1626

(If the intent changes, but the spec doesn't, has the language really changed or not?)

In any case, this is a perfect example of why you might want mixins to have the same private members as another class or mixin in the same library. If the base type is marked base, then you're even guaranteed that the member is there to override.

Here OtherMixin sends all possible signals that it intends to override: It is on Mixin, so it's clearly aware of the other member. It writes @override too, but the language doesn't care about annotations.
And the member it overrides is abstract, so clearly intended to be overridden.

I don't know which precise rule is implemented.

I do remember that one reason to disallow is to thereby allow implementations to assume (and optimize for) access to to a _method that wasn't overridden inside the same library as if it was statically resolved. Possibly mainly for access to instance variables without going through a getter. (Overriding an instance variable with another instance variable with the same private name, when the compiler doesn't think that it happens, can get very messy.)

That does not make sense for an abstract method, so that should not be the reason to disallow this case.

All in all, I think that error should be dropped.
Either the override works, the name and types match, and both are written inside the same library, so it's probably intentional, or the types don't match and then it's an error anyway. (Not one you can do much about, but the library declared two different members with the same name, one in a mixin, which makes that mixin not applicable to the other class with the incompatible member declaration. We can warn if the mixin appears to be public and applicable, but in the end, it's just incompatible interfaces, no different from if the members had been public. It's just harder to see from the outside.

If we do want some error, we should not err when the mixin is declared on a type that has the same member, because then the override is definitely intended. It has already happened at the interface level.
That could be the logic: If you apply a mixin to a class, and the mixin and class both declare a private member with the same name (and they have compatible types, because otherwise it's just a normal override error), then give an error unless at least one of the mixin's on interfaces contains a member with that name.
Because then we know that the superclass of the mixin application implements the same interface, and then it is the same member, the override is intended.

We could also not err if one of the mixins super-interfaces contain the member, but then we'd have to check the mixin and superclass has a shared superinterface declaring the member. Then it's also "the same member", so it's not surprising to a compiler that there exists an implementation of that interface member in a mixin.

@rrousselGit
Copy link
Author

What doesn't help here is that mixin composition isn't a thing.
Ideally OtherMixin should not be using on Mixin but instead do:

mixin OtherMixin with Mixin {
...
}

@nate-thegrate
Copy link

What doesn't help here is that mixin composition isn't a thing.

@rrousselGit absolutely! Currently we have to either use multiple mixins or duplicate the logic.

This includes the code in dart:core as well:

abstract mixin class Iterable<E> {
  // This class has methods copied verbatim into:
  // - SetMixin
  // If changing a method here, also change other copies.
  ...

@rrousselGit
Copy link
Author

I worked around it by using private top-level functions.

Instead of:

// mixin.dart
mixin Mixin {
  int _method(int value);

  void somePublicMethod() {
    _method(42);
  }
}

mixin OtherMixin on Mixin {
  @override
  int _method(int value) => value;
}

I did:

// mixin.dart
mixin Mixin {
  void somePublicMethod();
}

void _somePublicMethodLogic(Mixin mixin, {required int Function(int) method}) {
  _method(42);
}

mixin OtherMixin on Mixin {
  int _method(int value) => value;

  @override
  void somePublicMethod() => _somePublicMethodLogic(this, method: _method);
}

Not perfect, but that fixes the issue

@MarvinHannott
Copy link

I actually wonder why mixins support private members at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

4 participants