From 7bbf2921db25428cac305561596700079b794c3f Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Thu, 18 Jun 2020 20:02:42 +0000 Subject: [PATCH] [vm/nnbd] Non-null assertions in NNBD weak mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/dart-lang/sdk/issues/42357 Change-Id: I07341ba02361201ec2130fb7b6cb87a25b9b5f51 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151662 Commit-Queue: Alexander Markov Reviewed-by: Leaf Petersen Reviewed-by: Martin Kustermann Reviewed-by: Régis Crelier Reviewed-by: Ryan Macnak --- runtime/lib/errors.cc | 36 +++++++++ runtime/vm/bootstrap_natives.h | 1 + .../frontend/kernel_binary_flowgraph.cc | 2 +- runtime/vm/compiler/frontend/kernel_to_il.cc | 79 +++++++++++++++++++ runtime/vm/compiler/frontend/kernel_to_il.h | 13 +++ runtime/vm/flag_list.h | 2 + runtime/vm/symbols.h | 1 + sdk/lib/_internal/vm/lib/async_patch.dart | 2 +- sdk/lib/_internal/vm/lib/errors_patch.dart | 10 ++- .../parameter_checks_opted_in.dart | 19 +++++ .../parameter_checks_test.dart | 45 +++++++++++ 11 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart create mode 100644 tests/language/nnbd/null_assertions/parameter_checks_test.dart diff --git a/runtime/lib/errors.cc b/runtime/lib/errors.cc index 8d9fb8da2ff..2465c8bb5c1 100644 --- a/runtime/lib/errors.cc +++ b/runtime/lib/errors.cc @@ -107,6 +107,42 @@ DEFINE_NATIVE_ENTRY(AssertionError_throwNew, 0, 3) { return Object::null(); } +// Allocate and throw a new AssertionError. +// Arg0: Source code snippet of failed assertion. +// Arg1: Line number. +// Arg2: Column number. +// Arg3: Message object or null. +// Return value: none, throws an exception. +DEFINE_NATIVE_ENTRY(AssertionError_throwNewSource, 0, 4) { + // No need to type check the arguments. This function can only be called + // internally from the VM. + const String& failed_assertion = + String::CheckedHandle(zone, arguments->NativeArgAt(0)); + const intptr_t line = + Smi::CheckedHandle(zone, arguments->NativeArgAt(1)).Value(); + const intptr_t column = + Smi::CheckedHandle(zone, arguments->NativeArgAt(2)).Value(); + const Instance& message = + Instance::CheckedHandle(zone, arguments->NativeArgAt(3)); + + const Array& args = Array::Handle(zone, Array::New(5)); + + DartFrameIterator iterator(thread, + StackFrameIterator::kNoCrossThreadIteration); + iterator.NextFrame(); // Skip native call. + const Script& script = Script::Handle(zone, FindScript(&iterator)); + + args.SetAt(0, failed_assertion); + args.SetAt(1, String::Handle(zone, script.url())); + args.SetAt(2, Smi::Handle(zone, Smi::New(line))); + args.SetAt(3, Smi::Handle(zone, Smi::New(column))); + args.SetAt(4, message); + + Exceptions::ThrowByType(Exceptions::kAssertion, args); + UNREACHABLE(); + return Object::null(); +} + // Allocate and throw a new TypeError or CastError. // Arg0: index of the token of the failed type check. // Arg1: src value. diff --git a/runtime/vm/bootstrap_natives.h b/runtime/vm/bootstrap_natives.h index 5ed3f684200..a8056290c6a 100644 --- a/runtime/vm/bootstrap_natives.h +++ b/runtime/vm/bootstrap_natives.h @@ -158,6 +158,7 @@ namespace dart { V(DateTime_timeZoneOffsetInSeconds, 1) \ V(DateTime_localTimeZoneAdjustmentInSeconds, 0) \ V(AssertionError_throwNew, 3) \ + V(AssertionError_throwNewSource, 4) \ V(Async_rethrow, 2) \ V(StackTrace_asyncStackTraceHelper, 1) \ V(StackTrace_clearAsyncThreadStackTrace, 0) \ diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc index 589ed9e8167..b68233d1eac 100644 --- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc +++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc @@ -988,7 +988,7 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfFunction( // The RawParameter variables should be set to null to avoid retaining more // objects than necessary during GC. const Fragment body = - ClearRawParameters(dart_function) + + ClearRawParameters(dart_function) + B->BuildNullAssertions() + BuildFunctionBody(dart_function, first_parameter, is_constructor); auto extra_entry_point_style = ChooseEntryPointStyle( diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index 8ce6da1c001..1676b91fa47 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -16,6 +16,7 @@ #include "vm/compiler/frontend/kernel_translation_helper.h" #include "vm/compiler/frontend/prologue_builder.h" #include "vm/compiler/jit/compiler.h" +#include "vm/kernel_isolate.h" #include "vm/kernel_loader.h" #include "vm/longjump.h" #include "vm/native_entry.h" @@ -2572,6 +2573,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFieldAccessor( body += CheckAssignable(setter_value->type(), setter_value->name(), AssertAssignableInstr::kParameterCheck); } + body += BuildNullAssertions(); if (field.is_late()) { if (is_method) { body += Drop(); @@ -3139,6 +3141,83 @@ void FlowGraphBuilder::SetCurrentTryCatchBlock(TryCatchBlock* try_catch_block) { : try_catch_block->try_index()); } +bool FlowGraphBuilder::NeedsNullAssertion(const AbstractType& type) { + if (!type.IsNonNullable()) { + return false; + } + if (type.IsTypeParameter()) { + return NeedsNullAssertion( + AbstractType::Handle(Z, TypeParameter::Cast(type).bound())); + } + if (type.IsFutureOrType()) { + return NeedsNullAssertion(AbstractType::Handle(Z, type.UnwrapFutureOr())); + } + return true; +} + +Fragment FlowGraphBuilder::NullAssertion(LocalVariable* variable) { + Fragment code; + if (!NeedsNullAssertion(variable->type())) { + return code; + } + + TargetEntryInstr* then; + TargetEntryInstr* otherwise; + + code += LoadLocal(variable); + code += NullConstant(); + code += BranchIfEqual(&then, &otherwise); + + if (throw_new_null_assertion_ == nullptr) { + const Class& klass = Class::ZoneHandle( + Z, Library::LookupCoreClass(Symbols::AssertionError())); + ASSERT(!klass.IsNull()); + throw_new_null_assertion_ = + &Function::ZoneHandle(Z, klass.LookupStaticFunctionAllowPrivate( + Symbols::ThrowNewNullAssertion())); + ASSERT(!throw_new_null_assertion_->IsNull()); + } + + const Script& script = + Script::Handle(Z, parsed_function_->function().script()); + intptr_t line = -1; + intptr_t column = -1; + script.GetTokenLocation(variable->token_pos(), &line, &column); + + // Build equivalent of `throw _AssertionError._throwNewNullAssertion(name)` + // expression. We build throw (even through _throwNewNullAssertion already + // throws) because call is not a valid last instruction for the block. + // Blocks can only terminate with explicit control flow instructions + // (Branch, Goto, Return or Throw). + Fragment null_code(then); + null_code += Constant(variable->name()); + null_code += IntConstant(line); + null_code += IntConstant(column); + null_code += StaticCall(variable->token_pos(), *throw_new_null_assertion_, 3, + ICData::kStatic); + null_code += ThrowException(TokenPosition::kNoSource); + null_code += Drop(); + + return Fragment(code.entry, otherwise); +} + +Fragment FlowGraphBuilder::BuildNullAssertions() { + Fragment code; + if (I->null_safety() || !I->asserts() || !FLAG_null_assertions || + !KernelIsolate::GetExperimentalFlag("non-nullable")) { + return code; + } + + const Function& dart_function = parsed_function_->function(); + for (intptr_t i = dart_function.NumImplicitParameters(), + n = dart_function.NumParameters(); + i < n; ++i) { + LocalVariable* variable = parsed_function_->ParameterVariable(i); + code += NullAssertion(variable); + } + return code; +} + } // namespace kernel } // namespace dart diff --git a/runtime/vm/compiler/frontend/kernel_to_il.h b/runtime/vm/compiler/frontend/kernel_to_il.h index c5ea5ee9308..0aff44818ce 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.h +++ b/runtime/vm/compiler/frontend/kernel_to_il.h @@ -260,6 +260,16 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder { Fragment* implicit_checks, Fragment* implicit_redefinitions); + // Returns true if null assertion is needed for + // a parameter of given type. + bool NeedsNullAssertion(const AbstractType& type); + + // Builds null assertion for the given parameter. + Fragment NullAssertion(LocalVariable* variable); + + // Builds null assertions for all parameters (if needed). + Fragment BuildNullAssertions(); + // Builds flow graph for noSuchMethod forwarder. // // If throw_no_such_method_error is set to true, an @@ -407,6 +417,9 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder { ActiveClass active_class_; + // Cached _AssertionError._throwNewNullAssertion. + Function* throw_new_null_assertion_ = nullptr; + friend class BreakableBlock; friend class CatchBlock; friend class ProgramState; diff --git a/runtime/vm/flag_list.h b/runtime/vm/flag_list.h index 74b2a15e3b3..f9f9346b12a 100644 --- a/runtime/vm/flag_list.h +++ b/runtime/vm/flag_list.h @@ -115,6 +115,8 @@ constexpr bool kDartUseBackgroundCompilation = true; "Dump megamorphic cache statistics") \ R(dump_symbol_stats, false, bool, false, "Dump symbol table statistics") \ R(enable_asserts, false, bool, false, "Enable assert statements.") \ + R(null_assertions, false, bool, false, \ + "Enable null assertions for parameters.") \ P(enable_kernel_expression_compilation, bool, true, \ "Compile expressions with the Kernel front-end.") \ P(enable_mirrors, bool, true, \ diff --git a/runtime/vm/symbols.h b/runtime/vm/symbols.h index fb24834f495..8598430cc26 100644 --- a/runtime/vm/symbols.h +++ b/runtime/vm/symbols.h @@ -273,6 +273,7 @@ class ObjectPointerVisitor; V(SymbolCtor, "Symbol.") \ V(ThrowNew, "_throwNew") \ V(ThrowNewInvocation, "_throwNewInvocation") \ + V(ThrowNewNullAssertion, "_throwNewNullAssertion") \ V(TopLevel, "::") \ V(TransferableTypedData, "TransferableTypedData") \ V(TruncDivOperator, "~/") \ diff --git a/sdk/lib/_internal/vm/lib/async_patch.dart b/sdk/lib/_internal/vm/lib/async_patch.dart index c3201d3a13a..8d43f33c3bb 100644 --- a/sdk/lib/_internal/vm/lib/async_patch.dart +++ b/sdk/lib/_internal/vm/lib/async_patch.dart @@ -307,7 +307,7 @@ class _StreamImpl { } @pragma("vm:entry-point", "call") -void _completeOnAsyncReturn(Completer completer, Object value) { +void _completeOnAsyncReturn(Completer completer, Object? value) { completer.complete(value); } diff --git a/sdk/lib/_internal/vm/lib/errors_patch.dart b/sdk/lib/_internal/vm/lib/errors_patch.dart index c4b91f205e3..75776eccf61 100644 --- a/sdk/lib/_internal/vm/lib/errors_patch.dart +++ b/sdk/lib/_internal/vm/lib/errors_patch.dart @@ -36,8 +36,16 @@ class _AssertionError extends Error implements AssertionError { _doThrowNew(assertionStart, assertionEnd, message); } + @pragma("vm:entry-point", "call") + @pragma('vm:never-inline') + static _throwNewNullAssertion(String name, int line, int column) { + _doThrowNewSource('$name != null', line, column, null); + } + static _doThrowNew(int assertionStart, int assertionEnd, Object? message) native "AssertionError_throwNew"; + static _doThrowNewSource(String failedAssertion, int line, int column, Object? message) + native "AssertionError_throwNewSource"; @pragma("vm:entry-point", "call") static _evaluateAssertion(condition) { @@ -75,7 +83,7 @@ class _AssertionError extends Error implements AssertionError { } final String _failedAssertion; - final String _url; + final String? _url; final int _line; final int _column; final Object? message; diff --git a/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart b/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart new file mode 100644 index 00000000000..a4f959fc700 --- /dev/null +++ b/tests/language/nnbd/null_assertions/parameter_checks_opted_in.dart @@ -0,0 +1,19 @@ +// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file +// 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. + +// Opted-in library for parameter_checks_test.dart. + +import 'dart:async' show FutureOr; + +foo1(int a) {} +foo2(int a, [int b = 1, String c = '']) {} +foo3({int a = 0, required int b}) {} +foo4a(T a) {} +foo4b(T a) {} +foo5a(FutureOr a) {} +foo5b(FutureOr a) {} +foo6a, S extends U, U extends int?>(T a) {} +foo6b, S extends U, U extends int>(T a) {} + +void Function(int) bar() => (int x) {}; diff --git a/tests/language/nnbd/null_assertions/parameter_checks_test.dart b/tests/language/nnbd/null_assertions/parameter_checks_test.dart new file mode 100644 index 00000000000..c6f9cc64aec --- /dev/null +++ b/tests/language/nnbd/null_assertions/parameter_checks_test.dart @@ -0,0 +1,45 @@ +// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file +// 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. + +// Test for null assertions for parameters in NNBD weak mode. + +// Requirements=nnbd-weak +// VMOptions=--enable-asserts --null-assertions + +// Opt out of Null Safety: +// @dart = 2.6 + +import "package:expect/expect.dart"; + +import 'parameter_checks_opted_in.dart'; + +main() { + Expect.throws(() { + foo1(null); + }, (e) => e is AssertionError && e.toString().contains("a != null")); + Expect.throws(() { + foo2(1, null); + }, (e) => e is AssertionError && e.toString().contains("b != null")); + Expect.throws(() { + foo3(); + }, (e) => e is AssertionError && e.toString().contains("b != null")); + Expect.throws(() { + foo3(b: null); + }, (e) => e is AssertionError && e.toString().contains("b != null")); + foo4a(null); + Expect.throws(() { + foo4b(null); + }, (e) => e is AssertionError && e.toString().contains("a != null")); + foo5a(null); + Expect.throws(() { + foo5b(null); + }, (e) => e is AssertionError && e.toString().contains("a != null")); + foo6a(null); + Expect.throws(() { + foo6b(null); + }, (e) => e is AssertionError && e.toString().contains("a != null")); + Expect.throws(() { + bar().call(null); + }, (e) => e is AssertionError && e.toString().contains("x != null")); +}