From 82f2906764a1e554d52c7f196de2ae5d32b4ee90 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Thu, 15 Jul 2021 11:57:02 +0000 Subject: [PATCH] Migration: properly handle non-nullable built_value getters This change makes the migration tool understand that the method `BuiltValueNullFieldError.checkNotNull` expresses non-null intent. Since the built_value code generator calls this method from generated built_value constructors, this is sufficient to ensure that built value getters that lack the `@nullable` annotation will be treated by the migration tool as non-nullable. Bug: https://github.com/dart-lang/sdk/issues/43878 Change-Id: I37a9ce584a10d2a1bb27da92f3f664632547f6ef Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206881 Reviewed-by: Konstantin Shcheglov Reviewed-by: Samuel Rawlins --- pkg/nnbd_migration/lib/src/edge_builder.dart | 51 +++++++++-------- pkg/nnbd_migration/test/abstract_context.dart | 11 ++++ pkg/nnbd_migration/test/api_test.dart | 55 ++++++++++++++++++- 3 files changed, 92 insertions(+), 25 deletions(-) diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart index ecf99f50685..381a783c01d 100644 --- a/pkg/nnbd_migration/lib/src/edge_builder.dart +++ b/pkg/nnbd_migration/lib/src/edge_builder.dart @@ -1388,7 +1388,7 @@ class EdgeBuilder extends GeneralizingAstVisitor } _variables!.recordDecoratedExpressionType(node, expressionType); } - _handleArgumentErrorCheckNotNull(node); + _handleCustomCheckNotNull(node); _handleQuiverCheckNotNull(node); return expressionType; } @@ -2356,29 +2356,6 @@ class EdgeBuilder extends GeneralizingAstVisitor .decoratedTypeParameterBound((type.type as TypeParameterType).element); } - void _handleArgumentErrorCheckNotNull(MethodInvocation node) { - var callee = node.methodName.staticElement; - var calleeIsStatic = callee is ExecutableElement && callee.isStatic; - var target = node.realTarget; - bool targetIsArgumentError = - (target is SimpleIdentifier && target.name == 'ArgumentError') || - (target is PrefixedIdentifier && - target.identifier.name == 'ArgumentError'); - - if (calleeIsStatic && - targetIsArgumentError && - callee!.name == 'checkNotNull' && - node.argumentList.arguments.isNotEmpty) { - var argument = node.argumentList.arguments.first; - if (argument is SimpleIdentifier && _isReferenceInScope(argument)) { - var argumentType = - _variables!.decoratedElementType(argument.staticElement!); - _graph.makeNonNullable(argumentType.node, - ArgumentErrorCheckNotNullOrigin(source, argument)); - } - } - } - /// Creates the necessary constraint(s) for an assignment of the given /// [expression] to a destination whose type is [destinationType]. /// @@ -2551,6 +2528,32 @@ class EdgeBuilder extends GeneralizingAstVisitor redirectedClass.typeParameters); } + void _handleCustomCheckNotNull(MethodInvocation node) { + var callee = node.methodName.staticElement; + if (node.argumentList.arguments.isNotEmpty && + callee is ExecutableElement && + callee.isStatic) { + var enclosingElement = callee.enclosingElement; + if (enclosingElement is ClassElement) { + if (callee.name == 'checkNotNull' && + enclosingElement.name == 'ArgumentError' && + callee.library.isDartCore || + callee.name == 'checkNotNull' && + enclosingElement.name == 'BuiltValueNullFieldError' && + callee.library.source.uri.toString() == + 'package:built_value/built_value.dart') { + var argument = node.argumentList.arguments.first; + if (argument is SimpleIdentifier && _isReferenceInScope(argument)) { + var argumentType = _variables!.decoratedElementType( + _favorFieldFormalElements(getWriteOrReadElement(argument))!); + _graph.makeNonNullable(argumentType.node, + ArgumentErrorCheckNotNullOrigin(source, argument)); + } + } + } + } + } + void _handleExecutableDeclaration( Declaration node, ExecutableElement declaredElement, diff --git a/pkg/nnbd_migration/test/abstract_context.dart b/pkg/nnbd_migration/test/abstract_context.dart index bd4a6889cad..4e7b83edd10 100644 --- a/pkg/nnbd_migration/test/abstract_context.dart +++ b/pkg/nnbd_migration/test/abstract_context.dart @@ -45,6 +45,17 @@ class AbstractContextTest with ResourceProviderMixin { String get testsPath => '$homePath/tests'; + void addBuiltValuePackage() { + addPackageFile('built_value', 'built_value.dart', ''' +abstract class Built, B extends Builder> {} +abstract class Builder, B extends Builder> {} +const String nullable = 'nullable'; +class BuiltValueNullFieldError extends Error { + static T checkNotNull(T? value, String type, String field) => value!; +} +'''); + } + void addMetaPackage() { addPackageFile('meta', 'meta.dart', r''' library meta; diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart index 3017ea2addd..df8afb3d2f5 100644 --- a/pkg/nnbd_migration/test/api_test.dart +++ b/pkg/nnbd_migration/test/api_test.dart @@ -48,7 +48,7 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest { /// Optional parameter [removeViaComments] indicates whether dead code should /// be removed in its entirety (the default) or removed by commenting it out. Future _checkMultipleFileChanges( - Map input, Map expectedOutput, + Map input, Map expectedOutput, {Map migratedInput = const {}, bool removeViaComments = false, bool warnOnWeakCode = false, @@ -495,6 +495,59 @@ main() { await _checkSingleFileChanges(content, expected); } + Future test_built_value_non_nullable_getter() async { + addBuiltValuePackage(); + var root = '$projectPath/lib'; + var path1 = convertPath('$root/lib.dart'); + var file1 = r''' +import 'package:built_value/built_value.dart'; + +part 'lib.g.dart'; + +abstract class Foo implements Built { + int get value; + Foo._(); + factory Foo([void Function(FooBuilder) updates]) = _$Foo; +} +'''; + var expected1 = r''' +import 'package:built_value/built_value.dart'; + +part 'lib.g.dart'; + +abstract class Foo implements Built { + int get value; + Foo._(); + factory Foo([void Function(FooBuilder) updates]) = _$Foo; +} +'''; + // Note: in a real-world scenario the generated file would be in a different + // directory but we don't need to simulate that detail for this test. Also, + // the generated file would have a lot more code in it, but we don't need to + // simulate all the details of what is generated. + var path2 = convertPath('$root/lib.g.dart'); + var file2 = r''' +part of 'lib.dart'; + +class _$Foo extends Foo { + @override + final int value; + + factory _$Foo([void Function(FooBuilder) updates]) => throw ''; + + _$Foo._({this.value}) : super._() { + BuiltValueNullFieldError.checkNotNull(value, 'Foo', 'value'); + } +} + +class FooBuilder implements Builder { + int get value => throw ''; +} +'''; + await _checkMultipleFileChanges( + {path1: file1, path2: file2}, {path1: expected1, path2: anything}); + } + Future test_call_already_migrated_extension() async { var content = ''' import 'already_migrated.dart';