Skip to content

Don't delegate foreign private names to noSuchMethod #49687

Closed
@stereotype441

Description

@stereotype441

Change

If a concrete class implements an interface containing a name that's private to a different library, any attempt to invoke that name will result in an exception getting thrown.

Previously, such attempts would result in the call being diverted to noSuchMethod.

Rationale

1. Closes a loophole in Dart's privacy system

Without this change, noSuchMethod could be used to provide an alternative behavior for a private name belonging to a different library. In effect, this is a loophole in the privacy system, because it's natural for the author of a library to assume that any time they invoke a private name, they are invoking their own code. For example:

// lib1.dart
class A {
  void _foo() {
    print('_foo() invoked');
  }
}

void useA(A a) {
  // We think this prints "_foo() invoked" because the name `_foo` is private to
  // this library, and there's only one declaration of it.
  a._foo();
}
// main.dart
import "lib1.dart";

// Except this sneaky class overrides `_foo` even though it belongs to a
// different library, by being clever with `noSuchMethod`.
class E implements A {
  @override
  dynamic noSuchMethod(_) {
    print('sneaky override of _foo() invoked');
    return null;
  }
}

void main() {
  var e = new E();
  useA(e);
}

Without the proposed change, running this code prints the surprising message sneaky override of _foo() invoked, because when useA attempts to call _foo, the call gets dispatched to E.noSuchMethod. With the change, running this code will cause an exception to be thrown.

2. Makes field promotion possible

This change will allow us to implement type promotion of fields in a way that is sound, without forcing the compiler to analyze the user's whole program. For example, once field promotion is implemented, the following will be possible:

class C {
  final int? _i;
  C(this._i);
}

void bar(C c) {
  if (c._i != null) {
    // No need for `!` after `c._i`; the check above promoted it to non-null `int`.
    print(c._i + 1);
  }
}

Without the change, the above code would be unsound, because it would be possible for code in another library to override _i using noSuchMethod, changing its behavior to something that returns null sometimes and non-null other times.

See dart-lang/language#2020 for more details.

Impact

This change breaks an uncommon mocking pattern, where a library contains a class with a private member, and that private member is invoked from static code, or code in some other class. For example:

// lib.dart
class C {
  void _init() {
    print('C initialized');
  }
  bool _inUse = false;
}

void use(C c) {
  c._init();
  c._inUse = true;
}
// main.dart
import 'package:mockito/annotations.dart';
import 'package:test/test.dart';
import 'lib.dart';

@GenerateMocks([],
    customMocks: [MockSpec<C>(onMissingStub: OnMissingStub.returnDefault)])
import 'main.mocks.dart';

void main() {
  test('test', () {
    var c = MockC();
    use(c);
  });
}

This code works today because even though mockito code generation cannot genrate a stub for the _init method and the _inUse= setter (because they are private to another library), it's still able to generate an implementation of noSuchMethod that intercepts the calls to them. The mock spec onMissingStub: OnMissingStub.returnDefault ensures that these intercepted calls will return Null, so the test passes.

After the change, a test like this one will fail because the attempts to call _init and _inUse= will result in an exception being thrown.

Based on studying Google's internal codebase, I believe that this pattern arises quite rarely.

Mitigation

For the rare users with mocks affected by this change, there are several options:

  • Create a mixin that provides an alternative implementation of the private member, and include that mixin in the mock generation. This mixin can be marked @visibleForTesting to discourage clients from using it. In other words, the example from the "Impact" section could be rewritten like this:
// lib.dart
import 'package:meta/meta.dart';

class C {
  // ... unchanged ...
}

@visibleForTesting
mixin MockCMixin implements C {
  @override
  void _init() {}
  @override
  void set _isUse(bool value) {}
}

void use(C c) {
  // ... unchanged ...
}
// main.dart
import 'package:mockito/annotations.dart';
import 'package:test/test.dart';
import 'lib.dart';

@GenerateMocks([], customMocks: [
  MockSpec<C>(
      onMissingStub: OnMissingStub.returnDefault, mixingIn: [MockCMixin])
])
import 'main.mocks.dart';

void main() {
  // ... unchanged ...
}
  • Make the class member in question public, so that it can be be mocked normally. The user may want to mark the class member as @visibleForTesting to discourage clients from accessing it. In other words, the example from the "Impact" section could be rewritten like this:
// lib.dart
import 'package:meta/meta.dart';

class C {
  @visibleForTesting
  void init() {
    print('C initialized');
  }
  @visibleForTesting
  bool inUse = false;
}

void use(C c) {
  c.init();
  c.inUse = true;
}
// main.dart
// ... unchanged ...
  • If the class member is a field, replace it with a private static expando. If the class member is not a field, replace it with a private extension method. In other words, the example from the "Impact" section could be rewritten like this:
// lib.dart
class C {
  static final _inUse = Expando<bool>();
}

extension on C {
  void _init() {
    print('C initialized');
  }
}

void use(C c) {
  c._init();
  C._inUse[c] = true;
}
// main.dart
// ... unchanged ...
  • Rewrite the test so that it doesn't invoke a private member of a mock. In some cases the mock object may not be needed at all; the test can just use the real object, or a custom class that extends it. In other cases, additional mocking can help.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area-languageDart language related items (some items might be better tracked at github.com/dart-lang/language).breaking-change-approved

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions