Missing record field completion with switch expression and closures

[FMorschel]

Similar to https://github.com/dart-lang/sdk/issues/60604

Repro:

void f(num n) {
  ({int first, String second}) v = switch (n) {
    double() => (/*0*/),
    int() => () {
      return (/*1*/);
    }(),
  };

  ({int first, String second}) v2 = () {
    return (/*2*/);
  }();

  ({int first, String second}) v3 = (/*3*/);
}

For the above cases, only 3 shows the first: and second: completion items; the other cases don't suggest the record fields.

I'll take a look at this.

[FMorschel]

The first case was easy (about switch expressions). But the one about FunctionExpressionInvocation is on the resolver. I'm not sure how to tackle this one. The given contextType is the right one, but I don't think it is been passed on to the expected place for this to work.

https://github.com/dart-lang/sdk/blob/bddcddfe6b0914c7f234cd4400e1e70e8a72d374/pkg/analyzer/lib/src/generated/resolver.dart#L2953-L2983

I'm testing with

void f() {
  String f = () {
    return ^;
  }();
}

Which at the carret, we expect String.

@stereotype441, can you help me with this, or ping someone else, please? Thanks!

[FMorschel]

@bwilkerson, just to be sure you agree:

Future<({int first, String second})> foo() async => (^);

In that position, should it also suggest the field names? At that point the type should be considered FutureOr<({int first, String second})> from what I understand here.

[bwilkerson]

If we have a performant way of getting the field names, then yes, it would be good to offer the field names there.

[stereotype441]

@stereotype441, can you help me with this, or ping someone else, please? Thanks!

I'm quite familiar with how the resolver works, but not too familiar with what information the completion engine is consulting in order to decide what completions to give. Considering this case:

void f() {
  String f = () {
    return ^;
  }();
}

My guess is that the completion engine would do something like this:

• See that the cursor is in a return statement
• Go up the AST to the containing BlockFunctionBody, and look at its parent, which is a FunctionExpression.
• Look at the FunctionExpression's declaredFragment, which is an ExecutableFragment.
• Look at the ExecutableFragment's element, which is an ExecutableElement.
• Look at the ExecutableElement's returnType, which should then give enough information to figure out what completions to offer to the user.

Did I guess right? If so, which of these steps isn't working, and what are you seeing instead? If not, what is the completion engine doing to try to figure out what completions to offer?

[bwilkerson]

I think that's essentially correct, but it uses InScopeCompletionPass._computeContextType to do all the work, so that's what any other uses of the context type should also use.

[FMorschel]

I'm working on this at https://dart-review.googlesource.com/c/sdk/+/434124.

If on patchset 1 you run pkg\analysis_server\test\src\services\completion\dart\feature_computer_test.dart (annotated with soloTest already), with a breakpoint at pkg\analysis_server\lib\src\services\completion\dart\feature_computer.dart line 1084, you'll see:

if (node.returnKeyword.end < offset) {
  var functionBody = node.thisOrAncestorOfType<FunctionBodyImpl>();
  if (functionBody != null) {
    return functionBody.bodyContext?.contextType;
  }
}

Here, contextType is null:

But from what I could find (this is why I'm asking you/the analyzer team for help), I think the problem may be happening at pkg\analyzer\lib\src\generated\resolver.dart line 2958:

https://github.com/dart-lang/sdk/blob/4b3c8a3e371d57df73ae396ccae38aa531d415b5/pkg/analyzer/lib/src/generated/resolver.dart#L2953-L2983

I'm just out of my current depth here, so I'll need some guidance. I could probably work around it on the code I pasted above (going up the nodes so I find the invocation and the expected type), but I'm not sure that would be the correct approach.

[stereotype441]

Thanks for the context, @FMorschel, that really helps!

What's happening here is that the analysis server's completion engine is doing something a little bit dicey, which is that it's relying on some of the intermediate data used by type inference in order to guess what completions would be useful to offer to the user. The reason that's a little bit dicey is because type inference and code completion have such different design goals. Type inference needs to be simple (so that it's easy to reason about when making improvements to the language), conservative (so that it doesn't come to unwarranted conclusions that would break the soundness of the compiler), and stable (so that it behaves the same from one release of Dart to the next, avoiding breaking user code). Whereas the completion engine ought to take advantage of as much information as possible so that it can give the user the most useful possible completions, and it doesn't need to worry so much about being simple, conservative, or stable.

To see how that's manifesting in this specific instance, let's look at the test case that's failing:

void f() {
  String f = () {
    return ^;
  }();
}

The expression () { return ...; }() is a FunctionExpressionInvocation whose subexpression is the FunctionExpression () { return ...; }. The type inference rule for a FunctionExpressionInvocation is to discard the incoming context and analyze the FunctionExpression using an empty context. Which means that when the FunctionExpression gets analyzed, the type String has been completely lost. That's why functionBody.bodyContext.contextType is null. So in a sense, you're totally right that the blame for this issue is at least partly in ResolverVisitor.visitFunctionExpressionInvocation. If it propagated the incoming context instead of discarding it, then the completion engine would be able that the return type was String.

But we can't fix this issue by changing the behavior of ResolverVisitor.visitFunctionExpressionInvocation, because that would change the type inference rules for function expression invocations, and that would change the meaning of existing programs. (We might one day decide to change it, but if we do, we would almost certainly do it as a language versioned change, so that we don't break existing code).

