diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index 4ca915cfa0a..4962bdc131f 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -2170,21 +2170,18 @@ class ProgramCompiler extends ComputeOnceConstantVisitor var jsGetter = js_ast.Method(name, getter, isGetter: true) ..sourceInformation = _nodeStart(field); - var body = []; - 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 @@ class ProgramCompiler extends ComputeOnceConstantVisitor member.fileOffset, member.name.text.length)); if (!member.isFinal && !member.isConst) { - 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. + // 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 @@ 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( @@ -3687,8 +3638,30 @@ class ProgramCompiler extends ComputeOnceConstantVisitor 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 c94272137da..f028cb7c680 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/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 8ce384ff391..98a9cbfab19 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 @@ bool Function(Object) asserted(String name) { } 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 691e7eb7444..e1cb81182ec 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 @@ 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; @@ -38,26 +37,10 @@ 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; -}