mirror of
https://github.com/dart-lang/sdk
synced 2024-09-05 08:24:39 +00:00
Revert "[ddc] Add non-null assertions when setting fields"
This reverts commit c590001ef0
.
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>
This commit is contained in:
parent
380a505b0d
commit
c0b40136e5
|
@ -2170,21 +2170,18 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
|
|||
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 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
|
|||
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 @@ 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(
|
||||
|
@ -3687,8 +3638,30 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
|
|||
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -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,
|
||||
|
|
|
@ -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"));
|
||||
}
|
||||
|
|
|
@ -23,7 +23,6 @@ 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;
|
||||
|
@ -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;
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue