languages/dart-sdk
languages/dart-sdk
1
0
Fork 0
mirror of https://github.com/dart-lang/sdk synced 2024-10-14 09:58:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

Migration: consider null-aware access an indication of nullability for parameters on public methods.

Browse source 
If a parameter of a public method is accessed using null-aware access
(`?.`), the migration tool now considers that to be an indication that
the parameter should be nullable.

Rationale: when migration doesn't have access to the entire code base,
the migration tool's approach of propagating nullability forward
through the program doesn't always work, because it's possible that
the sources of nulls are not visible to the migration tool.  This has
often resulted in the migration tool marking a function parameter as
non-nullable, in spite of the fact that the use of `?.` in the method
body makes it clear that it's intended to be nullable.

This new heuristic is only applied to public methods; for private
methods there's no chance of there being callers outside of the code
that's immediately visible to the migration tool, so the problem
doesn't arise.  For local functions and closures, the situation is
less clear-cut, but of the examples I've looked through so far in
Google's internal code base, it seems like more often than not, the
better behavior is to continue erring on the side of non-nullability.

Bug: https://github.com/dart-lang/sdk/issues/49601
Change-Id: I76531591ce0b3eb9fe62273130aa45eb4ff6d456
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253864
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
This commit is contained in:
Paul Berry 2022-08-19 21:40:45 +00:00 committed by Commit Bot
parent 8b4e9c38a0
commit 162091bd92
6 changed files with 353 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
pkg/nnbd_migration/lib/instrumentation.dart
View file

@ -285,6 +285,7 @@ enum EdgeOriginKind {
nonNullableUsage,
nonNullAssertion,
nullabilityComment,
nullAwareAccess,
optionalFormalParameter,
parameterInheritance,
quiverCheckNotNull,

29
pkg/nnbd_migration/lib/src/edge_builder.dart
View file

@ -142,6 +142,10 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
/// information used in flow analysis. Otherwise `null`.
AssignedVariables<AstNode, PromotableElement>? _assignedVariables;
/// The outermost function or method being visited, or `null` if the visitor
/// is not inside any function or method.
ExecutableElement? _currentExecutable;
/// The [DecoratedType] of the innermost function or method being visited, or
/// `null` if the visitor is not inside any function or method.
///
@ -1036,6 +1040,8 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
var previousPostDominatedLocals = _postDominatedLocals;
var previousElementsWrittenToInLocalFunction =
_elementsWrittenToInLocalFunction;
var previousExecutable = _currentExecutable;
_currentExecutable ??= node.declaredElement;
try {
if (node.parent is! FunctionDeclaration) {
_elementsWrittenToInLocalFunction = {};
@ -1060,6 +1066,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
_currentFieldFormals = previousFieldFormals;
_currentFunctionExpression = previousFunction;
_postDominatedLocals = previousPostDominatedLocals;
_currentExecutable = previousExecutable;
}
}
@ -1394,7 +1401,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
} else if (calleeIsStatic) {
_dispatch(target);
} else if (isNullAware) {
targetType = _dispatch(target);
targetType = _handleNullAwareTarget(target, node);
} else {
targetType = _handleTarget(target, node.methodName.name, callee);
}
@ -2624,6 +2631,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
ConstructorName? redirectedConstructor) {
assert(_currentFunctionType == null);
assert(_currentFieldFormals.isEmpty);
assert(_currentExecutable == null);
_dispatchList(metadata);
_dispatch(returnType);
_createFlowAnalysis(node, parameters);
@ -2632,6 +2640,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
_currentFieldFormals = declaredElement is ConstructorElement
? _computeFieldFormalMap(declaredElement)
: const {};
_currentExecutable = declaredElement;
_addParametersToFlowAnalysis(parameters);
// Push a scope of post-dominated declarations on the stack.
_postDominatedLocals.pushScope(elements: declaredElement.parameters);
@ -2726,6 +2735,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
_currentFunctionType = null;
_currentFieldFormals = const {};
_postDominatedLocals.popScope();
_currentExecutable = null;
}
}
@ -3105,6 +3115,21 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
return calleeType.returnType;
}
DecoratedType? _handleNullAwareTarget(Expression? target, Expression node) {
var targetType = _dispatch(target);
if (target is SimpleIdentifier) {
var targetElement = target.staticElement;
if (targetElement is ParameterElement &&
targetElement.enclosingElement3 == _currentExecutable &&
!_currentExecutable!.name.startsWith('_')) {
_graph.makeNullable(
_variables.decoratedElementType(targetElement).node!,
NullAwareAccessOrigin(source, node));
}
}
return targetType;
}
DecoratedType? _handleNullCheckHint(
Expression expression, DecoratedType? type) {
// Sometimes we think we're looking at an expression but we're really not
@ -3157,7 +3182,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
} else if (calleeIsStatic) {
_dispatch(target);
} else if (isNullAware) {
targetType = _dispatch(target);
targetType = _handleNullAwareTarget(target, node);
} else {
targetType = _handleTarget(target, propertyName, callee);
}

11
pkg/nnbd_migration/lib/src/edge_origin.dart
View file

