languages/dart-sdk
languages/dart-sdk
1
0
Fork 0
mirror of https://github.com/dart-lang/sdk synced 2024-09-05 08:24:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

[ddc] Add non-null assertions when setting fields

Browse source 
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>
This commit is contained in:
Nicholas Shahan 2022-09-19 23:39:03 +00:00 committed by Commit Bot
parent 78bd225ac9
commit c590001ef0
4 changed files with 242 additions and 79 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

103
pkg/dev_compiler/lib/src/kernel/compiler.dart
View file

@ -2170,18 +2170,21 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
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 = <js_ast.Statement>[];
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<js_ast.Expression>
member.fileOffset,
member.name.text.length));
if (!member.isFinal && !member.isConst) {
// TODO(jmesserly): currently uses a dummy setter to indicate
// writable.
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.
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<js_ast.Expression>
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(
@ -3638,30 +3687,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
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));
}
}

2
sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart
View file

@ -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,

197
tests/language/nnbd/null_assertions/parameter_checks_fields_and_setters_test.dart
View file

@ -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;
}

19
tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart
View file

@ -23,6 +23,7 @@ foo6b<T extends FutureOr<S>, 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;
}