Repository navigation
Autocomplete for overriding methods does not show if the method has non-imported types #4116
Description
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"
Activity
[-]Autocomplete for overriding for method does not show if method has non-imported types[/-][+]Autocomplete for overriding methods does not show if the method has non-imported types[/+]DanTup commentedon Nov 9, 2022
@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
NotImportedContributorto 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 toCompletionSuggestion(to be populated ingetSuggestions2, and be passable togetSuggestionDetails2) - something likeList<String> importUris, although if we some questions I wonder if you have opinions on: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"?DartFileEditBuilderImplto suppress edits being generated for imports (and instead provide a mechanism to just read the list)?bwilkerson commentedon Nov 11, 2022
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.
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.
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
libraryUriin order to preserve our current behavior.If we don't create a protocol-agnostic suggestion, then we're probably better off continuing to populate
libraryUrias we currently do and make the additional field be a complete list, including the URI inlibraryUri. That will probably be the least error-prone approach.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.
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 commentedon Nov 14, 2022
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
srcfiles 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?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
importedits (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
importedits 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 theimportedits.bwilkerson commentedon Nov 14, 2022
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.
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.addDartFileEditthat indicates whether imports should automatically be generated as edits, and then some support inDartFileEditBuilderto return the libraries that need to be imported (or the edits for them, or whatever it turns out we actually need.DanTup commentedon Nov 16, 2022
Prefixes etc are respected (since they don't produce additional imports), although we still don't consider collisions (as noted above, that's not specific to this).
[analysis_server] Fix override completions being suppressed because o…
[analysis_server] Support adding multiple imports for @OVERRIDES comp…
DanTup commentedon Nov 21, 2022
dart-lang/sdk@1d64cba addresses the items not showing up in completion, and dart-lang/sdk@699b42c adds support for bringing in potentially multiple)
imports so the code is valid.These are SDK changes, so will require a future SDK release to show up. Thanks for raising this and figuring out what was triggering it - it saved me some time debugging! :-)