Don't delegate foreign private names to noSuchMethod.

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.

This change closes a loophole in Dart's privacy system, and paves the way for
a future implementation of promotion for private final fields (see
dart-lang/language#2020).

Bug: #49687
Change-Id: Ie55805e0fc77dc39713761a80a42c28bd0504722
Tested: language tests
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/255640
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Lasse Nielsen <[email protected]>

Author: stereotype441

CHANGELOG.md

@@ -39,6 +39,21 @@
 [#49635]: https://github.com/dart-lang/sdk/issues/49635
 [#49878]: https://github.com/dart-lang/sdk/issues/49878
 
+- **Breaking Change** [#49687][]: Don't delegate inaccessible private names to
+  `noSuchMethod`. If a concrete class implements an interface containing a
+  member with a name that's private to different library, and does not inherit
+  an implementation of that interface member, a invocation of that member will
+  result in an exception getting thrown.  Previously, such attempts would result
+  in the call being diverted to the `noSuchMethod` method.
+
+  This change closes a loophole in Dart's privacy system, where another library
+  can provide a different implementation of a supposedly private member using
+  `noSuchMethod`, and paves the way for a future implementation of promotion for
+  private final fields (see [#2020][]).
+
+[#49687]: https://github.com/dart-lang/sdk/issues/49687
+[#2020]: https://github.com/dart-lang/language/issues/2020
+
 ### Libraries
 
 #### `dart:convert`

pkg/dart2wasm/lib/target.dart

@@ -263,7 +263,7 @@ class WasmTarget extends Target {
       bool isStatic = false,
       bool isConstructor = false,
       bool isTopLevel = false}) {
-    return ConstructorInvocation(
+    return StaticInvocation(
         coreTypes.noSuchMethodErrorDefaultConstructor,
         Arguments(
             [receiver, _instantiateInvocation(coreTypes, name, arguments)]));

pkg/front_end/lib/src/fasta/source/source_class_builder.dart

@@ -1483,8 +1483,12 @@ class SourceClassBuilder extends ClassBuilderImpl
             _transformProcedureToNoSuchMethodForwarder(
                 noSuchMethod, target, member as Procedure);
           } else {
-            Procedure memberSignature =
-                combinedMemberSignature.createMemberFromSignature()!;
+            // The reason we don't copy the location is to ensure that the stack
+            // trace for throwing no-such-method forwarders point to the class
+            // in which they are inserted and not to the declaration of the
+            // missing member.
+            Procedure memberSignature = combinedMemberSignature
+                .createMemberFromSignature(copyLocation: false)!;
             _transformProcedureToNoSuchMethodForwarder(
                 noSuchMethod, target, memberSignature);
             cls.procedures.add(memberSignature);
@@ -1593,31 +1597,56 @@ class SourceClassBuilder extends ClassBuilderImpl
       Procedure noSuchMethodInterface,
       KernelTarget target,
       Procedure procedure) {
+    bool shouldThrow = false;
+    Name procedureName = procedure.name;
+    if (procedureName.isPrivate) {
+      Library procedureNameLibrary = procedureName.library!;
+      // If the name is defined in a different library than the library we're
+      // synthesizing a forwarder for, then the forwarder must throw.  This
+      // avoids surprising users by ensuring that all non-throwing
+      // implementations of a private name can be found solely by looking at the
+      // library in which the name is defined; it also avoids soundness holes in
+      // field promotion.
+      if (procedureNameLibrary.compareTo(procedure.enclosingLibrary) != 0) {
+        shouldThrow = true;
+      }
+    }
+    Expression result;
     String prefix = procedure.isGetter
         ? 'get:'
         : procedure.isSetter
             ? 'set:'
             : '';
-    String invocationName = prefix + procedure.name.text;
+    String invocationName = prefix + procedureName.text;
     if (procedure.isSetter) invocationName += '=';
+    CoreTypes coreTypes = target.loader.coreTypes;
     Expression invocation = target.backendTarget.instantiateInvocation(
-        target.loader.coreTypes,
+        coreTypes,
         new ThisExpression(),
         invocationName,
         new Arguments.forwarded(procedure.function, libraryBuilder.library),
         procedure.fileOffset,
         /*isSuper=*/ false);
-    Expression result = new InstanceInvocation(InstanceAccessKind.Instance,
-        new ThisExpression(), noSuchMethodName, new Arguments([invocation]),
-        functionType: noSuchMethodInterface.getterType as FunctionType,
-        interfaceTarget: noSuchMethodInterface)
-      ..fileOffset = procedure.fileOffset;
-    if (procedure.function.returnType is! VoidType) {
-      result = new AsExpression(result, procedure.function.returnType)
-        ..isTypeError = true
-        ..isForDynamic = true
-        ..isForNonNullableByDefault = libraryBuilder.isNonNullableByDefault
+    if (shouldThrow) {
+      // Build `throw new NoSuchMethodError(this, invocation)`.
+      result = new Throw(new StaticInvocation(
+          coreTypes.noSuchMethodErrorDefaultConstructor,
+          new Arguments([new ThisExpression(), invocation])))
         ..fileOffset = procedure.fileOffset;
+    } else {
+      // Build `this.noSuchMethod(invocation)`.
+      result = new InstanceInvocation(InstanceAccessKind.Instance,
+          new ThisExpression(), noSuchMethodName, new Arguments([invocation]),
+          functionType: noSuchMethodInterface.getterType as FunctionType,
+          interfaceTarget: noSuchMethodInterface)
+        ..fileOffset = procedure.fileOffset;
+      if (procedure.function.returnType is! VoidType) {
+        result = new AsExpression(result, procedure.function.returnType)
+          ..isTypeError = true
+          ..isForDynamic = true
+          ..isForNonNullableByDefault = libraryBuilder.isNonNullableByDefault
+          ..fileOffset = procedure.fileOffset;
+      }
     }
     procedure.function.body = new ReturnStatement(result)
       ..fileOffset = procedure.fileOffset

pkg/front_end/lib/src/fasta/source/source_loader.dart

@@ -2706,7 +2706,7 @@ abstract class pragma {
 class AbstractClassInstantiationError {}
 
 class NoSuchMethodError {
-  NoSuchMethodError.withInvocation(receiver, invocation);
+  factory NoSuchMethodError.withInvocation(receiver, invocation) => throw '';
 }
 
 class StackTrace {}

pkg/front_end/test/spell_checking_list_code.txt

@@ -618,6 +618,7 @@ histogram
 history
 hit
 hoc
+holes
 hook
 hopefully
 horizontal
@@ -1393,6 +1394,7 @@ superinterfaces
 supernode
 supers
 suppose
+surprising
 suspended
 svg
 sw

pkg/front_end/test/spell_checking_list_tests.txt

@@ -102,6 +102,7 @@ cafebabe
 calloc
 camel
 capitalized
+carefully
 categorization
 causal
 cb
@@ -362,12 +363,14 @@ insights
 instrument
 insufficient
 intdiv
+interactions
 interactive
 interchangeable
 internet
 interpolate
 introducer
 inv
+involves
 ioo
 ish
 it'll
@@ -486,6 +489,7 @@ partfoo
 party
 pause
 paused
+pays
 pc
 periodic
 periodically
@@ -560,6 +564,7 @@ risky
 rk
 row
 rows
+ruin
 runtimes
 rv
 sanitize
@@ -598,6 +603,7 @@ smoke
 snull
 somehow
 sorter
+soundly
 spans
 spell
 spellcheck
@@ -694,6 +700,7 @@ untransformed
 untrimmed
 unusual
 unversioned
+upgrade
 upload
 upward
 uuid

pkg/front_end/testcases/dart2js/generic_usage_type_variable.dart.weak.outline.expect

@@ -82,15 +82,15 @@ static method main() → dynamic
 Extra constant evaluation status:
 Evaluated: ListLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:22:35 -> ListConstant(const <List<dynamic>*>[])
 Evaluated: ConstructorTearOff @ org-dartlang-testcase:///generic_usage_type_variable.dart:18:16 -> ConstructorTearOffConstant(C.name2)
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:26:17 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:32:22 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:25:14 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:24:9 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:35:19 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:33:35 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:25:14 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:24:9 -> MapConstant(const <String*, dynamic>{})
-Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:34:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
+Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:39:7 -> MapConstant(const <String*, dynamic>{})
 Evaluated: ListLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:15:23 -> ListConstant(const <C*>[])
 Evaluated: MapLiteral @ org-dartlang-testcase:///generic_usage_type_variable.dart:16:24 -> MapConstant(const <Type*, Type*>{dynamic: dynamic})
 Extra constant evaluation: evaluated: 93, effectively constant: 13

pkg/front_end/testcases/enhanced_enums/enum_as_supertype_error.dart.strong.expect

@@ -98,17 +98,17 @@ class A extends core::Enum {
     ;
   get foo() → core::int
     return this.{core::Enum::index}{core::int};
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 class B extends core::Object implements core::Enum {
   synthetic constructor •() → self::B
     : super core::Object::•()
     ;
   get foo() → core::int
     return this.{core::Enum::index}{core::int};
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 abstract class EnumInterface extends core::Object implements core::Enum {
   synthetic constructor •() → self::EnumInterface
@@ -121,8 +121,8 @@ class EnumClass extends self::EnumInterface {
     ;
   get index() → core::int
     return 0;
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 abstract class AbstractEnumClass extends self::EnumInterface {
   synthetic constructor •() → self::AbstractEnumClass
@@ -133,8 +133,8 @@ class EnumClass2 extends self::AbstractEnumClass {
   synthetic constructor •() → self::EnumClass2
     : super self::AbstractEnumClass::•()
     ;
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 abstract class EnumMixin extends core::Enum /*isMixinDeclaration*/  {
 }
@@ -152,8 +152,8 @@ class EnumClass3 extends self::AbstractEnumClass2 {
   synthetic constructor •() → self::EnumClass3
     : super self::AbstractEnumClass2::•()
     ;
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 static method main() → dynamic {}
 

pkg/front_end/testcases/enhanced_enums/enum_as_supertype_error.dart.strong.transformed.expect

@@ -98,17 +98,17 @@ class A extends core::Enum {
     ;
   get foo() → core::int
     return this.{core::Enum::index}{core::int};
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 class B extends core::Object implements core::Enum {
   synthetic constructor •() → self::B
     : super core::Object::•()
     ;
   get foo() → core::int
     return this.{core::Enum::index}{core::int};
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 abstract class EnumInterface extends core::Object implements core::Enum {
   synthetic constructor •() → self::EnumInterface
@@ -121,8 +121,8 @@ class EnumClass extends self::EnumInterface {
     ;
   get index() → core::int
     return 0;
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 abstract class AbstractEnumClass extends self::EnumInterface {
   synthetic constructor •() → self::AbstractEnumClass
@@ -133,8 +133,8 @@ class EnumClass2 extends self::AbstractEnumClass {
   synthetic constructor •() → self::EnumClass2
     : super self::AbstractEnumClass::•()
     ;
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 abstract class EnumMixin extends core::Enum /*isMixinDeclaration*/  {
 }
@@ -152,8 +152,8 @@ class EnumClass3 extends self::AbstractEnumClass2 {
   synthetic constructor •() → self::EnumClass3
     : super self::AbstractEnumClass2::•()
     ;
-  no-such-method-forwarder get /* from org-dartlang-sdk:///sdk/lib/core/enum.dart */ _name() → core::String
-    return this.{core::Object::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::String;
+  no-such-method-forwarder get _name() → core::String
+    return throw core::NoSuchMethodError::withInvocation(this, new core::_InvocationMirror::_withType(#C1, 1, #C2, #C3, core::Map::unmodifiable<core::Symbol*, dynamic>(#C4)));
 }
 static method main() → dynamic {}