Augmentation-related navigation

[bwilkerson]

There are some interesting design questions related to library augmentations (and hence macros) and navigation in IDEs. I've included them all in a single issue because I think it would be good to think about them holistically, at least initially. If there are additional questions, please add them to the list.

While the limitations of the major supported platforms will need to be considered, I'm more interested initially in identifying the UX that will be best for users. If we have to compromise because of those limitations then at least we'll know what the ideal is.

1. There are some navigation paths that we probably want to support that are new to the library augmentations feature. It isn't clear that the existing capabilities of IDE's are sufficient to the task, so the question is: how should we support them. The new navigations I can think of are (please add to this list if it's incomplete):
   • from an augmentation to the augmented declaration
   • from an augmentation to other augmentations
   • from an augmented declaration to its augmentations

Note that any augmented declaration might have multiple augmentations. There is a well-defined "merge order" that we could use to linearize the augmentations, but it isn't clear whether that's the right UX or whether it would be better to be able to navigate from any augmentation to any other augmentation in a single click.

2. There are some existing navigations paths that might also be impacted. For example, from an invocation of an augmented method m, do we want 'Go to declaration' to navigate to the augmented method, to one of the augmentations, or to present multiple targets? The potentially impacted existing navigations I can think of are (please add to this list if it's incomplete):
   • go to declaration
   • go to definition
   • go to overridden
   • go to override
   • go to superclass
   • go to subclasses

[bwilkerson]

@jwren @DanTup

[bwilkerson]

@jakemac53 @munificent

[jakemac53]

As far as go to definition (and related features), I think it should by default go to the augmented declaration (the original one, without an augment keyword). Even if it has no implementation, this is likely the one that contains the most relevant doc comments, etc. It is the place the API was defined. From there, we can rely on the solution to the first item (showing augmentations of that augmented declaration).

[jakemac53]

Related to the first item, I think it would be ideal if you could tell that a declaration is being augmented at a glance, without requiring any hovering over the declaration to see the list of augmentations. I don't have a specific UX proposal here, I just want something to indicate that it is augmented.

[bwilkerson]

There are at least two ways in LSP that I can think of to show this information: semantic highlighting and inlay hints. I'm sure there are others. The other is currently supported in IntelliJ, but we might be able to add the second.

[DanTup]

As far as go to definition (and related features), I think it should by default go to the augmented declaration (the original one, without an augment keyword)

I think we should probably return both (or all) results here rather than just one, so that the user can see them all and easily jump between them (it would behave like "Go to References" when that shows multiple results).

I think it would be ideal if you could tell that a declaration is being augmented at a glance, without requiring any hovering over the declaration to see the list of augmentations.

There are at least two ways in LSP that I can think of to show this information: semantic highlighting and inlay hints. I'm sure there are others.

We could add a semantic token modifier, but I think the only thing we could do with that in the client is change the colour (and only in limited ways because of custom themes).. I don't know if that would be an obvious indicator (it also doesn't wouldn't give a way to navigate to them).

Inlay Hints are also quite limited right now (the standard ones are assumed to either by Type or Parameter names) and we couldn't style them very well. VS Code does have "decorations" which are very similar, but not standard LSP - that's what we use for Closing Labels which gives us some more flexibility (at the expense or requiring custom messages and code in the client).

Something that comes up now and then is using CodeLens to show references to symbols:

I haven't tried to implement this yet because it feels expensive (we have to tell the client where these code lenses will appear for an entire document in one go, and that either means potentially having lots of 0-reference CodeLens, or I think doing a lot of slow searching).

But maybe if we know how many augmentations there are quickly, we could show them in this way? We'd probably still need a small amount of custom client code (because we'd want clicking them to do a search, and LSP doesn't define a way to trigger something like definitions), but that should be fairly minimal.

[bwilkerson]

But maybe if we know how many augmentations there are quickly ...

Yes. We have that information in the element model directly without any searching, so that sounds like a viable option worth considering.

@parlough

[DanTup]

Something I just remembered is that LSP has both a "definition" and a "declaration" request. We don't currently implement declaration, but perhaps we should implement it now (going to the original class definition without any augment keyword).

This would allow definition to include all the augmentations and the user can either jump directly to the original class, or trigger the peek window with all of the definitions.

[bwilkerson]

We had an in-office discussion yesterday and had come to the conclusion that "go to definition" should, at least for invocations, take the user to the last augmentation in the merge list. If other augmentations in the chain, or the un-augmented function, are invoked (via augmented()), then the user can easily navigate to them that way. If they aren't invoked, then it's less clear that it's useful for users to be able to navigate to them.

We didn't discusses class-like declarations.

But I agree that the ability to implement "go to declaration" might open some interesting possibilities.

@jwren Does IntelliJ have a similar distinction between "definition" and "declaration"?

[DanTup]

We had an in-office discussion yesterday and had come to the conclusion that "go to definition" should, at least for invocations, take the user to the last augmentation in the merge list.

For LSP, I think returning only this option might not be the best choice. We can return multiple results and (in VS Code, at least), the user can set whether they're prefer to jump to the "primary" result or see a peek list of all (or both):

If we return them all, the user can choose which behaviour they want - but if we only provide one, they cannot choose to see them all.

[bwilkerson]

Good to know. I don't think any of us were aware of that setting.

The argument to make the choice available to the user is a valid one.

If the user chooses 'goto', can we control which one of the multiple options will be selected? I'd expect it to either be the first or the last, but the LSP spec doesn't describe the effect of the user setting on the UX. Ideally I'd like to have them ordered based on the "merge" order, because I believe that the order matters to users.

In our discussion, the primary argument was an assumption that most users wouldn't want to have to choose, and would be more interested in seeing the code that will actually be executed. This seems especially compelling given that some of the augmentations might never be invoked. It's hard to see why a user, when looking at an invocation site, would want to see anything other than the code being invoked, especially given that the entire method signature must be consistent and that some of the potential augmentations shown by 'peek' might be unreachable. Is there a way that we could mark the unreachable augmentations in the peek window? I don't see anything in the protocol, but that doesn't mean it doesn't exist.

If we think that users might want to see all of the augmentations, is there any way, other than the location of the augmentation, for us to distinguish them in the UI?

Of course, the argument only applies to function augmentations. Augmentations of class-like declarations are all always applied, and the "header" can be different in every augmentation. For that case it does seem likely that a user would want to be able to navigate to every augmentation, though the question of how to clearly distinguish the possible destinations is still valid.

[DanTup]

If the user chooses 'goto', can we control which one of the multiple options will be selected? I'd expect it to either be the first or the last, but the LSP spec doesn't describe the effect of the user setting on the UX.

LSP doesn't define options like this so this setting is VS Code-specific (other editors could choose to do similar, or not). VS Code also doesn't seem to specify it in the API docs, but the code shows that it uses the first:

https://github.com/microsoft/vscode/blob/0cae183880d27078528fd50fffd4e29b5de4a82c/src/vs/editor/contrib/gotoSymbol/browser/goToCommands.ts#L184

We could presumably sort the list in either direction to achieve what you want, although I think the UI groups by file, so if they can span multiple files (I'm not sure if they can?), that order might not be obvious.

In our discussion, the primary argument was an assumption that most users wouldn't want to have to choose, and would be more interested in seeing the code that will actually be executed.

That sounds reasonable to me. When I wrote some of the above I think I was just thinking about classes (and I think I'd prefer the declaration first, and then the augmentations). I agree for functions you'd probably want to start at the most specific and can jump into augmented() calls like you would super.().

Is there a way that we could mark the unreachable augmentations in the peek window?

I don't think so, but I'm also unsure what you mean that some might be unreachable. Do you mean if there's no call to augmented() (in which case nothing from that point up would be called?)

If we think that users might want to see all of the augmentations, is there any way, other than the location of the augmentation, for us to distinguish them in the UI?

Unfortunately I don't think so, at least not for Definition. There are of course other potential features for navigation though (like CodeLens, hierarchy, Go-to-Super etc.).

[bwilkerson]

... I think the UI groups by file, so if they can span multiple files (I'm not sure if they can?), that order might not be obvious.

Yes, the augmentations can span multiple files, at least for user-defined augmentations. For example:

test.dart

import augment 'a.dart';
import augment 'b.dart';

void f() {}

a.dart

library augment 'test.dart';

augment void f() {
  print('from a');
}

b.dart

library augment 'test.dart';

augment void f() {
  print('from b');
}

Do you mean if there's no call to augmented() (in which case nothing from that point up would be called?)

Yes, just like the augmentation in b.dart. Neither the augmentation in a.dart nor the base function in test.dart will ever be invoked.

[DanTup]

I don't know if I agree with what I wrote above now.. For classes, showing all of the agumentation classes makes sense to me, but if we're talking about invocations of methods/functions I'm less certain.

If they're essentially the same as overridden methods with super() calls (I think that's the case, but I'm not certain), then I think I'd probably expect the same behaviour (as you described above) - jump to the one that will actually be executed and then jump up the tree using the augmented() calls (or our "Go to Super" command or a similar new command)?

(and I think we should implement "Go to Declaration" to get straight to the declaration if that might be where the docs etc. are but Definition is not going there)

[bwilkerson]

If they're essentially the same as overridden methods with super() calls ...

They're mostly the same, and close enough that I think users will expect the same behavior.

The difference is that with overrides we can't know which method will be executed, only the method that would be executed if lookup started in the class corresponding to the static type. With augmentations we know exactly which method will be executed (assuming there are no overrides of the base method).

and I think we should implement "Go to Declaration" to get straight to the declaration if that might be where the docs etc. are but Definition is not going there

That seems reasonable, but

• It hasn't yet beed decided whether we're going to support augmenting docs in augmentations, so it's possible that the docs might be spread out all over the place. I'm not sure whether there's a valid use case for supporting it, and I think it would be awfully hard to author docs that are distributed that way, so I'm kind of hoping we don't. @srawlins is there an issue for this discussion?
• I'd probably remove the "but" and just say that Declaration should go to the same place as Definition when there's only one reasonable place to go. Unless that's counter to what VS Code does for other languages.

[DanTup]

I'd probably remove the "but" and just say that Declaration should go to the same place as Definition when there's only one reasonable place to go. Unless that's counter to what VS Code does for other languages.

It doesn't seem like many languages currently support "Go to Declaration" (I could only find C++ where I think it's going to headers).

It feels like a missing feature to me though - we don't have a quick way to jump to the declaration of a method, we always go to the most overridden version. If you want to get to the base you'd have to keep jumping to the super calls (or using Go-to-Super). Go-to-Declaration seems like a good way to jump right to the top of the tree (in a standard way).

(note: Go to Declaration is a separate command with no keybindings, so it wouldn't affect F12/Ctrl+click, it would just be an extra option on the context menu/command palette).

[bwilkerson]

I could only find C++ where I think it's going to headers

When I first saw that both were supported, C++ was what immediately came to mind as a language where the declaration and definition can be separate.

... we don't have a quick way to jump to the declaration of a method, we always go to the most overridden version.

I'm not sure whether that distinction really makes a lot of sense in Dart. Let me give a couple of examples for why I have to question the value.

First, it's possible for the method found by starting lookup at the static type to have a different signature than the base method (the method that does not override any other). Consider

class A {
  void m(String a) {}
}

class B extends A {
  @override
  void m(String a, [int b = 0]) {}
}

It isn't clear to me that in this case that it's useful for the user to be able to navigate to A.m, because that method's signature is incomplete, and the doc comment likely is too.

Second, there isn't necessarily a single base method. Consider

class A {
  void m(String a, {int b = 0}) {}
}

abstract class I {
  void m(String a, {int c = 0}) {}
}

class B extends A implements I {
  @override
  void m(String a, {int b = 0, int c = 0}) {}
}

Would it be more appropriate to navigate to A.m or I.m as the base method? Or to provide both? Even if we said "both" we'd still have the first problem to deal with.

I'm not saying that we shouldn't implement something for Go To Declaration, only that the choice for how to implement it doesn't seem as straightforward to me as the design of Go To Definition.

[DanTup]

You're right, it is less clear than I thought :)

We can indeed return multiple results for Go-to-Declaration too, but you're right that it doesn't help answer all of the questions.

[srawlins]

This is a P1 issue, so it needs an assignee and weekly progress. @bwilkerson should that assignee be you?

[DanTup]

I had a little dig into the VS Code APIs for "Peek Location" to see how they'd work triggered from CodeLens. Unfortunately this isn't standard LSP so would require some custom code in Dart-Code.

Here I added an "x augmentations" link above declarations with augmentations (though only those that are not themselves augmentations) which when clicked will show the locations of all augmentations. I think it works reasonable well, but it's unfortunate that the list of results on the right is basically the same for everything (unfortunately we don't get to choose what appears here, we're just providing a list of locations and VS Code is extracting the relevant line).

I also added CodeLens for "Augments x" on augmentations and "Augmented by x" for things with augmentations that jump between them directly. I don't know if there's a better way to word this and I'm not sure if taking up this extra space with CodeLens is worth it in that case (for example for Super we just have a "Go to Super" command that's on the context menu... I wonder if we should just have "Go to Augmented" and "Go to Augmentation" commands.. although I'll note that we can't make these conditional, so they'd always appear in the context menu, just like Go to Super does).

https://github.com/dart-lang/sdk/assets/1078012/540e6800-bb3d-4ee3-afac-99b447bcb48d

[bwilkerson]

Thanks for looking into this.

... it's unfortunate that the list of results on the right is basically the same for everything ...

That is unfortunate. It makes it really hard to know which one to select. I'm not sure whether it actually adds value for the user. Kind of seems like we'd be better off with just the 'Go to annotated' and 'Go to annotation' style navigation.

... I wonder if we should just have "Go to Augmented" and "Go to Augmentation" commands.. although I'll note that we can't make these conditional, so they'd always appear in the context menu, just like Go to Super does).

That's also unfortunate. It wouldn't be so bad if we could disable the menu item when it doesn't apply, but I'm guessing that we can't.

I do think it would be good for us to treat these two forms of implementation-chain navigation in a similar way. The concepts are similar enough that it seems like having a similar UI would be helpful. (And we might consider having similar support for redeclarations in extension types, though it isn't clear how often users will use extension types so it might not be worth adding.)

[DanTup]

That's also unfortunate. It wouldn't be so bad if we could disable the menu item when it doesn't apply, but I'm guessing that we can't.

This is unfortunately correct. For the context menu we have to declare all the items up-front, and we have no context of where it will be invoked (because showing the context menu is synchronous in the VS code UI and it will never wait on async calls to the extension host for showing a menu).

For some kinds of things we can set conditions based on something like the cursor (caret?) location within the editor, however this doesn't work for context menus because being async we'd always be "late" and the user would see menu items enabled based on where the caret was before they right-clicked.

[DanTup]

I hacked up an example of using CodeLens for various kinds of navigation like this (Super, Overrides, Augmented, Augmentation):

I realised that Super/Overrides and Augmented/Augmentations are not quite as similar as I initially imagined. Whereas the augmentation chain is a one-to-one relationship, overrides are not (that is, there is only ever 0 or 1 direct augmentations of a member, but because multiple classes can extend a class, there can be multiple overrides).

That means if we did something like the screenshot above, "Overrides" would potentially trigger a search that uses a peek window (as shown above), whereas "Augmentation" would always just jump to the next item in the chain.

I don't think that difference is a reason not to make them otherwise consistent though.

Some questions I had while looking at this:

1. The distinction between "augmented" and "augmentation" is obvious enough
2. Where we should show "overrides" for an augmented member.. I guess technically it's the last augmentation in the chain that's being overridden, but probably you'd expect to be able to find overrides from the base member. Should overrides be shown on all augmentations and the original declaration?
3. Where should "Go to Super" go if the super method is augmented?
4. Can we compute the necessary information to show these CodeLens efficiently (we need to provide the CodeLens text for an entire document in one go, and frequently as the user modifies the file)

[bwilkerson]

These are all good questions.

Whereas the augmentation chain is a one-to-one relationship, overrides or not ...

Technically, neither are 'Super' (or 'Overridden'), at least not in my mental model. I tend to think of concrete members in classes as overriding abstract members in either a superclass or an interface, and they can override members from multiple interfaces. I'm not sure what we currently use as the target for 'Go To Super'; maybe it only ever goes to an overridden member from a superclass.

2. Where we should show "overrides" for an augmented member.. I guess technically it's the last augmentation in the chain that's being overridden, but probably you'd expect to be able to find overrides from the base member. Should overrides be shown on all augmentations and the original declaration?

I don't know that I have any concrete answer, but my view of this is likely to be influenced by my mental model. I tend to think of the two chains graphically as

C.m
 |
D.m - D'.m - D''.m
 |
E.m

That is, the augmentation chain is effectively a single member in the override chain.

As opposed to

C.m
 |
D.m - D'.m - D''.m
                |
               E.m

where the override attaches to the last element in the augmentation chain.

Of course, the real question here is what will users want. This probably depends in part on what's convenient, but also on the mental model that we're going to encourage users to have (via documentation, discussions, etc.) @MaryaBelanger

3. Where should "Go to Super" go if the super method is augmented?

Should "Go to Super" go to the same place as navigating from the keyword super? Would it be too confusing if they didn't, or would it just make it easier for users to navigate to where they want to go?

I usually use navigation from super to see what code will be invoked at that point in the execution, so I think I'd prefer for navigation from super to take me to the last augmentation of the method, if any other code in these chains will be executed it will be through an invocation of either augmented or super.

But "Go To Super" might be different, it might want to walk up to the head of the augmentation chain in the superclass (which could very well be in a library augmentation).

4. Can we compute the necessary information to show these CodeLens efficiently (we need to provide the CodeLens text for an entire document in one go, and frequently as the user modifies the file)

I think the answer is 'yes'. I think we have enough information today to make that efficient, but if not I'm reasonably confident that we're already computing all the necessary information and just need to capture it.

[DanTup]

Should "Go to Super" go to the same place as navigating from the keyword super? Would it be too confusing if they didn't, or would it just make it easier for users to navigate to where they want to go?

My intention when I added this was to go to the same place as super() would (I think it always does this, but I'm not certain there aren't cases where it might not). As such, I would expect it to go to the last (first?) member in the augmentation chain (it probably does not currently though, because probably the code that handles it is unaware of augmentations).

I don't know whether my idea is what most users would expect though - this command hasn't had a lot of use/feedback because until recently (when it was added to the context menu) it wasn't particularly easy to discover.

If we're not sure, we could ofc offer both options - but since they'd always be on the context menu everywhere, we'd need to make sure the labels are clear enough that users understand why they both exist (I suspect in the overwhelming majority of cases they will be the same target).

My comment about augmentations and overrides being different (which I typo'd a bit) was that a method can have multiple overrides (eg. A.foo() might have an override in B.foo() and C.foo()), but the augmentation chain is flat - a member can only have one augmentation (although it could then have another augmentation, etc.).

So if we had both overrides and augmentations as CodeLenses, it might be that one always jumps directly to a target (augmentation) whereas the other may need to show a set of results.

[bwilkerson]

... I would expect ['Super'] to go to the last (first?) member in the augmentation chain ...

(I'd probably say 'last'; it seems more consistent with the notion of a merge order because the augmentation that will be invoked is always the last augmentation in the merge order.)

I think I agree with that. It's worth noting that super and augmented are different in an augmentation, so under this definition 'Super' would always jump to the superclass' implementation of the member. I think that's reasonable; it's certainly consistent, which is good.

[DanTup]

How does the following sound:

1. We ensure the "Go to Super" command goes to the place that a super() call would invoke
   • Probably this is currently not going to the end of the augmentation chain of the super class, but it should
2. We add a new "Go to Override(s)" command that shows a peek window for all overrides (or if only one, navigates directly)
3. We add new commands for "Go to Augmented" and "Go to Augmentation" that will move back and forth along the augmentation chain
   • Unsure about whether these belong on the context menu alongside "Go to Super".. for consistency/discoverability maybe they should, but CodeLenses can appear conditionally so maybe we're better just leaving them for that
4. We add CodeLenses for:
   1. Super - just invokes "Go to Super" command
   2. Overrides - just invokes "Go to Override(s)" command
   3. Augmented - just invokes "Go to Augmented" command
   4. Augmentation - just invokes "Go to Augmentation" command
   5. References(?) - this has been requested before but never implemented
      • Each of these should be controllable by their own setting so users can choose which they care about (eg. "dart.codeLens.foo": true or "dart.codeLens": { "foo": true }).

Maybe for classes things should be a little different though. For example "Overrides" would probably be replaced by "Subclasses"? Should we also have an "Augmentations" on the original class declaration, or just Augmented/Augmentation so you move along the chain? I feel like both could be useful but having three CodeLenses with very similar text doesn't feel great).

I guess as a start, I can ensure Go to Super does the same as super() for augmentations, and add Go to Augmented/Go to Augmentation commands that can work from the palette (which we can then expand to context menu/CodeLens after).

[bwilkerson]

I like the idea of implementing them as commands available from the command palette first. That would allow us to experiment a bit to get the UX right, though I do think we're close to knowing what we want. I also like that code lenses are configurable.

1. I agree that "Go to Super" and navigating from super should have the same behavior, and that they should go to the last augmentation when there is one. Also, obviously, it should only be available as a CodeLens on instance members.

2. I agree with "Go to Override(s)". I assume that it will also navigate to the last augmentation if there is one in a given subclass.

3. Agreed. Again, presumably only available when there is an augmentation. And presumably available for any declaration that can be augmented (methods, classes, top-level functions, etc.).

5-8. Yes.

9. We already have a "Go to References" on the context menu. I assume the code lens would be the same command.

Maybe for classes things should be a little different though. For example "Overrides" would probably be replaced by "Subclasses"?

Yes, I think we should match the standard terminology. We already have a "Go to Implementations" on the context menu. I assume that that we'd use the same command for both "Subclasses" and "Overrides".

Should we also have an "Augmentations" on the original class declaration, or just Augmented/Augmentation so you move along the chain? I feel like both could be useful but having three CodeLenses with very similar text doesn't feel great).

If it's useful to have an "Augmentations" (I assume it would open a peek window when there's more than one), which I'm not sure that it is, then it should be available on every augmented declaration, and maybe on every augmentation as well.

I think I'd wait on implementing this until we have evidence that it's useful, and even then I think we should consider making it available on the command palette without making it a code lens until there's evidence that it's really desired. I'm a little concerned about having too many code lenses and having the list of commands get too long. Maybe it'll be fine, but ...

[srawlins]

As this is a P1 issue, I have to ask, @DanTup , @bwilkerson , any updates? Is this complete with the commits landed 2 weeks ago?

[DanTup]

The changes above support "Go to Augment(ed|ation)" CodeLens. We don't yet have CodeLenses for other things (like Go to Super), and I don't know if we want to do that now or iterate a little on the augmentation CodeLenses first (anyone have opinions on this?).

There are some other things noted above about things like Find References.. I don't know if those are all working as expected for augmentations, so I suspect there might at least be some tests that need adding to verify the behaviour of those where they haven't already.

[bwilkerson]

... I don't know if we want to do that now or iterate a little on the augmentation CodeLenses first ...

I would vote for iterating on the augmentation CodeLenses first. I also wouldn't include work on Go to Super in the work for this particular issue because I see them as being independent from each other.

Find References, on the other hand, does seem to fit under this issue.

[bwilkerson]

Just a status update: Danny has a prototype of CodeLens support for navigating along the augmentation chain. It looks promising (see https://github.com/Dart-Code/Dart-Code/issues/5063).

[pq]

Any fresh updates here? Is this still a P1?

[bwilkerson]

I downgraded the priority. I think it's critical that we have a good design, but it's kind of odd to have a P1 for a design discussion.