I could probably work around it on the code I pasted above (going up the nodes so I find the invocation and the expected type), but I'm not sure that would be the correct approach.

I think this would be the correct approach. I don't even consider it a workaround. It's just an unfortunate fact of life that since completion and type inference have such different design goals, sometimes completion can't use the data left behind by type inference, and needs to augment it with its own analysis.

Does this all make sense? Let me know if any of it doesn't, and I'd be glad to say more.

[FMorschel]

Thanks for the detailed explanation!

Does this all make sense?

Yes, it does. I'll see to it then, thanks again!

[FMorschel]

For the cases I mentioned above, I have it working as it should. I was just reviewing FutureOr case, and I got to this:

FutureOr<({int foo01, String foo02})> f() => (foo0^);

No matter where I try to get this information from, it seems to be unable to tell me what type is expected. Any tips?

Edit: The CL is up to date with my changes.

[stereotype441]

No matter where I try to get this information from, it seems to be unable to tell me what type is expected. Any tips?

Edit: The CL is up to date with my changes.

Can you give more detailed repro instructions? I just downloaded patchset 2 of https://dart-review.googlesource.com/c/sdk/+/434124, and for me, pkg\analysis_server\test\src\services\completion\dart\feature_computer_test.dart runs without any failures.

[FMorschel]

I've added a FailingTest annotation to both cases where it is failing, sorry I didn't say so. The failing tests are:

• test_futureOrRecord in pkg/analysis_server/test/src/services/completion/dart/feature_computer_test.dart
• test_futureOr_suggests_fieldNames in pkg/analysis_server/test/services/completion/dart/location/record_literal_test.dart

[stereotype441]

Aha, I see it! You're missing an import.

This fixes the problem:

$ git diff
diff --git a/pkg/analysis_server/test/services/completion/dart/location/record_literal_test.dart b/pkg/analysis_server/test/services/completion/dart/location/record_literal_test.dart
index 2bf2e2ba5f9..c38b8154a62 100644
--- a/pkg/analysis_server/test/services/completion/dart/location/record_literal_test.dart
+++ b/pkg/analysis_server/test/services/completion/dart/location/record_literal_test.dart
@@ -451,9 +451,10 @@ suggestions
 ''');
   }
 
-  @FailingTest()
   Future<void> test_futureOr_suggests_fieldNames() async {
     await computeSuggestions('''
+import 'dart:async';
+
 FutureOr<({int foo01, String foo02})> f() => (foo0^);
 ''');
     assertResponse(r'''
diff --git a/pkg/analysis_server/test/src/services/completion/dart/feature_computer_test.dart b/pkg/analysis_server/test/src/services/completion/dart/feature_computer_test.dart
index 2217c6d6769..3f9f09cc428 100644
--- a/pkg/analysis_server/test/src/services/completion/dart/feature_computer_test.dart
+++ b/pkg/analysis_server/test/src/services/completion/dart/feature_computer_test.dart
@@ -614,9 +614,10 @@ void f() {
 ''', 'String');
   }
 
-  @FailingTest()
   Future<void> test_futureOrRecord() async {
     await assertContextType('''
+import 'dart:async';
+
 FutureOr<({int field})> f() => ^;
 ''', 'FutureOr<({int field})>');
   }

[FMorschel]

Oh 🤦. For assists/fixes, I get an error when a library has errors like that, so I see these errors.

Thanks a lot!