Migration: infer non-null intent for `this` in extensions.
Since `this` is essentially a variable in extensions, it should be
treated similarly to variables in terms of inference of non-null
intent; for example, if an extension declaration contains a a method
that unconditionall dereferences `this` (either implicitly or
explicitly), that's a good indication that the user probably doesn't
intend for the extension to be applied to nullable types.
Fixes #44675.
Bug: https://github.com/dart-lang/sdk/issues/44675
Change-Id: I004328f5b1fd6710954363c07f4e9db6bc6ac2cd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180268
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
diff --git a/pkg/nnbd_migration/lib/instrumentation.dart b/pkg/nnbd_migration/lib/instrumentation.dart
index 5295242..a5fa5ce 100644
--- a/pkg/nnbd_migration/lib/instrumentation.dart
+++ b/pkg/nnbd_migration/lib/instrumentation.dart
@@ -274,6 +274,7 @@
implicitMixinSuperCall,
implicitNullInitializer,
implicitNullReturn,
+ implicitThis,
inferredTypeParameterInstantiation,
instanceCreation,
instantiateToBounds,
diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart
index 572ea5d..5f30b51 100644
--- a/pkg/nnbd_migration/lib/src/edge_builder.dart
+++ b/pkg/nnbd_migration/lib/src/edge_builder.dart
@@ -10,6 +10,7 @@
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/dart/element/type_provider.dart';
import 'package:analyzer/dart/element/type_system.dart';
+import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/dart/element/inheritance_manager3.dart';
import 'package:analyzer/src/dart/element/type.dart';
import 'package:analyzer/src/dart/element/type_system.dart' show TypeSystemImpl;
@@ -1268,6 +1269,7 @@
}
} else if (target == null && callee.enclosingElement is ClassElement) {
targetType = _thisOrSuper(node);
+ _checkThisNotNull(targetType, node);
}
DecoratedType expressionType;
DecoratedType calleeType;
@@ -1575,6 +1577,7 @@
@override
DecoratedType visitSimpleIdentifier(SimpleIdentifier node) {
+ DecoratedType targetType;
DecoratedType result;
var staticElement = getWriteOrReadElement(node);
if (staticElement is PromotableElement) {
@@ -1594,15 +1597,16 @@
} else if (staticElement is FunctionElement ||
staticElement is MethodElement ||
staticElement is ConstructorElement) {
- result = getOrComputeElementType(staticElement,
- targetType: staticElement.enclosingElement is ClassElement
- ? _thisOrSuper(node)
- : null);
+ if (staticElement.enclosingElement is ClassElement) {
+ targetType = _thisOrSuper(node);
+ }
+ result = getOrComputeElementType(staticElement, targetType: targetType);
} else if (staticElement is PropertyAccessorElement) {
- var elementType = getOrComputeElementType(staticElement,
- targetType: staticElement.enclosingElement is ClassElement
- ? _thisOrSuper(node)
- : null);
+ if (staticElement.enclosingElement is ClassElement) {
+ targetType = _thisOrSuper(node);
+ }
+ var elementType =
+ getOrComputeElementType(staticElement, targetType: targetType);
result = staticElement.isGetter
? elementType.returnType
: elementType.positionalParameters[0];
@@ -1621,6 +1625,9 @@
_unimplemented(node,
'Simple identifier with a static element of type ${staticElement.runtimeType}');
}
+ if (targetType != null) {
+ _checkThisNotNull(targetType, node);
+ }
return result;
}
@@ -1986,6 +1993,18 @@
return sourceType;
}
+ /// Generates the appropriate edge to assert that the value of `this` is
+ /// non-null.
+ void _checkThisNotNull(DecoratedType thisType, AstNode node) {
+ // `this` can only be `null` in extensions, so if we're not in an extension,
+ // there's nothing to do.
+ if (_currentExtendedType == null) return;
+ var origin = ImplicitThisOrigin(source, node);
+ var hard =
+ _postDominatedLocals.isInScope(_postDominatedLocals.extensionThis);
+ _graph.makeNonNullable(thisType.node, origin, hard: hard, guards: _guards);
+ }
+
@override
void _connect(NullabilityNode source, NullabilityNode destination,
EdgeOrigin origin, FixReasonTarget edgeTarget,
@@ -2452,6 +2471,9 @@
_addParametersToFlowAnalysis(parameters);
// Push a scope of post-dominated declarations on the stack.
_postDominatedLocals.pushScope(elements: declaredElement.parameters);
+ if (declaredElement.enclosingElement is ExtensionElement) {
+ _postDominatedLocals.add(_postDominatedLocals.extensionThis);
+ }
try {
_dispatchList(initializers);
if (declaredElement is ConstructorElement &&
@@ -3659,6 +3681,10 @@
///
/// Contains helpers for dealing with expressions as if they were elements.
class _ScopedLocalSet extends ScopedSet<Element> {
+ /// The synthetic element we use as a stand-in for `this` when analyzing
+ /// extension methods.
+ Element get extensionThis => DynamicElementImpl.instance;
+
bool isReferenceInScope(Expression expression) {
var element = referencedElement(expression);
return element != null && isInScope(element);
@@ -3670,6 +3696,8 @@
expression = expression.unParenthesized;
if (expression is SimpleIdentifier) {
return expression.staticElement;
+ } else if (expression is ThisExpression || expression is SuperExpression) {
+ return extensionThis;
} else {
return null;
}
diff --git a/pkg/nnbd_migration/lib/src/edge_origin.dart b/pkg/nnbd_migration/lib/src/edge_origin.dart
index f1d39f6..cff0b7b 100644
--- a/pkg/nnbd_migration/lib/src/edge_origin.dart
+++ b/pkg/nnbd_migration/lib/src/edge_origin.dart
@@ -324,6 +324,18 @@
ReturnStatement get node => super.node as ReturnStatement;
}
+/// Edge origin used for edges that arise from an implicit use of `this`, e.g.
+/// during a method call from an extension.
+class ImplicitThisOrigin extends EdgeOrigin {
+ ImplicitThisOrigin(Source source, AstNode node) : super(source, node);
+
+ @override
+ String get description => 'implicit use of `this`';
+
+ @override
+ EdgeOriginKind get kind => EdgeOriginKind.implicitThis;
+}
+
/// Edge origin resulting from the inference of a type parameter, which
/// can affects the nullability of that type parameter's bound.
class InferredTypeParameterInstantiationOrigin extends EdgeOrigin {
diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart
index 7d9e21f..7089d06 100644
--- a/pkg/nnbd_migration/test/api_test.dart
+++ b/pkg/nnbd_migration/test/api_test.dart
@@ -2116,6 +2116,51 @@
await _checkSingleFileChanges(content, expected, removeViaComments: true);
}
+ Future<void> test_extension_extended_type_nullability_intent() async {
+ var content = '''
+extension E on C {
+ String foo() => this.bar();
+}
+
+class C {
+ String bar() => null;
+}
+
+void test(C c, bool b) {
+ if (b) {
+ c.foo();
+ }
+}
+
+main() {
+ test(null, false);
+}
+''';
+ // The call to `bar` from `foo` should be taken as a demonstration that the
+ // extension E is not intended to apply to nullable types, so the call to
+ // `foo` should be null checked.
+ var expected = '''
+extension E on C {
+ String? foo() => this.bar();
+}
+
+class C {
+ String? bar() => null;
+}
+
+void test(C? c, bool b) {
+ if (b) {
+ c!.foo();
+ }
+}
+
+main() {
+ test(null, false);
+}
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
Future<void> test_extension_null_check_non_nullable() async {
var content = '''
class C {}
diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart
index e943de9..9558d4e 100644
--- a/pkg/nnbd_migration/test/edge_builder_test.dart
+++ b/pkg/nnbd_migration/test/edge_builder_test.dart
@@ -2952,6 +2952,87 @@
// adding assertion(s).
}
+ Future<void> test_extension_this_non_null_intent_explicit_direct() async {
+ await analyze('''
+extension on int {
+ f() => g(this);
+}
+void g(int i) {}
+''');
+ assertEdge(decoratedTypeAnnotation('int {').node,
+ decoratedTypeAnnotation('int i').node,
+ hard: true);
+ }
+
+ Future<void> test_extension_this_non_null_intent_explicit_method() async {
+ await analyze('''
+extension on int {
+ f() => this.abs();
+}
+''');
+ assertEdge(decoratedTypeAnnotation('int').node, never, hard: true);
+ }
+
+ Future<void>
+ test_extension_this_non_null_intent_explicit_property_get() async {
+ await analyze('''
+extension on int {
+ f() => this.isEven;
+}
+''');
+ assertEdge(decoratedTypeAnnotation('int').node, never, hard: true);
+ }
+
+ Future<void>
+ test_extension_this_non_null_intent_explicit_property_set() async {
+ await analyze('''
+class C {
+ int x;
+}
+extension on C /*reference*/ {
+ f() {
+ this.x = 0;
+ }
+}
+''');
+ assertEdge(decoratedTypeAnnotation('C /*reference*/').node, never,
+ hard: true);
+ }
+
+ Future<void> test_extension_this_non_null_intent_implicit_method() async {
+ await analyze('''
+extension on int {
+ f() => abs();
+}
+''');
+ assertEdge(decoratedTypeAnnotation('int').node, never, hard: true);
+ }
+
+ Future<void> test_extension_this_non_null_intent_implicit_property() async {
+ await analyze('''
+extension on int {
+ f() => isEven;
+}
+''');
+ assertEdge(decoratedTypeAnnotation('int').node, never, hard: true);
+ }
+
+ Future<void>
+ test_extension_this_non_null_intent_implicit_property_set() async {
+ await analyze('''
+class C {
+ int x;
+}
+extension on C /*reference*/ {
+ f() {
+ x = 0;
+ }
+}
+''');
+ assertEdge(decoratedTypeAnnotation('C /*reference*/').node, never,
+ hard: true);
+ }
+
Future<void> test_field_final_does_not_override_setter() async {
await analyze('''
abstract class A {
@@ -7712,11 +7793,11 @@
}
''');
expect(
- assertEdge(anyNode, decoratedTypeAnnotation('int f1').node, hard: false)
+ assertEdge(anyNode, decoratedTypeAnnotation('int f1').node, hard: true)
.sourceNode,
isNot(never));
expect(
- assertEdge(anyNode, decoratedTypeAnnotation('int f2').node, hard: false)
+ assertEdge(anyNode, decoratedTypeAnnotation('int f2').node, hard: true)
.sourceNode,
never);
expect(hasNullCheckHint(findNode.this_('this/*!*/')), isTrue);
