diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index 4962bdc131f..4ca915cfa0a 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -2170,18 +2170,21 @@ class ProgramCompiler extends ComputeOnceConstantVisitor var jsGetter = js_ast.Method(name, getter, isGetter: true) ..sourceInformation = _nodeStart(field); - var args = field.isFinal - ? [js_ast.Super(), name] - : [js_ast.This(), virtualFieldSymbol]; - - js_ast.Expression value = _emitIdentifier('value'); - if (!field.isFinal && isCovariantField(field)) { - value = _emitCast(value, field.type); + var body = []; + var value = _emitIdentifier('value'); + if (_requiresExtraNullCheck(field.setterType, field.annotations)) { + body.add( + _nullSafetyParameterCheck(value, field.location, field.name.text)); } - args.add(value); - - var jsSetter = js_ast.Method( - name, js.fun('function(value) { #[#] = #; }', args), + 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)), isSetter: true) ..sourceInformation = _nodeStart(field); @@ -2368,10 +2371,16 @@ class ProgramCompiler extends ComputeOnceConstantVisitor member.fileOffset, member.name.text.length)); if (!member.isFinal && !member.isConst) { - // TODO(jmesserly): currently uses a dummy setter to indicate - // writable. + var body = []; + 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. accessors.add(js_ast.Method( - access, js.call('function(_) {}') as js_ast.Fun, + access, js_ast.Fun([value], js_ast.Block(body)), isSetter: true)); } } else if (member is Procedure) { @@ -3608,6 +3617,46 @@ class ProgramCompiler extends ComputeOnceConstantVisitor 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 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 _emitArgumentInitializers( @@ -3638,30 +3687,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor if (_annotatedNullCheck(p.annotations)) { body.add(_nullParameterCheck(jsParam)); - } 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); + } else if (_requiresExtraNullCheck(p.type, p.annotations)) { + body.add(_nullSafetyParameterCheck(jsParam, p.location, p.name)); } } 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 f028cb7c680..c94272137da 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 @@ String _nullFailedMessage(variableName) => '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/future-releases/nnbd/feature-specification.md#automatic-debug-assertion-insertion +// https://github.com/dart-lang/language/blob/master/accepted/2.12/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 98a9cbfab19..8ce384ff391 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'; +import 'parameter_checks_opted_in.dart' as null_safe; bool Function(Object) asserted(String name) { if (const bool.fromEnvironment('checkString', defaultValue: true)) { @@ -25,65 +25,184 @@ bool Function(Object) asserted(String name) { } void use(bool x) { - if (x) { + if (x != null && x) { print('hey'); } } -class C extends A { - // Overrides the getters but not the setters. +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 { @override int get getterSetterPair => 999; @override int get field => 999; } -main() { - Expect.throws(() { - topLevelField = null; - }, asserted("topLevelField")); - use(topLevelField); // Make sure topLevelField is not tree-shaken. - Expect.throws(() { - topLevelGetterSetterPair = null; - }, asserted("i")); - Expect.throws(() { - topLevelSetterOnly = null; - }, asserted("s")); +/// 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; + use(topLevelField); // Make sure topLevelField is not tree-shaken. + topLevelGetterSetterPair = null; + topLevelSetterOnly = null; + + // 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(); - 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")); + a.getterSetterPair = null; + a.setterOnly = null; + a.field = null; + A.staticGetterSetterPair = null; + A.staticSetterOnly = null; + A.staticField = null; 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")); + }, asserted('i')); + Expect.throws(() { + b.setterOnly = null; + }, asserted('s')); 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(() { - c.getterSetterPair = null; - }, asserted("i")); + nullSafeC.getterSetterPair = null; + }, asserted('i')); Expect.throws(() { - c.field = null; - }, asserted("field")); + 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; } 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 e1cb81182ec..691e7eb7444 100644 --- a/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart +++ b/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart @@ -23,6 +23,7 @@ foo6b, S extends U, U extends int>(T a) {} void Function(int) bar() => (int x) {}; +/// Base class. class A { int get getterSetterPair => 0; set getterSetterPair(int i) => null; @@ -37,10 +38,26 @@ class A { 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; +}