CFE: issues with noSuchMethod forwarders for private members from other libs

[sigmundch]

Not sure if this was by design or not: the CFE is including nsm-forwarders for private members that originate in other libraries. It appears we can workaround this on the dart2js side by not generating the code for some nsm-forwarders, but it seems to me like we don't want to generate them in the IR in the first place.

Here are some examples.

Example 1: a private field:

a.dart:

import 'b.dart';

class A implements I {
  int _field = 1;
  noSuchMethod(Invocation m) => 2;
}

main() => print(new A()._field);

b.dart:

class I {
  int _field = 0;
}

The CFE generates a nsm-forwarder for _field defined in b.dart. As a result, dart2js prints '2' instead of '1' on this program.

I discovered this from a failing test of one internal customer.

Example 2: other private members, from same and different libraries.

This example just highlights that we do want nsm-forwarders for private members of the same library though.

a2.dart:

import 'b2.dart';
class J {
  int _m3() => 0;
  int _m4() => 0;
}

class A implements I, J {
  int _m1() => 1;
  int _m3() => 3;
  noSuchMethod(Invocation m) => -1;
}

main() {
  dynamic a = new A();
  dynamic b = confuse([]);
  print(a._m1());   // should print 1
  print(a._m3());   // should print 3
  print((a._m1)()); // should print 1
  print((a._m3)()); // should print 3

  print(a._m2());   // should print -1 
  print(a._m4());   // should print -1
  print(a._m2);     // should print -1, and is not a closure
  print((a._m4)()); // should print -1, a._m4 does generate a valid tear-off from the nsm-forwarder.
}

b2.dart:

class I {
  int _m1() => 0;
  int _m2() => 0;
}

[sigmundch]

@kmillikin - I'm guessing this is a CFE issue, but let me know if that's not the case.

We are landing a workaround in dart2js here: https://dart-review.googlesource.com/c/sdk/+/62711, but we should remove it once the CFE is fixd.

[kmillikin]

We'll figure out what we want in this case and report back here.

[chloestefantsova]

Warning: a rather lengthy explanation ahead :)

TL;DR I think, it's a non-issue.

The noSuchMethod forwarders generated from I._field in A have a name that is private to b.dart and is not visible in a.dart. So, in main, A._field should be invoked, because it's the only visible _field in that file.

The first example above is compiled to the following:

library;
import self as self;
import "dart:core" as core;
import "./b.dart" as b;

class A extends core::Object implements b::I {
  field core::int _field = 1;
  synthetic constructor •() → void
    : super core::Object::•()
    ;
  method noSuchMethod(core::Invocation m) → dynamic
    return 2;
  no-such-method-forwarder get /* from file:///tmp/b.dart */ _field() → core::int
    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("get:_field", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false)) as{TypeError} core::int;
  no-such-method-forwarder set /* from file:///tmp/b.dart */ _field(core::int value) → void
    return this.{self::A::noSuchMethod}(new core::_InvocationMirror::_withoutType("set:_field", const <core::Type>[], core::List::unmodifiable<dynamic>(<dynamic>[value]), core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));
}
static method main() → dynamic
  return core::print(new self::A::•().{self::A::_field});
library;
import self as self;
import "dart:core" as core;

class I extends core::Object {
  field core::int _field = 0;
  synthetic constructor •() → void
    : super core::Object::•()
    ;
}

Note that A gets both the field _field and the accessors for I._field that are marked with /* from file:///tmp/b.dart */. This comment is actually the textual representation of a private name and shows in which library that name is visible. It means that self::A::_field above looks up the one from the same module and should ignore the one from b.dart.

This is how, I believe, is done in the VM that currently prints 1 as the output.

Now, let's modify the example a little, so that it matters that the noSuchMethod forwarders are generated for the private fields:

// ======== a.dart ========
import 'b.dart';

class A implements I {
  int _field = 1;
  noSuchMethod(Invocation m) => 2;
}

class B implements I {
  noSuchMethod(Invocation m) => 3;
  // Doesn't have a `_field`, but has the forwarders for `I._field`, private to `b.dart`.
}

main() {
  print(new A()._field);
  // print(new B()._field);
  foo(new B());
}

// ======== b.dart ========
class I {
  int _field = 0;
}

void foo(I i) {
  print("${i._field}");
}

