Autocomplete for overriding methods does not show if the method has non-imported types

[osaxma]

Describe the bug

Assume we've this on a.dart file:

import 'dart:async';

abstract class Base {
  FutureOr<String> method() => '';
}

And on b.dart file we are trying to overide Base.method:

import 'a.dart';

class BaseImpl extends Base {
  
  // <~~ typing the word `method` here does not show autocomplete suggestions 

} 

i.e.:

Though, if we import dart:async in b.dart, the autocomplete for override works.

The same issue would occur if the non-imported type is an argument of the method instead of a return type.

Expected Behavior:

I would expect the autocomplete to show up, and once it's selected, it'll import all the necessary libraries (i.e. dart:async in this case)

---

Please complete the following information:

• VS Code version: Version: 1.70.1
• Dart extension version: v3.46.1
• Dart/Flutter SDK version: Dart SDK version: 2.17.6 (stable) (Tue Jul 12 12:54:37 2022 +0200) on "macos_x64"

[DanTup]

@bwilkerson @scheglov the item doesn't show up because of this condition here:

https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/services/completion/dart/suggestion_builder.dart#L896-L899

The edit produces for this override includes multiple edits (some for adding `imports). We could alter this condition to include the item anyway (ignoring the edits that aren't at the location of the completion) but that would leave code with analysis errors (which perhaps is better than not suggesting the item, though it's not ideal).

There's some existing code used by NotImportedContributor to attach URIs to be imported to suggestions but this only supports a single URI and in theory there could be multiple here. We could add an additional field to CompletionSuggestion (to be populated in getSuggestions2, and be passable to getSuggestionDetails2) - something like List<String> importUris, although if we some questions I wonder if you have opinions on:

• Should we continue using libraryUri (in the clients) for the primary/not-imported URI or deprecate it and use the list for both not-imported URI and "additionally required URIs"?
• If we keep it (as the location the completing element exists in), should its value also be included in the list?
• Is it reasonable to have some flag on DartFileEditBuilderImpl to suppress edits being generated for imports (and instead provide a mechanism to just read the list)?
• do we need to be concerned about prefixes, or do we never generate prefixed imports now?

[bwilkerson]

We could alter this condition to include the item anyway ... but that would leave code with analysis errors ...

That wouldn't be terrible. We have a fix for undefined names that can add the import after the fact. The question is, is it better to not have the suggestion, or is it better to have the suggestion and require more work after the fact? I'm leaning toward having the suggestion.

... attach URIs to be imported to suggestions but this only supports a single URI ...

So we could get a little better by adding one of the required imports and have one fewer import to manually add. I don't know what percentage of the time there's only one missing import, but there would be no manual cleanup in such cases. So it sounds like a win, but I don't know how much of a win or whether it's worth the effort.

We could add an additional field to CompletionSuggestion ...

I don't have any problem with doing that, though we might want to combine this with the idea of using a protocol-agnostic object. We'd probably want to ensure that the first URI in the list is always the URI that would go in libraryUri in order to preserve our current behavior.

• Should we continue using libraryUri (in the clients) for the primary/not-imported URI or deprecate it and use the list for both not-imported URI and "additionally required URIs"?
• If we keep it (as the location the completing element exists in), should its value also be included in the list?

If we don't create a protocol-agnostic suggestion, then we're probably better off continuing to populate libraryUri as we currently do and make the additional field be a complete list, including the URI in libraryUri. That will probably be the least error-prone approach.

• Is it reasonable to have some flag on DartFileEditBuilderImpl to suppress edits being generated for imports (and instead provide a mechanism to just read the list)?

I'm not enthusiastic about that idea, but I'm not sure I understand the problem you're trying to solve to know whether there's a better way.

• do we need to be concerned about prefixes, or do we never generate prefixed imports now?

I don't believe that we ever generate prefixes now. We do use prefixes in references when the library is already imported with a prefix (to avoid having to add another import), but I think at the moment we assume that there won't be collisions. I don't remember seeing any issues about that, so I'm guessing that it isn't a problem for users.

[DanTup]

I agree that showing the completion without imports is better than not showing it at all (as is the case today), and that adding the "first" import is better again. Ofc, I'd prefer we added them all :)

That said, I'm reminded about #49081 and wonder if adding imports for all the src files might look bad, so wonder if we shoud skip the imports for now (but fix that the completion doesn't appear) and then try to address that before adding automatic imports here - WDYT?

Is it reasonable to have some flag on DartFileEditBuilderImpl to suppress edits being generated for imports (and instead provide a mechanism to just read the list)?

I'm not enthusiastic about that idea, but I'm not sure I understand the problem you're trying to solve to know whether there's a better way.

To add the completion without any imports, we need to remove that original condition. However to do that, we need to know which edits are the import edits (so we can ignore them to get the "real" edit to get the replacement text) or suppress them. Suppressing them seems like the most reliable way.

When we implement the auto-imports, we similarly need to exclude the import edits from the original edit that goes to the client (at least for LSP) and handle the auto-imports separately. So in both cases it seems like it's useful to generate the main edits without the import edits.

[bwilkerson]

... wonder if we shoud skip the imports for now ...

My inclination would be to implement the feature the way we want it to work, even if there's a bug that makes is suboptimal, rather than to make it less functional and hope that we remember to enhance the feature when the result would be more optimal.

... so we can ignore them to get the "real" edit to get the replacement text ...

Right. So the underlying problem is that we're trying to reuse some logic from quick fixes that isn't working quite the way we need it to for code completion; two features that aren't quite compatible.

While I can think of some kludgy work-arounds, I'd rather have a clean approach, and I think the one you proposed is probably the best: add a new parameter to ChangeBuilder.addDartFileEdit that indicates whether imports should automatically be generated as edits, and then some support in DartFileEditBuilder to return the libraries that need to be imported (or the edits for them, or whatever it turns out we actually need.