sdk
sdk copied to clipboard
dart-lang
Don't delegate foreign private names to `noSuchMethod`
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 https://github.com/dart-lang/language/issues/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
@visibleForTestingto 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
@visibleForTestingto 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.
This would be really nice to land. Can we run a test to double check impact on flutter and google tests?
@srawlins - any concerns re mocking?
This would be really nice to land. Can we run a test to double check impact on flutter and google tests?
@vsmenon and I spoke about this offline. It's complicated, but here's a summary:
- I've thoroughly tested the effect on mockito mocks in Google's internal codebase, and found 15 affected classes (2 in code that's imported from open source, 13 in closed source). The two open source classes are tracked by https://github.com/flutter/flutter/issues/109237 (already closed) and https://github.com/flutter/flutter/issues/109339 (in progress). I've prototyped fixes for the remaining closed source issues and will be sending them for code review soon.
- I've tested the effect on code other than mocks in Google's internal codebase, however due to some technical limitations I haven't yet been able to test code compiled using dart2js, or code using flutter. In the code I was able to test, I didn't find any additional breakages beyond the 15 classes mentioned above. I'm working on filling in the gaps in my testing, and I will post an update when I have more information.
- I haven't yet tested the effect on flutter open source tests. I will test that ASAP.
How about not delegating any private names to noSuchMethod at all? It may be a bigger breaking change, but perhaps the semantic is more sane?
I mean, why is this even legal to write in Dart today:
class A {
void _foo() {}
void bar() {
_foo();
}
}
void main() {
dynamic x = A();
x._foo(); // why not make it illegal to call private stuff on dynamic variables everywhere?
}
What if assigning to a dynamic variable always (even in the same file) prevented you from accessing the private interface?
@nielsenko - yeah, I suspect that'd be a much bigger breaking change to land. Perhaps worth filing as a separate suggestion here though: https://github.com/dart-lang/language/issues
This would be really nice to land. Can we run a test to double check impact on flutter and google tests?
@vsmenon and I spoke about this offline. It's complicated, but here's a summary:
- I've thoroughly tested the effect on mockito mocks in Google's internal codebase, and found 15 affected classes (2 in code that's imported from open source, 13 in closed source). The two open source classes are tracked by
NavigatorObserver._navigatorneeds reworking due to an upcoming dart language change flutter/flutter#109237 (already closed) andPlatformInterface._instanceTokenneeds reworking due to an upcoming dart language change flutter/flutter#109339 (in progress). I've prototyped fixes for the remaining closed source issues and will be sending them for code review soon.- I've tested the effect on code other than mocks in Google's internal codebase, however due to some technical limitations I haven't yet been able to test code compiled using dart2js, or code using flutter. In the code I was able to test, I didn't find any additional breakages beyond the 15 classes mentioned above. I'm working on filling in the gaps in my testing, and I will post an update when I have more information.
- I haven't yet tested the effect on flutter open source tests. I will test that ASAP.
Thanks to a lot of help from @a-siva, @iinozemtsev, @godofredoc, and @athomas, this change has now been tested throughly, both in Google's internal codebase and in the open source tests for Flutter and Dart. The only other breakage, other than those I've mentioned previously, was a breakage to flutter plugins. I've prepared a fix for that (https://github.com/flutter/plugins/pull/6318), but @stuartmorgan is going to work on a better fix, with an ETA of Monday.
All the other fixes are either landed or making good progress. So I think we are good to move forward with this change.
Considering,
Thanks to a lot of help from @a-siva, @iinozemtsev, @godofredoc, and @athomas, this change has now been tested throughly, both in Google's internal codebase and in the open source tests for Flutter and Dart.
LGTM.
