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

Strong mode: Disallow private collisions induced via mixin #28809

Closed
kasperl opened this issue Feb 17, 2017 · 33 comments

Comments

@kasperl
Copy link
Contributor

commented Feb 17, 2017

We want to introduce an error in strong mode that flags all private member (field/getter/setter/method) collisions induced by a separate library applying a mixin.

The suggested name for the error is StrongMode.PRIVATE_COLLISION_IN_MIXIN_APPLICATION.

@kasperl kasperl added this to the 1.23 milestone Feb 17, 2017

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2017

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Is there a test case for this? (There's not a lot of detail in this issue, and nobody has volunteered to own the issue.)

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

Writing down the test cases for this seems like a very useful exercise. I'll see if I can find time or someone to take a stab at that.

The basic idea is that two private members (fields, getters, setters, and methods) with the same name from the same library shouldn't be allowed to be mixed together through mixin applications in other libraries.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

So would the following be an example?

class Mixin1 {
  void _init() {}
}
class Mixin2 {
  void _init() {}
}
class C extends Object with Mixin1, Mixin2 {} // Error because _init came from two separate mixins
@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

If C is in a different library than Mixin1 and Mixin2 then yes, the intent is that it should be flagged. Maybe @vsmenon can confirm the intent?

@vsmenon

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Right, what @kasperl said.

In general, when a programmer is applying a mixin from a different library, the analyzer will need to check that it doesn't cause a private member collision (which must also involve a class/mixin in that other library).

The intent is to let the programmer and compiler reason about a library's private members without worrying about how downstream code may accidentally use them / override them. The intuition is that it's unlikely that the downstream programmer intended the private member conflict.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

I'm working my way toward a guess at the specification, but I should have started by asking whether there is an informal spec for this. If so, you can probably ignore this next question.

So what about indirect in-mixing? For example, if we have in lib1.dart:

class Mixin1 {
  void _init() {}
}
class Mixin2 {
  void _init() {}
}

and in lib2.dart:

class A extends Object with Mixin1 {}
class B extends A with Mixin2 {} // Error?
@vsmenon

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

This thread is probably the closest thing to an informal spec right now. :-)

Yes, the intent is that would be a collision. lib2 is applying Mixin2 from lib1 and it's creating a collision on _init.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

How does the following sound for a first cut toward an informal(-ish) spec:

Given a class C, we define the immediate mixed-in classes of C to be the set of classes in the with clause of C. We define the mixed-in classes of C to be the union of the immediate mixed-in classes of C and the mixed-in classes of the superclass of C.

Given a class C whose mixed-in classes are M1, M2, ... Mn, it is a compile-time error if there is a pair of classes Mj and Mk such that both Mj and Mk (a) are defined in the same library and (b) define a private member named n.

Does that match your expectations?

@vsmenon

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

I don't think that covers this case:

lib1.dart:

class A {
  int _x;
}

class B {
  int _x;
}

lib2.dart:

class C extends A with B {} // This should be an error -> it introduces a conflict on _x
@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

Ah. Then I take it we want to include all of the superclasses of C in the list of classes being checked.

