[dart2js] Check non-nullable native call values
When `--null-assertions` is enabled, values returned from @Native
methods and getters with non-nullable return types are checked.
Change-Id: I4b38e651a9a6b370ebed5420a3cfac0a53cfeb60
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/158527
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Srujan Gaddam <srujzs@google.com>
diff --git a/pkg/compiler/lib/src/options.dart b/pkg/compiler/lib/src/options.dart
index 2871956..f4bc3a6 100644
--- a/pkg/compiler/lib/src/options.dart
+++ b/pkg/compiler/lib/src/options.dart
@@ -250,6 +250,10 @@
/// operate with a stronger guarantee.
bool enableNullAssertions = false;
+ /// Whether to generate code asserting that non-nullable return values of
+ /// `@Native` methods are checked for being non-null.
+ bool enableNativeReturnNullAssertions = false;
+
/// Whether to generate a source-map file together with the output program.
bool generateSourceMap = true;
@@ -616,6 +620,11 @@
if (_deferClassTypes) deferClassTypes = true;
if (_noDeferClassTypes) deferClassTypes = false;
+
+ if (enableNullAssertions) {
+ // TODO(sra): Add a command-line flag to control this independently.
+ enableNativeReturnNullAssertions = true;
+ }
}
/// Returns `true` if warnings and hints are shown for all packages.
diff --git a/pkg/compiler/lib/src/ssa/builder_kernel.dart b/pkg/compiler/lib/src/ssa/builder_kernel.dart
index 24104e5..dfc216c 100644
--- a/pkg/compiler/lib/src/ssa/builder_kernel.dart
+++ b/pkg/compiler/lib/src/ssa/builder_kernel.dart
@@ -1540,8 +1540,18 @@
push(HInvokeExternal(targetElement, inputs, returnType, nativeBehavior,
sourceInformation: null));
- // TODO(johnniwinther): Provide source information.
HInstruction value = pop();
+ // TODO(johnniwinther): Provide source information.
+ if (options.enableNativeReturnNullAssertions) {
+ if (_isNonNullableByDefault(functionNode)) {
+ DartType type = _getDartTypeIfValid(functionNode.returnType);
+ if (dartTypes.isNonNullableIfSound(type)) {
+ push(HNullCheck(value, _abstractValueDomain.excludeNull(returnType),
+ sticky: true));
+ value = pop();
+ }
+ }
+ }
if (targetElement.isSetter) {
_closeAndGotoExit(HGoto(_abstractValueDomain));
} else {
diff --git a/pkg/compiler/lib/src/ssa/nodes.dart b/pkg/compiler/lib/src/ssa/nodes.dart
index 9e5cdc5..72631d5 100644
--- a/pkg/compiler/lib/src/ssa/nodes.dart
+++ b/pkg/compiler/lib/src/ssa/nodes.dart
@@ -1548,18 +1548,20 @@
: super(inputs, type);
}
-/// A [HCheck] instruction is an instruction that might do a dynamic
-/// check at runtime on another instruction. To have proper instruction
-/// dependencies in the graph, instructions that depend on the check
-/// being done reference the [HCheck] instruction instead of the
-/// instruction itself.
+/// A [HCheck] instruction is an instruction that might do a dynamic check at
+/// runtime on an input instruction. To have proper instruction dependencies in
+/// the graph, instructions that depend on the check being done reference the
+/// [HCheck] instruction instead of the input instruction.
abstract class HCheck extends HInstruction {
HCheck(inputs, type) : super(inputs, type) {
setUseGvn();
}
+
HInstruction get checkedInput => inputs[0];
+
@override
bool isJsStatement() => true;
+
@override
bool canThrow(AbstractValueDomain domain) => true;
@@ -3607,10 +3609,12 @@
/// field getter or setter when the receiver might be null. In these cases, the
/// [selector] and [field] members are assigned.
class HNullCheck extends HCheck {
+ // A sticky check is not optimized away on the basis of the input type.
+ final bool sticky;
Selector selector;
FieldEntity field;
- HNullCheck(HInstruction input, AbstractValue type)
+ HNullCheck(HInstruction input, AbstractValue type, {this.sticky = false})
: super(<HInstruction>[input], type);
@override
@@ -3632,6 +3636,7 @@
bool dataEquals(HNullCheck other) => true;
bool isRedundant(JClosedWorld closedWorld) {
+ if (sticky) return false;
AbstractValueDomain abstractValueDomain = closedWorld.abstractValueDomain;
AbstractValue inputType = checkedInput.instructionType;
return abstractValueDomain.isNull(inputType).isDefinitelyFalse;
diff --git a/pkg/compiler/lib/src/ssa/optimize.dart b/pkg/compiler/lib/src/ssa/optimize.dart
index 9ff7388..0d00347 100644
--- a/pkg/compiler/lib/src/ssa/optimize.dart
+++ b/pkg/compiler/lib/src/ssa/optimize.dart
@@ -908,7 +908,7 @@
AbstractValueFactory.fromNativeBehavior(nativeBehavior, _closedWorld);
HInstruction receiver = node.inputs.last; // Drop interceptor.
receiver = maybeGuardWithNullCheck(receiver, node, null);
- HInvokeExternal result = HInvokeExternal(
+ HInstruction result = HInvokeExternal(
method, [receiver], returnType, nativeBehavior,
sourceInformation: node.sourceInformation);
_registry.registerStaticUse(StaticUse.methodInlining(method, null));
@@ -919,7 +919,25 @@
result.sideEffects.setDependsOnSomething();
result.sideEffects.clearAllSideEffects();
result.setUseGvn();
- return result;
+
+ return maybeAddNativeReturnNullCheck(node, result, method);
+ }
+
+ HInstruction maybeAddNativeReturnNullCheck(
+ HInstruction node, HInstruction replacement, FunctionEntity method) {
+ if (_options.enableNativeReturnNullAssertions) {
+ if (method.library.isNonNullableByDefault) {
+ FunctionType type =
+ _closedWorld.elementEnvironment.getFunctionType(method);
+ if (_closedWorld.dartTypes.isNonNullableIfSound(type.returnType)) {
+ node.block.addBefore(node, replacement);
+ replacement = HNullCheck(replacement,
+ _abstractValueDomain.excludeNull(replacement.instructionType),
+ sticky: true);
+ }
+ }
+ }
+ return replacement;
}
// Try to 'inline' an instance setter call to a known native or js-interop
@@ -1025,14 +1043,15 @@
AbstractValueFactory.fromNativeBehavior(nativeBehavior, _closedWorld);
HInstruction receiver = inputs[1];
receiver = maybeGuardWithNullCheck(receiver, node, null);
- HInvokeExternal result = HInvokeExternal(
+ HInstruction result = HInvokeExternal(
method,
[receiver, ...inputs.skip(2)], // '2': Drop interceptor and receiver.
returnType,
nativeBehavior,
sourceInformation: node.sourceInformation);
_registry.registerStaticUse(StaticUse.methodInlining(method, null));
- return result;
+
+ return maybeAddNativeReturnNullCheck(node, result, method);
}
@override
diff --git a/tests/dart2js/native/null_assertions_opt_in_lib.dart b/tests/dart2js/native/null_assertions_opt_in_lib.dart
new file mode 100644
index 0000000..e08c788
--- /dev/null
+++ b/tests/dart2js/native/null_assertions_opt_in_lib.dart
@@ -0,0 +1,58 @@
+// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'native_testing.dart';
+
+abstract class Interface {
+ int get size;
+ String get name;
+ String? get optName;
+ int method1();
+ String method2();
+}
+
+@Native("AAA")
+class AAA implements Interface {
+ int get size native;
+ String get name native;
+ String? get optName native;
+ int method1() native;
+ String method2() native;
+}
+
+/// Returns an 'AAA' object that satisfies the interface.
+AAA makeA() native;
+
+/// Returns an 'AAA' object where each method breaks the interface's contract.
+AAA makeAX() native;
+
+void setup() {
+ JS('', r"""
+(function(){
+ function AAA(s,n,m1,m2) {
+ this.size = s;
+ this.name = n;
+ this.optName = n;
+ this._m1 = m1;
+ this._m2 = m2;
+ }
+ AAA.prototype.method1 = function(){return this._m1};
+ AAA.prototype.method2 = function(){return this._m2};
+
+ makeA = function() {return new AAA(100, 'Albert', 200, 'amazing!')};
+ makeAX = function() {return new AAA(void 0, void 0, void 0, void 0)};
+
+ self.nativeConstructor(AAA);
+})()""");
+}
+
+class BBB implements Interface {
+ int get size => 300;
+ String get name => 'Brenda';
+ String? get optName => name;
+ int method1() => 400;
+ String method2() => 'brilliant!';
+}
+
+List<Interface> items = [makeA(), BBB()];
diff --git a/tests/dart2js/native/null_assertions_test.dart b/tests/dart2js/native/null_assertions_test.dart
new file mode 100644
index 0000000..0a07785
--- /dev/null
+++ b/tests/dart2js/native/null_assertions_test.dart
@@ -0,0 +1,74 @@
+// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Requirements=nnbd-weak
+// dart2jsOptions=--null-assertions
+
+// @dart=2.8
+
+// Test that `--null-assertions` injects null-checks on the returned value of
+// native methods with a non-nullable return type in an opt-in library.
+
+import 'native_testing.dart';
+import 'null_assertions_opt_in_lib.dart' as lib;
+
+// The 'Interface' version of the code is passed both native and Dart objects,
+// so there will be an interceptor dispatch to the method. This tests that the
+// null-check exists in the forwarding method.
+//
+// The 'AAA' version of the code is passed only objects of a single native
+// class, so the native method can be inlined (which happens in the optimizer).
+// This tests that the null-check exists in the 'inlined' code.
+
+@pragma('dart2js:noInline')
+String describeInterface(lib.Interface o) {
+ return '${o.name} ${o.method2()} ${o.size} ${o.method1()}';
+}
+
+@pragma('dart2js:noInline')
+String describeAAA(lib.AAA o) {
+ return '${o.name} ${o.method2()} ${o.size} ${o.method1()}';
+}
+
+@pragma('dart2js:noInline')
+void checkOptNameInterface(lib.Interface o, dynamic expected) {
+ Expect.equals(expected, o.optName);
+}
+
+@pragma('dart2js:noInline')
+void checkOptNameAAA(lib.AAA o, dynamic expected) {
+ Expect.equals(expected, o.optName);
+}
+
+const expectedA = 'Albert amazing! 100 200';
+const expectedB = 'Brenda brilliant! 300 400';
+
+void main() {
+ nativeTesting();
+ lib.setup();
+ lib.AAA a = lib.makeA();
+ lib.BBB b = lib.BBB();
+
+ Expect.equals(expectedA, describeInterface(a));
+ Expect.equals(expectedB, describeInterface(b));
+
+ Expect.equals(expectedA, describeAAA(a));
+
+ lib.AAA x = lib.makeAX(); // This object returns `null`!
+ Expect.throws(() => describeInterface(x));
+ Expect.throws(() => describeAAA(x));
+
+ Expect.throws(() => x.name);
+ Expect.throws(() => x.size);
+ Expect.throws(() => x.method1());
+ Expect.throws(() => x.method2());
+
+ // Now test that a nullable return type does not have a check.
+ checkOptNameInterface(a, 'Albert');
+ checkOptNameInterface(b, 'Brenda');
+ checkOptNameInterface(x, null);
+
+ checkOptNameAAA(a, 'Albert');
+ checkOptNameAAA(x, null);
+}
