sdk
sdk copied to clipboard
Offer a list of potential overridable methods when typing @override
Raised by @escamoteur at https://github.com/Dart-Code/Dart-Code/issues/736:
It would be very comfortable if when overriding a method in an extended class if DartCode could offer a list of potential candidates via intellisense and completes after selection of one of them the whole method.
One way of implementing this might be to prefix all override items with @override in their displayLabel so that if you're typing @override the completion list might look like this:
- @override myMethod1
- @override myMethod2
I think the displayName property was added specifically to remove @override but now I'm thinking that it's actually useful (as long as it's not on its own line).
If there are concerns over ordering or filtering, it could also be added to the end.
Actually, it also just occurred to me that I have metadata for this so I might be able to set the filterText for these options to include @override without changing the display. I'll test it out and report back!
I tried this:
if (suggestion.kind === "OVERRIDE") {
completion.filterText = "@override " + completion.label;
}
However, it doesn't seem to work because given this code:
abstract class Person {
getName() {}
}
class Danny extends Person {
// Cursor here
}
getName doesn't appear in the completion list unless I've already typed @override\n. I think it probably should (@pq?), and that would allow the user to see a filtered list of all overridable methods as they start typing @override?
The current implementation in server appears to be missing a couple of triggers. If you type part of a method name (with nothing else; even something as short as g) you should get back suggestions for all of the inherited methods. This is because server explicitly looks for the AST structure that is produced in those cases to determine that this is what the user is likely trying to do.
But if there is nothing in the body of a class, or between two methods, then server will get a different AST structure and we haven't written the code to recognize it. The same is probably true of the AST we get when the user has typed @override. We just need to add the code to recognize the additional AST structures that should cause these suggestions to be produced.
I agree that it would be nice to have these suggestions displayed in more cases.
If you'd like to take a stab at adding this support I'd be happy to walk you through the process.
A less ambitious first step might be to add some failing tests for these cases (they would go in https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/test/services/completion/dart/override_contributor_test.dart).
Neither is required, of course. :-)
Sure, I can take a stab at adding some failing tests :-)
I've opened https://dart-review.googlesource.com/c/sdk/+/52442 to get things started.
@DanTup Did that CL (which has been merged) cover all the cases? If so, we can probably close this issue.
I believe it covers the case I spotted - though it was only failing tests, no implementation.
Ok. Then I'll close this issue and we can open a new one if we discover other work that needs to be done.
This came up again - I think there was a misunderstanding in this issue - my change above only added some tests that failed because of this issue, no implementation. I think there should be an open issue tracking adding an implementation/fixing the tests.
@bwilkerson if you can provide some pointers, I don't mind having a look at it (at least, when othe priorities are done!).
These kind of completions are added by the class in https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/services/completion/dart/override_contributor.dart.
I did some digging into this, but it seems non-trivial to fix. There override completion code expects to find an existing partial identifier (targetId) in order to provide completions, however there are situations where we don't have one:
class Foo {
@override^
}
Here, the nodes we have are simple string tokens for "@", "override" and "}", and:
class Foo {
@override^
existingMethod() {
}
}
Here, the override annotation is parsed as being on existingMethod but in this case we wanted to override an additional method.
I think to support these we might have to add some special cases (to cover the string tokens when the existing code doesn't parse into a valid annotation, and to cover the case where there's an override annotation attached to a method but completion was invoke at least one newline before the method name) but I don't know whether we'd end up doing the wrong thing in some cases (or that the code would be fragile).
WDYT?
I'm happy to have override completions suggested in more situations, as long as we're careful to not produce them in cases where it's clearly not appropriate. We want to make sure the list of suggestions contains the most likely completions and not just every possible completion. It's kind of a fine line to walk.
@jwren @lambdabaa have been thinking about this recently and might have thoughts.
Any news on this?
I had a little dig around here again when https://github.com/Dart-Code/Dart-Code/issues/1955 came up, but solving that case is difficult because of the parsed AST.
I do think there are some improvements that can be made (some mentioned above) handling some more special cases, but I haven't had chance to properly try to implement or test them.
This would be great, it seems to work inconsistently on VS Code.
any luck? This is the only reason for me using Intellij instead of Vscode
It appears this has been fixed at some point. Using the example from above, the super classes methods show up:
@DanTup I believe this issue was about a different behavior. The one that you mention does really work for quite a long time, but it only works when you write the name of the method you want to override.
But there's no way to get the list of all the methods you can override, e.g. by typing @overr...:
So when you don't remember the name of the method, you have to actually go to the class implementation and check all the methods.
E.g., in Android Studio I can just press ⌘+O and it will show me the list of all the overridable methods:
You're right, I was mislead by my example and failing test from above. It seems there have been a few issues:
- No way to see a list of overridable methods (this issue)
- Unable to complete the name of an overridable method to insert a stub at the end of a class (this was mentioned above as a possibly blocker for the first issue, and this has been fixed)
- Unable to complete an annotation like
@overrideat the end of a class (I've re-opened https://github.com/Dart-Code/Dart-Code/issues/3182 about this, which I'd previously closed as a dupe of this one)
Welcome to 2024.
Any Updates?
Not really, no.
We're in the process of re-working the implementation of code completion, and I'm hoping that we can address this issue more easily after that's done, but I don't have a firm date for either the completion of that work or the start of working on this issue.
We're in the process of re-working the implementation of code completion
Is there some project or something we can look at to keep ourselves updated with it?
Edit: would it be https://github.com/dart-lang/sdk/projects/24?
Sorry, no. The analyzer team tends to be somewhat inconsistent about using the issue tracker to track work. We generally don't use the issue tracker for this. There are a few exception, such as implementing support for new language features and the example you found (https://github.com/dart-lang/sdk/projects/24).
The project you linked to was, as far as I remember, an effort to fix bugs related to code completion. It was a (but not the only) motivator for the current arc of work in that it made it more visible to us how the old architecture was inhibiting our ability to fix these bugs.
The first step is to restructure the way we generate suggestions, which is the piece that will have the biggest impact on making this issue easier (but not necessarily easy) to fix. That step is well under way, but not yet complete.
Raising this to P2. I'd also be open to raising it to P1 given how much more time consuming this makes the toothbrush task of overriding a method.
I expect we should be able to hit the same quality bar as Java in IntelliJ where as soon as you have typed @Ov you get a completion list that only shows the method overrides that exist in the file (plus a couple other completions for annotations). I think that is principled as those are the only valid lines in Java that you can type that begin with @Ov.
To simplify the problem, if you happen to type @Override in Java and then manually trigger completions you don't get any special handling for Override so we should not feel we need to handle that case either unless it is straightforward to handle. We just need to handle the case while you are have typed part of @override and are trying to quickly pick a method to override.
https://dart-review.googlesource.com/c/sdk/+/356640
I suspect in VS Code, these might get filtered out because they don't include the typed prefix in the filterText. We might need to prefix it with @override or something (I actually wonder if that would also be useful in the display text so it's much easier to identify them?).
I'll do some testing to see.
Yeah, in VS Code this is currently filtered out both on the server and the client. The server we filter it out here:
https://github.com/dart-lang/sdk/blob/c2e9ceeb6b97329f75a84bd41e40b540dd40e7a4/pkg/analysis_server/lib/src/lsp/handlers/handler_completion.dart#L519
And with that removed, VS Code removes it because of the filterText. We'll need to fix both for this to show up. The question is whether we should show "override" somewhere to the user. Here are some possibilities:
1. Don't show "override"
My slight concern here is that it's unclear why some of these things are being suggested - it almost feels like a bug (especially given the highlighting of matches like "r" in "runtimeType").
2. Show "override" at start of completion
This feels noisy, although I think it does make it the clearest what's happening.
3. Show "override" in details (when new labels are supported)
I thought this might be the best of both, but it still has the weird highlighting issue where characters from "override" are highlighted as a match, but most of the characters aren't visible ("runtimeType").
I'm interested in other peoples thoughts.
@DanTup I would prefer option 3, since we already use inline to show imports, so that would make sense.
Also, maybe the documentation could be shown in the override?
@scheglov Could you attach a video please?
I've had trouble with this because it selects the super. statement, instead of the // TODO statement, and I have to manually unselect the super and select and delete the TODO statement.
@DanTup if we can't avoid the weird highlighting thing, I'd personally prefer option 2.
But I have a more general question: how will these suggestions work with existing declaration members? Can we treat a newline between the cursor and an existing declaration as a sign to suggest an overridden member, but otherwise suggest the annotation itself (or at least make it first in the list of suggestions)?