When run with the VM, it prints out the following:

$ out/ReleaseX64/dart /tmp/a.dart
1
3

Here foo successfully invokes I._field, because it's in the same library. In this case result is whatever noSuchMethod returns in B.

Note that if the commented line in main is uncommented, the following error message is produced:

$ out/ReleaseX64/dart /tmp/a.dart
file:///tmp/a.dart:14:17: Error: The getter '_field' isn't defined for the class '#lib1::B'.
Try correcting the name to the name of an existing getter, or defining a getter or field named '_field'.
  print(new B()._field);

That is, the getter is not visible in a.dart, and the CFE notifies about that.

[eernstg]

@sigmundch, I agree with your analysis. If we were to allow an nSM-forwarder to implement an inaccessible method signature (that is, the method signature of a private method declared in a different library) then we would again position noSuchMethod as a magic tool: There is no way you could have implemented (or overridden) a private method declared in a different library using a regular member declaration. (It's 'again' because we decided to avoid magic in the semantics of nSM-forwarder invocations concerning the treatment of optional parameters: they are treated exactly as the optional parameters of a user-written method declaration).

With the support for noSuchMethod based implementations of inaccessible private methods we would give developers a new power to violate privacy. That should be a separate language mechanism that we have decided to introduce, or it should not exist—it should not be a subtle "extra power" in noSuchMethod.

So print(new A()._field) should print 1 and not 2. But that's also what I see with dart2js as well as dart ... so maybe some action was already taken to get that behavior? The version I'm using is 2.0.0-dev.66.0 (checking an arbitrary older version, 64.0 behaves the same).

However, print(a._m2) prints a closure rather than -1, and it shouldn't do that.

@stefantsov, you describe how the treatment of private names in CFE is such that a private name _n from a library L1 is inaccessible to code in a different library L2 even when that private name is the name of an nSM forwarder in L2, so I can see how we avoid a number of additional problems (that we would have if _n were treated as a private name from L2). But this still doesn't preserve the privacy protection that we otherwise enforce, because it allows the code written in noSuchMethod in L2 to run when that (no longer so) private method in L1 is executed. Maybe L1 wants to give a password in clear text as an argument to _n, and code in L2 would now know the password. ;-)

The consistent treatment would be to ensure that "external" code (looking from a library L1, it would be code from a different library L2) cannot specify private names (from L1), not even indirectly via nSM forwarders.

[eernstg]

Having discovered (with a little help from my friends ;-) that we cannot avoid giving noSuchMethod the above-mentioned powers to violate privacy, I agree with @stefantsov that we should allow the front end to generate nSM forwarders also when they will have private names from a different library, that is, the current behavior in the front end should be preserved. @lrhn agreed to this conclusion.

[kmillikin]

Kernel doesn't have private names. It has some names that are strings and it has some names that are strings paired with an opaque token.

This doesn't have anything to do with privacy in Kernel. It's just a mechanism for a class to have more than one member with the same textual name. In the first example, class A has a field named the equivalent of (1, "_field") and it has a getter named (2, "_field") which is a noSuchMethod forwarder (there is also a setter NSM forwarder).

The property get new A()._field has name (1, "_field") and it is statically dispatched to an implementation of A.(1, "_field") via its interfaceTarget.

As @stefantsov says, we really do want these NSM forwarders in class A because you can pass an instance of A into the library b.dart as an I.

Dart2js will need a mechanism to allow these to be distinct members. It probably works to decorate the name with the token (it's a library reference in fact) somehow.

[eernstg]

I just created an area-language & area-specification issue about this, #33725.

Kernel doesn't have private names.

Right, but from the perspective of Dart there is no way in library L1 to express a private name _n declared in L2 distinct from L1, and the ability to specify the behavior of an instance method with that inaccessible name via noSuchMethod appears to me to be a privacy violation (there's no way we could write such a method implementation in L1 directly).

However, I just realized that there is no way we can avoid it, short of outlawing such classes, so we should of course maintain the underlying invariant that every statically checked invocation will find a corresponding method implementation to invoke—no matter whether it's a violation or not, we can't avoid it.

[sigmundch]

Thank you so much everyone for the healthy discussion and clarifications!

I filed #33732 to ensure dart2js preserves the right semantics, I'm closing this bug as there is no other action needed from the CFE team.

Thanks again!