From 8a1e69063ed5343a0fc490c8fb69464d227e3a28 Mon Sep 17 00:00:00 2001 From: Nicholas Shahan Date: Thu, 19 Oct 2023 21:32:15 +0000 Subject: [PATCH] [ddc] Update for extra null safety checks in RTI - In development mode (DDC) the extra null safety errors will be thrown. - Remove extra code paths that called unsound helpers. - Fix expectations in weak_null_safety_errors_test.dart. Change-Id: I107c602b0ae38b13038e501564cba9b8cfc58e70 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/329568 Reviewed-by: Mayank Patke Commit-Queue: Nicholas Shahan --- .../lib/_null_safety_dartdevc.dart | 2 +- pkg/dev_compiler/lib/src/kernel/compiler.dart | 47 ++--- .../private/ddc_runtime/errors.dart | 9 +- .../private/ddc_runtime/operations.dart | 193 +++--------------- .../private/ddc_runtime/types.dart | 16 +- .../js_dev_runtime/private/js_array.dart | 2 +- .../js_dev_runtime/private/js_helper.dart | 4 +- sdk/lib/_internal/js_shared/lib/rti.dart | 8 +- .../weak_null_safety_errors_test.dart | 12 +- 9 files changed, 68 insertions(+), 225 deletions(-) diff --git a/pkg/dart2js_runtime_metrics/lib/_null_safety_dartdevc.dart b/pkg/dart2js_runtime_metrics/lib/_null_safety_dartdevc.dart index 47b3a96a66b..0aebb4640c6 100644 --- a/pkg/dart2js_runtime_metrics/lib/_null_safety_dartdevc.dart +++ b/pkg/dart2js_runtime_metrics/lib/_null_safety_dartdevc.dart @@ -2,5 +2,5 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -// TODO(nshahan): Provide a DDC implementation. +// Unused in DDC. void Function(TypeError, StackTrace)? onExtraNullSafetyError; diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index 31d884b87f4..a1248e41752 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -7238,22 +7238,14 @@ class ProgramCompiler extends ComputeOnceConstantVisitor return js.call('typeof # == #', [lhs, js.string(typeofName, "'")]); } - if (_options.newRuntimeTypes) { - // When using the new runtime type system with sound null safety we can - // call to the library directly. In weak mode we call a DDC only method - // that can optionally produce warnings or errors when the check passes - // but would fail with sound null safety. - return _options.soundNullSafety - ? js.call('#.#(#)', [ - _emitType(type), - _emitMemberName(js_ast.FixedNames.rtiIsField, - memberClass: rtiClass), - lhs - ]) - : runtimeCall('is(#, #)', [lhs, _emitType(type)]); - } - - return js.call('#.is(#)', [_emitType(type), lhs]); + return _options.newRuntimeTypes + ? js.call('#.#(#)', [ + _emitType(type), + _emitMemberName(js_ast.FixedNames.rtiIsField, + memberClass: rtiClass), + lhs + ]) + : js.call('#.is(#)', [_emitType(type), lhs]); } @override @@ -7331,21 +7323,14 @@ class ProgramCompiler extends ComputeOnceConstantVisitor js_ast.Expression _emitCast(js_ast.Expression expr, DartType type) { if (_types.isTop(type)) return expr; - if (_options.newRuntimeTypes) { - // When using the new runtime type system with sound null safety we can - // call to the library directly. In weak mode we call a DDC only method - // that can optionally produce warnings or errors when the cast passes but - // would fail with sound null safety. - return _options.soundNullSafety - ? js.call('#.#(#)', [ - _emitType(type), - _emitMemberName(js_ast.FixedNames.rtiAsField, - memberClass: rtiClass), - expr - ]) - : runtimeCall('as(#, #)', [expr, _emitType(type)]); - } - return js.call('#.as(#)', [_emitType(type), expr]); + return _options.newRuntimeTypes + ? js.call('#.#(#)', [ + _emitType(type), + _emitMemberName(js_ast.FixedNames.rtiAsField, + memberClass: rtiClass), + expr + ]) + : js.call('#.as(#)', [_emitType(type), expr]); } @override 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 cad732adde5..8623a77f249 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 @@ -14,10 +14,13 @@ argumentError(value) { } /// Only used during the development of the new runtime type system in branches -/// that should never be executed. +/// that should never be executed in the active type system. // TODO(48585): Remove after switching to the new runtime type system. -Never throwUnimplementedInOldRti() => throw UnimplementedError( - 'This code path is not supported with the old runtime type system.'); +Never throwUnimplementedInCurrentRti() { + var systemVersion = compileTimeFlag('newRuntimeTypes') ? 'new' : 'old'; + throw UnimplementedError('This code path is not supported with the ' + '$systemVersion runtime type system.'); +} throwUnimplementedError(String message) { throw UnimplementedError(message); diff --git a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart index 5a082f5fc8a..3c50f920f2f 100644 --- a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart +++ b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart @@ -187,16 +187,10 @@ dput(obj, field, value) { if (f != null) { var setterType = getSetterType(getType(obj), f); if (setterType != null) { - if (JS_GET_FLAG('NEW_RUNTIME_TYPES')) { - return compileTimeFlag('soundNullSafety') - // Call the 'as' method directly in sound mode. - ? JS('', '#[#] = #.#(#)', obj, f, setterType, - JS_GET_NAME(JsGetName.RTI_FIELD_AS), value) - // Call `cast` in weak mode to provide optional warnings or errors. - : JS('', '#[#] = #', obj, f, cast(value, setterType)); - } else { - return JS('', '#[#] = #.as(#)', obj, f, setterType, value); - } + return JS_GET_FLAG('NEW_RUNTIME_TYPES') + ? JS('', '#[#] = #.#(#)', obj, f, setterType, + JS_GET_NAME(JsGetName.RTI_FIELD_AS), value) + : JS('', '#[#] = #.as(#)', obj, f, setterType, value); } // Always allow for JS interop objects. if (isJsInterop(obj)) return JS('', '#[#] = #', obj, f, value); @@ -269,44 +263,26 @@ String? _argumentErrors(Object type, @notNull List actuals, namedActuals) { } } // Now that we know the signature matches, we can perform type checks. - if (compileTimeFlag('soundNullSafety')) { - // Call the 'as' method directly in sound mode. - for (var i = 0; i < requiredCount; ++i) { - var requiredRti = JS('!', '#[#]', requiredPositional, i); - var passedValue = JS('', '#[#]', actuals, i); - JS('', '#.#(#)', requiredRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), + for (var i = 0; i < requiredCount; ++i) { + var requiredRti = JS('!', '#[#]', requiredPositional, i); + var passedValue = JS('', '#[#]', actuals, i); + JS('', '#.#(#)', requiredRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), + passedValue); + } + for (var i = 0; i < extras; ++i) { + var optionalRti = JS('!', '#[#]', optionalPositional, i); + var passedValue = JS('', '#[#]', actuals, i + requiredCount); + JS('', '#.#(#)', optionalRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), + passedValue); + } + if (names != null) { + for (var name in names) { + var namedRti = JS( + '!', '#[#] || #[#]', requiredNamed, name, optionalNamed, name); + var passedValue = JS('', '#[#]', namedActuals, name); + JS('', '#.#(#)', namedRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), passedValue); } - for (var i = 0; i < extras; ++i) { - var optionalRti = JS('!', '#[#]', optionalPositional, i); - var passedValue = JS('', '#[#]', actuals, i + requiredCount); - JS('', '#.#(#)', optionalRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), - passedValue); - } - if (names != null) { - for (var name in names) { - var namedRti = JS( - '!', '#[#] || #[#]', requiredNamed, name, optionalNamed, name); - var passedValue = JS('', '#[#]', namedActuals, name); - JS('', '#.#(#)', namedRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), - passedValue); - } - } - } else { - // Call `cast` in weak mode to provide optional warnings or errors. - for (var i = 0; i < requiredCount; ++i) { - cast(JS('', '#[#]', actuals, i), JS('', '#[#]', requiredPositional, i)); - } - for (var i = 0; i < extras; ++i) { - cast(JS('', '#[#]', actuals, i + requiredCount), - JS('', '#[#]', optionalPositional, i)); - } - if (names != null) { - for (var name in names) { - cast(JS('', '#[#]', namedActuals, name), - JS('', '#[#] || #[#]', requiredNamed, name, optionalNamed, name)); - } - } } return null; } else { @@ -517,10 +493,10 @@ _checkAndCall(f, ftype, obj, typeArgs, args, named, displayName) { // there is no longer any warnings/errors in weak null safety mode. if (bound != typeArg) { var instantiatedBound = rti.substitute(bound, typeArgs); - var validSubtype = compileTimeFlag('soundNullSafety') - ? rti.isSubtype(JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), typeArg, - instantiatedBound) - : _isSubtypeWithWarning(typeArg, instantiatedBound); + var validSubtype = rti.isSubtype( + JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), + typeArg, + instantiatedBound); if (!validSubtype) { throwTypeError("The type '${rti.rtiToString(typeArg)}' " "is not a subtype of the type variable bound " @@ -720,38 +696,7 @@ dsetindex(obj, index, value) => @JSExportName('is') bool instanceOf(obj, type) { if (JS_GET_FLAG('NEW_RUNTIME_TYPES')) { - // When running without sound null safety is type tests are dispatched here - // to issue optional warnings/errors when the result is true but would be - // false with sound null safety. - var testRti = JS('!', '#', type); - // TODO(nshahan): Move to isSubtype in dart:rti once all fast path checks - // have been updated to be aware of - // `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')`. - rti.Rti legacyErasedType; - if (JS_GET_FLAG('PRINT_LEGACY_STARS')) { - // When preserving the legacy stars in the runtime type, avoid caching - // the version with erased types on the Rti. - var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(testRti); - legacyErasedType = rti.findType(legacyErasedRecipe); - } else { - legacyErasedType = rti.getLegacyErasedRti(testRti); - } - extraNullSafetyChecks = true; - legacyTypeChecks = false; - var result = JS('bool', '#.#(#)', legacyErasedType, - JS_GET_NAME(JsGetName.RTI_FIELD_IS), obj); - extraNullSafetyChecks = false; - legacyTypeChecks = true; - if (result) return true; - result = - JS('bool', '#.#(#)', testRti, JS_GET_NAME(JsGetName.RTI_FIELD_IS), obj); - if (result) { - // Type test returned true but would be false with sound null safety. - var t1 = runtimeType(obj); - var t2 = rti.createRuntimeType(testRti); - _nullWarn('$t1 is not a subtype of $t2.'); - } - return result; + throwUnimplementedInCurrentRti(); } else { if (obj == null) { return _equalType(type, Null) || @@ -770,41 +715,7 @@ bool instanceOf(obj, type) { @JSExportName('as') cast(obj, type) { if (JS_GET_FLAG('NEW_RUNTIME_TYPES')) { - // When running without sound null safety casts are dispatched here to issue - // optional warnings/errors when casts pass but would fail with sound null - // safety. - var testRti = JS('!', '#', type); - if (obj == null && !rti.isNullable(testRti)) { - // Allow cast to pass but warn that it would fail in sound null safety. - _nullWarnOnType(type); - return obj; - } - // TODO(nshahan): Move to isSubtype in dart:rti once all fast path checks - // have been updated to be aware of - // `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')`. - rti.Rti legacyErasedType; - if (JS_GET_FLAG('PRINT_LEGACY_STARS')) { - // When preserving the legacy stars in the runtime type, avoid caching - // the version with erased types on the Rti. - var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(testRti); - legacyErasedType = rti.findType(legacyErasedRecipe); - } else { - legacyErasedType = rti.getLegacyErasedRti(testRti); - } - extraNullSafetyChecks = true; - legacyTypeChecks = false; - var result = JS('!', '#.#(#)', legacyErasedType, - JS_GET_NAME(JsGetName.RTI_FIELD_IS), obj); - extraNullSafetyChecks = false; - legacyTypeChecks = true; - if (result) return obj; - // Perform the actual cast and allow the error to be thrown if it fails. - JS('', '#.#(#)', testRti, JS_GET_NAME(JsGetName.RTI_FIELD_AS), obj); - // Cast succeeds but would fail with sound null safety. - var t1 = runtimeType(obj); - var t2 = rti.createRuntimeType(testRti); - _nullWarn('$t1 is not a subtype of $t2.'); - return obj; + throwUnimplementedInCurrentRti(); } else { // We hoist the common case where null is checked against another type here // for better performance. @@ -819,54 +730,6 @@ cast(obj, type) { } } -/// Returns `true` if [t1] is a subtype of [t2]. -/// -/// This method should only be called when running with weak null safety. -/// -/// Will produce a warning/error (if enabled) when the subtype passes but would -/// fail in sound null safety. -// TODO(48585) Revise argument types after removing old type representation. -@notNull -bool _isSubtypeWithWarning(@notNull t1, @notNull t2) { - // Avoid classes from the rti library appearing unless they are used. - if (JS_GET_FLAG('NEW_RUNTIME_TYPES')) { - var t1Rti = JS('!', '#', t1); - var t2Rti = JS('!', '#', t2); - // TODO(nshahan): Move to isSubtype in dart:rti once all fast path checks - // have been updated to be aware of - // `JS_GET_FLAG('EXTRA_NULL_SAFETY_CHECKS')`. - rti.Rti legacyErasedType; - if (JS_GET_FLAG('PRINT_LEGACY_STARS')) { - // When preserving the legacy stars in the runtime type, avoid caching - // the version with erased types on the Rti. - var legacyErasedRecipe = rti.Rti.getLegacyErasedRecipe(t2Rti); - legacyErasedType = rti.findType(legacyErasedRecipe); - } else { - legacyErasedType = rti.getLegacyErasedRti(t2Rti); - } - extraNullSafetyChecks = true; - legacyTypeChecks = false; - var validSubtype = rti.isSubtype( - JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), t1Rti, legacyErasedType); - extraNullSafetyChecks = false; - legacyTypeChecks = true; - if (validSubtype) return true; - validSubtype = - rti.isSubtype(JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), t1Rti, t2Rti); - if (validSubtype) { - // Subtype check passes but would fail with sound null safety. - _nullWarn('${rti.createRuntimeType(t1Rti)} ' - 'is not a subtype of ' - '${rti.createRuntimeType(t2Rti)}.'); - } - return validSubtype; - } else { - // Should never be reached because this method isn't called when using old - // runtime type representation. - throwUnimplementedInOldRti(); - } -} - bool test(bool? obj) { if (obj == null) throw BooleanConversionAssertionError(); return obj; diff --git a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart index b3d05f20c46..989694c2767 100644 --- a/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart +++ b/sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart @@ -74,6 +74,7 @@ void weakNullSafetyErrors(bool showErrors) { } _weakNullSafetyErrors = showErrors; + extraNullSafetyChecks = showErrors; } @notNull @@ -1221,17 +1222,10 @@ bool isType(obj) => JS('', '#[#] === #', obj, _runtimeType, Type); void checkTypeBound( @notNull Object type, @notNull Object bound, @notNull String name) { - bool validSubtype; - if (JS_GET_FLAG('NEW_RUNTIME_TYPES')) { - validSubtype = compileTimeFlag('soundNullSafety') - // Check subtype directly in sound mode. - ? rti.isSubtype(JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), - JS('!', '#', type), JS('!', '#', bound)) - // Check subtype but issue warnings/errors in weak mode. - : _isSubtypeWithWarning(type, bound); - } else { - validSubtype = isSubtypeOf(type, bound); - } + var validSubtype = JS_GET_FLAG('NEW_RUNTIME_TYPES') + ? rti.isSubtype(JS_EMBEDDED_GLOBAL('', RTI_UNIVERSE), + JS('!', '#', type), JS('!', '#', bound)) + : isSubtypeOf(type, bound); if (!validSubtype) { throwTypeError('type `$type` does not extend `$bound` of `$name`.'); } diff --git a/sdk/lib/_internal/js_dev_runtime/private/js_array.dart b/sdk/lib/_internal/js_dev_runtime/private/js_array.dart index a5f4ee409fa..631801ea709 100644 --- a/sdk/lib/_internal/js_dev_runtime/private/js_array.dart +++ b/sdk/lib/_internal/js_dev_runtime/private/js_array.dart @@ -50,7 +50,7 @@ class JSArray extends JavaScriptObject /// `JS_EMBEDDED_GLOBAL('', ARRAY_RTI_PROPERTY)`. Object get arrayRti => JS_GET_FLAG('NEW_RUNTIME_TYPES') ? dart.typeRep>() - : throw dart.throwUnimplementedInOldRti(); + : throw dart.throwUnimplementedInCurrentRti(); /// Unsupported action, only provided here to help diagnosis of an accidental /// attempt to set the value manually. diff --git a/sdk/lib/_internal/js_dev_runtime/private/js_helper.dart b/sdk/lib/_internal/js_dev_runtime/private/js_helper.dart index 848a083120c..29bdda0ff13 100644 --- a/sdk/lib/_internal/js_dev_runtime/private/js_helper.dart +++ b/sdk/lib/_internal/js_dev_runtime/private/js_helper.dart @@ -887,7 +887,7 @@ Object? createRecordTypePredicate(String partialShapeTag, JSArray fieldRtis) { rti.pairwiseIsTest(fieldRtis, JS('!', '#.values', obj)); }; } else { - dart.throwUnimplementedInOldRti(); + dart.throwUnimplementedInCurrentRti(); } } @@ -911,7 +911,7 @@ rti.Rti getRtiForRecord(Object? record) { return rti.evaluateRtiForRecord(recipeBuffer.toString(), recordObj.values); } else { - dart.throwUnimplementedInOldRti(); + dart.throwUnimplementedInCurrentRti(); } } diff --git a/sdk/lib/_internal/js_shared/lib/rti.dart b/sdk/lib/_internal/js_shared/lib/rti.dart index ff879712c08..cb788d95759 100644 --- a/sdk/lib/_internal/js_shared/lib/rti.dart +++ b/sdk/lib/_internal/js_shared/lib/rti.dart @@ -36,9 +36,11 @@ bool _reportingExtraNullSafetyError = false; @pragma('dart2js:as:trust') void _onExtraNullSafetyError(TypeError error, StackTrace trace) { - // If [onExtraNullSafetyError] itself produces an extra null safety error, - // this avoids blowing the stack. - if (!_reportingExtraNullSafetyError) { + if (JS_GET_FLAG('DEV_COMPILER')) { + throw error; + } else if (!_reportingExtraNullSafetyError) { + // If [onExtraNullSafetyError] itself produces an extra null safety error, + // this avoids blowing the stack. _reportingExtraNullSafetyError = true; if (onExtraNullSafetyError != null) { (onExtraNullSafetyError as OnExtraNullSafetyError)(error, trace); diff --git a/tests/dartdevc/weak_null_safety_errors_test.dart b/tests/dartdevc/weak_null_safety_errors_test.dart index fbac154e387..229f1c2c125 100644 --- a/tests/dartdevc/weak_null_safety_errors_test.dart +++ b/tests/dartdevc/weak_null_safety_errors_test.dart @@ -13,8 +13,8 @@ import 'package:expect/expect.dart'; /// `--weak-null-safety-errors`. void fn(StringBuffer arg) {} -void testArg(T t) => t is T; -void testReturn(T Function() f) => throw 'do not call'; +void testArg(T t) => throw 'do not call'; +T testReturn() => throw 'do not call'; const c = C(); @@ -28,10 +28,7 @@ void main() { dynamic dynamicNull = null; Expect.throwsTypeError(() => fn(dynamicNull)); - var l = [Duration(days: 1), null]; - Expect.throwsTypeError(() => l as List); - - Expect.throwsTypeError(() => (testReturn) as Duration Function()); + Expect.throwsTypeError(() => [Duration(days: 1), null] as List); // Constants get legacy types introduced in their type arguments. C c2 = c; @@ -44,6 +41,5 @@ void main() { // subtype of `void Function(Duration?)`. In sound null safety the signature // is `void Function(Duration)` which should fail in the cast. Expect.throwsTypeError(() => (testArg) as void Function(Duration?)); - Expect.throwsTypeError( - () => (testReturn) as void Function(Duration? Function())); + Expect.throwsTypeError(() => (testReturn) as Duration Function()); }