Revert "[ddc] Add non-null assertions when setting fields"
This reverts commit c590001ef043a2bfd00927743a548d20f72da941.
Reason for revert: Breaks google3 (b/247639927)
Original change's description:
> [ddc] Add non-null assertions when setting fields
>
> Re-land previously reverted change:
> https://dart-review.googlesource.com/c/sdk/+/258780
>
> Adds additional tests and more checks to ensure assertions are only
> added in libraries that have been migrated to null safety.
>
> Issue: https://github.com/dart-lang/sdk/issues/49918
> Change-Id: If0b9bca9fe0d7db5e15023fe03ccac891af568e8
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/259120
> Reviewed-by: Sigmund Cherem <sigmund@google.com>
> Commit-Queue: Nicholas Shahan <nshahan@google.com>
TBR=sigmund@google.com,nshahan@google.com,annagrin@google.com
Change-Id: Iee2d33005cbabfacab7fc5c45046c6eeb8e31b01
Issue: https://github.com/dart-lang/sdk/issues/49918
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/260106
Reviewed-by: Oleh Prypin <oprypin@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Commit-Queue: Sigmund Cherem <sigmund@google.com>
diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart
index 4ca915c..4962bdc 100644
--- a/pkg/dev_compiler/lib/src/kernel/compiler.dart
+++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart
@@ -2170,21 +2170,18 @@
var jsGetter = js_ast.Method(name, getter, isGetter: true)
..sourceInformation = _nodeStart(field);
- var body = <js_ast.Statement>[];
- var value = _emitIdentifier('value');
- if (_requiresExtraNullCheck(field.setterType, field.annotations)) {
- body.add(
- _nullSafetyParameterCheck(value, field.location, field.name.text));
- }
var args = field.isFinal
- ? [js_ast.Super(), name, value]
- : [
- js_ast.This(),
- virtualFieldSymbol,
- if (isCovariantField(field)) _emitCast(value, field.type) else value
- ];
- body.add(js.call('#[#] = #', args).toStatement());
- var jsSetter = js_ast.Method(name, js_ast.Fun([value], js_ast.Block(body)),
+ ? [js_ast.Super(), name]
+ : [js_ast.This(), virtualFieldSymbol];
+
+ js_ast.Expression value = _emitIdentifier('value');
+ if (!field.isFinal && isCovariantField(field)) {
+ value = _emitCast(value, field.type);
+ }
+ args.add(value);
+
+ var jsSetter = js_ast.Method(
+ name, js.fun('function(value) { #[#] = #; }', args),
isSetter: true)
..sourceInformation = _nodeStart(field);
@@ -2371,16 +2368,10 @@
member.fileOffset,
member.name.text.length));
if (!member.isFinal && !member.isConst) {
- var body = <js_ast.Statement>[];
- var value = _emitIdentifier('value');
- if (_requiresExtraNullCheck(member.setterType, member.annotations)) {
- body.add(_nullSafetyParameterCheck(
- value, member.location, member.name.text));
- }
- // Even when no null check is present a dummy setter is still required
- // to indicate writeable.
+ // TODO(jmesserly): currently uses a dummy setter to indicate
+ // writable.
accessors.add(js_ast.Method(
- access, js_ast.Fun([value], js_ast.Block(body)),
+ access, js.call('function(_) {}') as js_ast.Fun,
isSetter: true));
}
} else if (member is Procedure) {
@@ -3617,46 +3608,6 @@
bool _mustBeNonNullable(DartType type) =>
type.nullability == Nullability.nonNullable;
- /// Returns `true` when an additional null check is needed because of the
- /// null safety compile mode, the null safety migration status of the current
- /// library and the provided [type] with its [annotations].
- bool _requiresExtraNullCheck(DartType type, List<Expression> annotations) =>
- !_options.soundNullSafety &&
- // Libraries that haven't been migrated to null safety represent
- // non-nullable as legacy.
- _currentLibrary!.nonNullable == Nullability.nonNullable &&
- _mustBeNonNullable(type) &&
- !_annotatedNotNull(annotations);
-
- /// Returns a null check for [value] that if fails produces an error message
- /// containing the [location] and [name] of the original value being checked.
- ///
- /// This is used to generate checks for non-nullable parameters when running
- /// with weak null safety. The checks can be silent, warn, or throw, depending
- /// on the flags set in the SDK at runtime.
- js_ast.Statement _nullSafetyParameterCheck(
- js_ast.Identifier value, Location? location, String? name) {
- // TODO(nshahan): Remove when weak mode null safety assertions are no longer
- // supported.
- // The check on `field.setterType` is per:
- // https://github.com/dart-lang/language/blob/master/accepted/2.12/nnbd/feature-specification.md#automatic-debug-assertion-insertion
- var condition = js.call('# == null', [value]);
- // Offsets are not available for compiler-generated variables
- // Get the best available location even if the offset is missing.
- // https://github.com/dart-lang/sdk/issues/34942
- return js.statement(' if (#) #;', [
- condition,
- runtimeCall('nullFailed(#, #, #, #)', [
- location != null
- ? _cacheUri(location.file.toString())
- : js_ast.LiteralNull(),
- js.number(location?.line ?? -1),
- js.number(location?.column ?? -1),
- js.escapedString('$name')
- ])
- ]);
- }
-
/// Emits argument initializers, which handles optional/named args, as well
/// as generic type checks needed due to our covariance.
List<js_ast.Statement> _emitArgumentInitializers(
@@ -3687,8 +3638,30 @@
if (_annotatedNullCheck(p.annotations)) {
body.add(_nullParameterCheck(jsParam));
- } else if (_requiresExtraNullCheck(p.type, p.annotations)) {
- body.add(_nullSafetyParameterCheck(jsParam, p.location, p.name));
+ } else if (!_options.soundNullSafety &&
+ _mustBeNonNullable(p.type) &&
+ !_annotatedNotNull(p.annotations)) {
+ // TODO(vsm): Remove if / when CFE does this:
+ // https://github.com/dart-lang/sdk/issues/40597
+ // The check on `p.type` is per:
+ // https://github.com/dart-lang/language/blob/master/accepted/future-releases/nnbd/feature-specification.md#automatic-debug-assertion-insertion
+ var condition = js.call('# == null', [jsParam]);
+ // Offsets are not available for compiler-generated variables
+ // Get the best available location even if the offset is missing.
+ // https://github.com/dart-lang/sdk/issues/34942
+ var location = p.location;
+ var check = js.statement(' if (#) #;', [
+ condition,
+ runtimeCall('nullFailed(#, #, #, #)', [
+ location != null
+ ? _cacheUri(location.file.toString())
+ : js_ast.LiteralNull(),
+ js.number(location?.line ?? -1),
+ js.number(location?.column ?? -1),
+ js.escapedString('${p.name}')
+ ])
+ ]);
+ body.add(check);
}
}
diff --git a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart
index c942721..f028cb7 100644
--- a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart
+++ b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart
@@ -65,7 +65,7 @@
'A null value was passed into a non-nullable parameter: $variableName.';
// Run-time null safety assertion per:
-// https://github.com/dart-lang/language/blob/master/accepted/2.12/nnbd/feature-specification.md#automatic-debug-assertion-insertion
+// https://github.com/dart-lang/language/blob/master/accepted/future-releases/nnbd/feature-specification.md#automatic-debug-assertion-insertion
nullFailed(String? fileUri, int? line, int? column, String? variable) {
if (_nonNullAsserts) {
throw AssertionErrorImpl(_nullFailedMessage(variable), fileUri, line,
diff --git a/tests/language/nnbd/null_assertions/parameter_checks_fields_and_setters_test.dart b/tests/language/nnbd/null_assertions/parameter_checks_fields_and_setters_test.dart
index 8ce384f..98a9cbf 100644
--- a/tests/language/nnbd/null_assertions/parameter_checks_fields_and_setters_test.dart
+++ b/tests/language/nnbd/null_assertions/parameter_checks_fields_and_setters_test.dart
@@ -14,7 +14,7 @@
import "package:expect/expect.dart";
-import 'parameter_checks_opted_in.dart' as null_safe;
+import 'parameter_checks_opted_in.dart';
bool Function(Object) asserted(String name) {
if (const bool.fromEnvironment('checkString', defaultValue: true)) {
@@ -25,184 +25,65 @@
}
void use(bool x) {
- if (x != null && x) {
+ if (x) {
print('hey');
}
}
-bool topLevelField = false;
-int get topLevelGetterSetterPair => 0;
-set topLevelGetterSetterPair(int i) => null;
-set topLevelSetterOnly(String s) => null;
-
-abstract class Abs {
- int field;
- Abs([int init]) {
- field = init ?? 20;
- }
-}
-
-class Impl extends Abs {
- @override
- final int field;
-
- Impl([int init])
- : field = init ?? 10,
- super();
-}
-
-// NOTE - All class definitions should be identical to the same implementations
-// in the null safe library so the difference in behavior can be observed.
-
-/// Base class.
-class A {
- int get getterSetterPair => 0;
- set getterSetterPair(int i) => null;
- set setterOnly(String s) => null;
- int field = 0;
- static bool staticField = false;
- static int get staticGetterSetterPair => 0;
- static set staticGetterSetterPair(int i) => null;
- static set staticSetterOnly(String s) => null;
-
- void instanceMethod(String s) => print(s);
- static void staticMethod(String s) => print(s);
-}
-
-/// Overrides the getters but inherits the setters.
-class B extends null_safe.A {
+class C extends A {
+ // Overrides the getters but not the setters.
@override
int get getterSetterPair => 999;
@override
int get field => 999;
}
-/// Overrides the setters.
-class C extends null_safe.A {
- @override
- set getterSetterPair(int i) => null;
- @override
- set setterOnly(String s) => null;
- @override
- set field(int i) => null;
-}
-
-/// Overrides field with a field.
-class D extends null_safe.A {
- @override
- int field = 10;
-}
-
main() {
- // Top level definitions in opted out library allow null without errors.
- topLevelField = null;
+ Expect.throws(() {
+ topLevelField = null;
+ }, asserted("topLevelField"));
use(topLevelField); // Make sure topLevelField is not tree-shaken.
- topLevelGetterSetterPair = null;
- topLevelSetterOnly = null;
+ Expect.throws(() {
+ topLevelGetterSetterPair = null;
+ }, asserted("i"));
+ Expect.throws(() {
+ topLevelSetterOnly = null;
+ }, asserted("s"));
- // Same definitions in a null safe library throw when set to null.
- Expect.throws(() {
- null_safe.topLevelField = null;
- }, asserted('topLevelField'));
- use(null_safe.topLevelField); // Make sure topLevelField is not tree-shaken.
- Expect.throws(() {
- null_safe.topLevelGetterSetterPair = null;
- }, asserted('i'));
- Expect.throws(() {
- null_safe.topLevelSetterOnly = null;
- }, asserted('s'));
-
- // Class defined in opted out library allows null.
var a = A();
- a.getterSetterPair = null;
- a.setterOnly = null;
- a.field = null;
- A.staticGetterSetterPair = null;
- A.staticSetterOnly = null;
- A.staticField = null;
+ Expect.throws(() {
+ a.getterSetterPair = null;
+ }, asserted("i"));
+ Expect.throws(() {
+ a.setterOnly = null;
+ }, asserted("s"));
+ Expect.throws(() {
+ a.field = null;
+ }, asserted("field"));
+ Expect.throws(() {
+ A.staticGetterSetterPair = null;
+ }, asserted("i"));
+ Expect.throws(() {
+ A.staticSetterOnly = null;
+ }, asserted("s"));
+ Expect.throws(() {
+ A.staticField = null;
+ }, asserted("staticField"));
use(A.staticField); // Make sure A.staticField is not tree-shaken.
- // Same class as above defined in a null safe library, throws on null.
- var nullSafeA = null_safe.A();
- Expect.throws(() {
- nullSafeA.getterSetterPair = null;
- }, asserted('i'));
- Expect.throws(() {
- nullSafeA.setterOnly = null;
- }, asserted('s'));
- Expect.throws(() {
- nullSafeA.field = null;
- }, asserted('field'));
- Expect.throws(() {
- null_safe.A.staticGetterSetterPair = null;
- }, asserted('i'));
- Expect.throws(() {
- null_safe.A.staticSetterOnly = null;
- }, asserted('s'));
- Expect.throws(() {
- null_safe.A.staticField = null;
- }, asserted('staticField'));
- use(null_safe.A.staticField); // Make sure A.staticField is not tree-shaken.
-
- // Class defined in opted out library overrides getters but inherited
- // implementations throw.
var b = B();
Expect.throws(() {
b.getterSetterPair = null;
- }, asserted('i'));
- Expect.throws(() {
- b.setterOnly = null;
- }, asserted('s'));
+ }, asserted("i"));
Expect.throws(() {
b.field = null;
- }, asserted('field'));
+ }, asserted("field"));
- // Same class as above defined in and inherited from null safe library throw.
- var nullSafeB = null_safe.B();
- Expect.throws(() {
- nullSafeB.getterSetterPair = null;
- }, asserted('i'));
- Expect.throws(() {
- nullSafeB.setterOnly = null;
- }, asserted('s'));
- Expect.throws(() {
- nullSafeB.field = null;
- }, asserted('field'));
-
- // Class defined in opted out library, overrides all setters, doesn't throw.
var c = C();
- c.getterSetterPair = null;
- c.setterOnly = null;
- c.field = null;
-
- // Same class as above defined in null safe library throws.
- var nullSafeC = null_safe.C();
Expect.throws(() {
- nullSafeC.getterSetterPair = null;
- }, asserted('i'));
+ c.getterSetterPair = null;
+ }, asserted("i"));
Expect.throws(() {
- nullSafeC.setterOnly = null;
- }, asserted('s'));
- Expect.throws(() {
- nullSafeC.field = null;
- }, asserted('i'));
-
- // Class defined in opted out library overrides field, doesn't throw.
- var d = D();
- d.field = null;
-
- // Same class as above defined in null safe library throws.
- var nullSafeD = null_safe.D();
- Expect.throws(() {
- nullSafeD.field = null;
- }, asserted('field'));
-
- var s = Impl();
- // Should not throw because the setter is defined in a library that has not
- // yet migrated to null safety.
- //
- // The Impl class has no setter, but the field has a setterType of `Never`
- // which is non-nullable. This acts as a regression test to ensure we don't
- // introduce a null check because of the strange setter type.
- s.field = null;
+ c.field = null;
+ }, asserted("field"));
}
diff --git a/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart b/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart
index 691e7eb..e1cb811 100644
--- a/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart
+++ b/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart
@@ -23,7 +23,6 @@
void Function(int) bar() => (int x) {};
-/// Base class.
class A {
int get getterSetterPair => 0;
set getterSetterPair(int i) => null;
@@ -38,26 +37,10 @@
static void staticMethod(String s) => print(s);
}
-/// Overrides the getters but inherits the setters.
class B extends A {
+ // Overrides the getters but not the setters.
@override
int get getterSetterPair => 999;
@override
int get field => 999;
}
-
-/// Overrides the setters.
-class C extends A {
- @override
- set getterSetterPair(int i) => null;
- @override
- set setterOnly(String s) => null;
- @override
- set field(int i) => null;
-}
-
-/// Overrides field with a field
-class D extends A {
- @override
- int field = 10;
-}