@ -561,6 +561,17 @@ class NullabilityCommentOrigin extends EdgeOrigin {
EdgeOriginKind get kind => EdgeOriginKind.nullabilityComment;
}
/// An edge origin arising from a null aware access.
class NullAwareAccessOrigin extends EdgeOrigin {
NullAwareAccessOrigin(super.source, AstNode super.node);
@override
String get description => 'null aware access';
@override
EdgeOriginKind get kind => EdgeOriginKind.nullAwareAccess;
}
/// Edge origin resulting from the presence of an optional formal parameter.
///
/// For example, in the following code snippet:

310
pkg/nnbd_migration/test/api_test.dart
View file

@ -7232,6 +7232,94 @@ void g(MapGetter? mapGetter) {
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_call_on_closure_param_not_nullable() async {
// The null-aware access on `i` is *not* considered a strong enough signal
// that `i` is meant to be nullable, because the migration tool can see all
// callers of the closure, so it can tell whether it needs to be nullable or
// not.
//
// (Note: this is not strictly true, because the closure could be called
// from elsewhere. But it's a heuristic that seems to be usually right in
// the cases we've found so far.)
var content = '''
main() {
var x = (int i) => i?.abs();
}
''';
var expected = '''
main() {
var x = (int i) => i.abs();
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_call_on_local_param_not_nullable() async {
// The null-aware access on `i` is *not* considered a strong enough signal
// that `i` is meant to be nullable, because the migration tool can see all
// callers of `f`, so it can tell whether it needs to be nullable or not.
//
// (Note: this is not strictly true, because the local function could be
// torn off and called from elsewhere. But it's a heuristic that seems to
// be usually right in the cases we've found so far.)
var content = '''
main() {
int f(int i) => i?.abs();
}
''';
var expected = '''
main() {
int f(int i) => i.abs();
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_call_on_private_param_not_nullable() async {
// The null-aware access on `i` is *not* considered a strong enough signal
// that `i` is meant to be nullable, because the migration tool can see all
// callers of `_f`, so it can tell whether it needs to be nullable or not.
var content = 'int _f(int i) => i?.abs();';
var expected = 'int _f(int i) => i.abs();';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_call_on_public_param_implies_nullable() async {
// The null-aware access on `i` is considered a strong signal that `i` is
// meant to be nullable.
var content = 'int f(int i) => i?.abs();';
var expected = 'int? f(int? i) => i?.abs();';
await _checkSingleFileChanges(content, expected);
}
Future<void>
test_null_aware_call_on_public_param_overridable_by_hint() async {
// The null-aware access on `i` is considered a strong signal that `i` is
// meant to be nullable, but an explicit `/*!*/` is a stronger signal.
var content = 'int f(int/*!*/ i) => i?.abs();';
var expected = 'int f(int i) => i.abs();';
await _checkSingleFileChanges(content, expected);
}
Future<void>
test_null_aware_call_on_public_param_overridable_by_intent() async {
// The null-aware access on `i` is considered a strong signal that `i` is
// meant to be nullable, but non-null intent is a stronger signal.
var content = '''
int f(int i) {
print(i + 1);
return i?.abs();
}
''';
var expected = '''
int f(int i) {
print(i + 1);
return i.abs();
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_call_tearoff() async {
// Kind of a weird use case because `f?.call` is equivalent to `f`, but
// let's make sure we analyze it correctly.
@ -7241,6 +7329,93 @@ void g(MapGetter? mapGetter) {
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_get_on_closure_param_not_nullable() async {
// The null-aware access on `i` is *not* considered a strong enough signal
// that `i` is meant to be nullable, because the migration tool can see all
// callers of the closure, so it can tell whether it needs to be nullable or
// not.
//
// (Note: this is not strictly true, because the closure could be called
// from elsewhere. But it's a heuristic that seems to be usually right in
// the cases we've found so far.)
var content = '''
main() {
var x = (int i) => i?.isEven;
}
''';
var expected = '''
main() {
var x = (int i) => i.isEven;
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_get_on_local_param_not_nullable() async {
// The null-aware access on `i` is *not* considered a strong enough signal
// that `i` is meant to be nullable, because the migration tool can see all
// callers of `f`, so it can tell whether it needs to be nullable or not.
//
// (Note: this is not strictly true, because the local function could be
// torn off and called from elsewhere. But it's a heuristic that seems to
// be usually right in the cases we've found so far.)
var content = '''
main() {
bool f(int i) => i?.isEven;
}
''';
var expected = '''
main() {
bool f(int i) => i.isEven;
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_get_on_private_param_not_nullable() async {
// The null-aware access on `i` is *not* considered a strong enough signal
// that `i` is meant to be nullable, because the migration tool can see all
// callers of `_f`, so it can tell whether it needs to be nullable or not.
var content = 'bool _f(int i) => i?.isEven;';
var expected = 'bool _f(int i) => i.isEven;';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_get_on_public_param_implies_nullable() async {
// The null-aware access on `i` is considered a strong signal that `i` is
// meant to be nullable.
var content = 'bool f(int i) => i?.isEven;';
var expected = 'bool? f(int? i) => i?.isEven;';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_get_on_public_param_overridable_by_hint() async {
// The null-aware access on `i` is considered a strong signal that `i` is
// meant to be nullable, but an explicit `/*!*/` is a stronger signal.
var content = 'bool f(int/*!*/ i) => i?.isEven;';
var expected = 'bool f(int i) => i.isEven;';
await _checkSingleFileChanges(content, expected);
}
Future<void>
test_null_aware_get_on_public_param_overridable_by_intent() async {
// The null-aware access on `i` is considered a strong signal that `i` is
// meant to be nullable, but non-null intent is a stronger signal.
var content = '''
bool f(int i) {
print(i + 1);
return i?.isEven;
}
''';
var expected = '''
bool f(int i) {
print(i + 1);
return i.isEven;
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_getter_invocation() async {
var content = '''
bool f(int i) => i?.isEven;
@ -7273,6 +7448,141 @@ main() {
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_set_on_closure_param_not_nullable() async {
// The null-aware access on `c` is *not* considered a strong enough signal
// that `c` is meant to be nullable, because the migration tool can see all
// callers of the closure, so it can tell whether it needs to be nullable or
// not.
//
// (Note: this is not strictly true, because the closure could be called
// from elsewhere. But it's a heuristic that seems to be usually right in
// the cases we've found so far.)
var content = '''
class C {
int i = 0;
}
main() {
var x = (C c) { c?.i = 0; };
}
''';
var expected = '''
class C {
int i = 0;
}
main() {
var x = (C c) { c.i = 0; };
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_set_on_local_param_not_nullable() async {
// The null-aware access on `c` is *not* considered a strong enough signal
// that `c` is meant to be nullable, because the migration tool can see all
// callers of `f`, so it can tell whether it needs to be nullable or not.
//
// (Note: this is not strictly true, because the local function could be
// torn off and called from elsewhere. But it's a heuristic that seems to
// be usually right in the cases we've found so far.)
var content = '''
class C {
int i = 0;
}
main() {
void f(C c) { c?.i = 0; }
}
''';
var expected = '''
class C {
int i = 0;
}
main() {
void f(C c) { c.i = 0; }
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_set_on_private_param_not_nullable() async {
// The null-aware access on `c` is *not* considered a strong enough signal
// that `c` is meant to be nullable, because the migration tool can see all
// callers of `_f`, so it can tell whether it needs to be nullable or not.
var content = '''
class C {
int i = 0;
}
void _f(C c) { c?.i = 0; }
''';
var expected = '''
class C {
int i = 0;
}
void _f(C c) { c.i = 0; }
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_set_on_public_param_implies_nullable() async {
// The null-aware access on `c` is considered a strong signal that `c` is
// meant to be nullable.
var content = '''
class C {
int i = 0;
}
void f(C c) { c?.i = 0; }
''';
var expected = '''
class C {
int i = 0;
}
void f(C? c) { c?.i = 0; }
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_set_on_public_param_overridable_by_hint() async {
// The null-aware access on `c` is considered a strong signal that `c` is
// meant to be nullable, but an explicit `/*!*/` is a stronger signal.
var content = '''
class C {
int i = 0;
}
void f(C/*!*/ c) { c?.i = 0; }
''';
var expected = '''
class C {
int i = 0;
}
void f(C c) { c.i = 0; }
''';
await _checkSingleFileChanges(content, expected);
}
Future<void>
test_null_aware_set_on_public_param_overridable_by_intent() async {
// The null-aware access on `c` is considered a strong signal that `c` is
// meant to be nullable, but non-null intent is a stronger signal.
var content = '''
class C {
int i = 0;
}
void f(C c) {
print(c.i);
c?.i = 0;
}
''';
var expected = '''
class C {
int i = 0;
}
void f(C c) {
print(c.i);
c.i = 0;
}
''';
await _checkSingleFileChanges(content, expected);
}
Future<void> test_null_aware_setter_invocation_null_target() async {
var content = '''
class C {

4
pkg/nnbd_migration/test/front_end/info_builder_test.dart
View file

@ -1032,9 +1032,9 @@ int f(int/*!*/ x, int y) => x ??= y;
void test_nullAwarenessUnnecessaryInStrongMode() async {
var unit = await buildInfoForSingleTestFile('''
int f(String s) => s?.length;
int f(String/*!*/ s) => s?.length;
''', migratedContent: '''
int f(String s) => s?.length;
int f(String/*!*/ s) => s?.length;
''', warnOnWeakCode: true);
var question = '?';
var questionOffset = unit.content!.indexOf(question);

4
pkg/nnbd_migration/test/front_end/unit_renderer_test.dart
View file

@ -354,9 +354,9 @@ int? f(List<int > values)
void test_nullAwarenessUnnecessaryInStrongMode() async {
await buildInfoForSingleTestFile('''
int f(String s) => s?.length;
int f(String/*!*/ s) => s?.length;
''', migratedContent: '''
int f(String s) => s?.length;
int f(String/*!*/ s) => s?.length;
''', warnOnWeakCode: true);
var output = renderUnits()[0];
expect(_stripDataAttributes(output.regions!),