Is it true that we can ignore interfaces (because they won't contribute private members)?

@vsmenon

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

Right, I believe spec linearizes the mixins so you can consider them one at a time (and the application of any previous ones to be already be in the superclass chain).

So, if you're mixing in a class from a different library, check the superclass chain for a conflict on any private member.

Does that sound right to you?

I think you're correct on interfaces.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

I'm not sure. What about this example:
lib1.dart:

class A {
  int _x;
}

class B extends A {
  int _x;
}

lib2.dart:

class C extends B {}

Should there be an error for C? Two of its superclasses define members with the same private name, but neither of them are mixed in. I suspect that it matters whether the private member is introduced by a class in the with clause or by a superclass.

Is it only an error for conflicts that are directly mixed in by the declaration of C? For example, I suspect that there should be no errors in the following:
lib1.dart

class Mixin1 {
 int _conflict;
}
class Mixin2 {
  int _conflict;
}
class Mixin3 {
  int _noConflict;
}
class B extends Object with Mixin1, Mixin2 {}

lib2.dart:

class C extends B with Mixin3 {}
@vsmenon

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

I think those examples are fine (i.e., no error). In neither case does lib2.dart trigger the conflict.

Part of the intent here is to let us look at lib1.dart in isolation and figure out what can be overridden.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

... look at lib1.dart in isolation and figure out what can be overridden.

I don't understand what you mean by that. Given that the conflicting private methods are by definition from a different library, you have to look outside lib1.dart in order to determine that they exist. Can you elaborate (possibly by writing an informal spec)?

@vsmenon

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

I don't understand what you mean by that. Given that the conflicting private methods are by definition from a different library, you have to look outside lib1.dart in order to determine that they exist. Can you elaborate (possibly by writing an informal spec)?

You should be able to look at lib1.dart in your second example and prove that _noConflict cannot be overridden. You should not need to look at lib2.dart in order to reach this conclusion about lib1.dart.

Today, lib2.dart could do this:

class Trouble extends Mixin3 with Mixin3 {}

That should also trigger an error.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

Ok. I have a first cut at implementing this error, but I can't commit it because it breaks the build. It breaks the build because dart2js contains a violation of this rule. The error occurs here (https://github.com/dart-lang/sdk/blob/master/pkg/compiler/lib/src/tree/nodes.dart#L2504) and is cause by the fact that StoredTreeElementMixin defines a private member named _element which conflicts with the private element named _element defined in NullTreeElementMixin (which, despite is name, is extended by Node and hence by AsyncForIn).

Do we need to rethink / redefine this error, or do we need to fix dart2js?

@dgrove

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

@vsmenon

This comment has been minimized.

Copy link
Member

commented Feb 26, 2017

Thanks, Brian. Is that the only error you're seeing the SDK?

Looks like these are the types in question:

https://github.com/dart-lang/sdk/blob/master/pkg/compiler/lib/src/resolution/secret_tree_element.dart#L48

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Feb 26, 2017

Actually, all I saw was a build bot failure saying that the kernel snapshot couldn't be built, so I asked for help. Vyacheslav investigated and pointed out this failure. I didn't ask whether this was the only failure; I assumed he would have said so if there were more.

Yes, that's one of the types. The other is on line 33 of the same file.

@sigmundch

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

/cc @johnniwinther
Regarding the dart2js code:

  • not sure why this is causing failures at this moment: dart2js is not strong clean, so I expect other strong failures at this moment
  • will we also expose a way to declare that we intentionally override a private member with a mixin? (for example, labeling a private field as @virtual?)

FWIW - here is a simplified snippet of the code you linked above:

class I {
  get _x;
  set _x(v);
}

class N implements I {
  get _x => null;
  set _x(v) => throw "";
}

class S implements I {
  Object _x;
}

// lib2.dart:

class B extends N {} 
class C extends B with S {}
  • an alternative here might be to change our getTreeElement logic and do first a check like is TreeElementMixin or something similar and avoid the override, but there might be some associated cost to that.

  • last, but not least, this part of the code will no longer exist once we switch to kernel, so I'd prefer to ignore/ypass this error in dart2js in the interim if we can.

@vsmenon

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

The objective is for this to only be a strong-mode error. Brian was checking everything. If this code is never intended to be strong mode clean, I don't think you need to do anything.

There is no immediate (1.23) plan for @virtual, but there is discussion about making mixins more explicit in the language (which might re-allow this use case).

@rakudrama

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

We could apply one of these two CLs to make dart2js conform to the restrictions.
Both have problems, but the technical debt will expire when the code is deleted after completing the switch to kernel.

https://codereview.chromium.org/2722923002
https://codereview.chromium.org/2722043002

I think this feature (the aggregation of an implementation that has a private contract) is useful and would like it to be supported in a future version of dart.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Mar 8, 2017

I have updated my CL to only generate the error in strong mode. In so doing I had to update the 4 tests I had written to be run in strong mode. That turned up the interesting information that 3 of the 4 tests also produce an error with the StrongModeErrorCode.INVALID_FIELD_OVERRIDE error code. Maybe it's just the tests I came up with, but it seems like we shouldn't need two errors that are this likely to be produced together.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

This change is to enable the elimination of the INVALID_FIELD_OVERRIDE error. All fields will become virtual in strong mode, modulo the restriction from this issue.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2017

Brian, thanks so much for working on this. How far are you with landing this?

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

The CL has been approved and is ready to land, but is held up by other work.

@bwilkerson bwilkerson closed this Mar 15, 2017

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

Undo the accidental close.

@bwilkerson bwilkerson reopened this Mar 15, 2017

@rakudrama

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

I have committed one of the CLs that makes dart2js 'clean' with respect to this warning.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

Thanks! Do we have an estimate for when the other CLs might be ready?

@vsmenon

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

I think @rakudrama looked at multiple potential fixes for the same warning and decided which one to land (and landed it). I don't think we're waiting on the rest.

@dgrove

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

@bwilkerson what's remaining here?

@bwilkerson

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.