Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customizable IDE click-through target for generated code #56518

Open
goderbauer opened this issue Aug 19, 2024 · 14 comments
Open

Customizable IDE click-through target for generated code #56518

goderbauer opened this issue Aug 19, 2024 · 14 comments
Labels
analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. feature-augmentations Implementation of the augmentations feature P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@goderbauer
Copy link
Contributor

Let's imagine we have been provided with a GenerateFields macro that generates the fields for constructor arguments to cut down on boilerplate. Developers can use it as follows:

/// Doc string about the MyObject.
///
/// It contains lots of facinating details.
/// Wow, this class is very well documented.
@GenerateFields()
class MyObject {
  /// Doc string about the constructor.
  MyObject({
    /// Doc string about the integerValue.
    ///
    /// Did you know that some integers have negative attitude?
    int? integerValue,

    /// Doc string about stringValue.
    ///
    /// Strings are very interesting.
    String? stringValue,
  });

  /// Doc string about this method.
  ///
  /// It does lots of interesting things.
  bool checkCondition() {
    return integerValue == 42 && stringValue == 'forty-two';
  }

  // ... more methods here.
}

The code that the macro generates for use behind the scenes looks like this:

augment class MyObject {
  final prefix0.int? integerValue;
  final prefix0.String? stringValue;
  augment MyObject({ prefix0.int? integerValue, prefix0.String? stringValue, })
      : integerValue = integerValue,
        stringValue = stringValue;
}

Developers now use instances of MyObject in their code, example:

void main() {
  final obj = MyObject(integerValue: 54);
  if (obj.stringValue == 'Hello World') {
    print(obj.checkCondition() ? 'Yes!' : 'No!');
  }
}

As developers write the code snippet above they want to jump to the source of MyObject, e.g. to read the docs or to figure out what other methods MyObject implements. So, they Ctrl+click on "MyObject" (or "stringValue" or "integerValue"). This takes them to the generated code, which is not very useful here and also kinda hard to read. It would have been much more useful to send the developer to the original MyObject class that was authored by the developer itself. That's the code they are familiar with and it contains all the useful information. The generated code is more of an implementation detail, but reading it on a regular basis is not the primary use case for developers that simply use the macro. Which finally brings my to my request:

There should be a way (maybe an annotation) to configure the IDE click-through target for any entity in generated code. In our example, we'd want to annotate the constructor augmentation (augment MyObject(...)) with something that sends click-throughs to the constructor in the original code. Similarly, the fields integerValue and stringValue in the generated code should be annotated with something that sends click-throughs to the corresponding constructor parameter in the original code. The target of the click-through should be configurable by the annotation and the annotations itself would be added by the macro.

Designing the mechanism for this in an open, configurable way independent of macros can also make them useful for other generated code use cases. An example for this are Protocol Buffers (protobufs). When you use protobufs in your dart code and you click through on the name of the protobuf or one of its fields you probably primarily want to go to the .proto file itself (it has all the useful documentation etc.). You probably do not want to go to the generated code that implements the marshaling and unmarshalling of the protobuf in Dart. That logic is sort of an implementation detail.

Last, but not least, an important caveat: While it can be useful to tuck the generated code out of sight and redirect click-throughs to a more appropriate location, the macro-generated code should never be completely hidden. The IDE should make it very clear (visually) that a class/method/thing is being augmented (e.g. with symbols in the margin) and provide an easy way to jump to the generated code. Similarly, stepping through with a debugger should not be affected in any way by these annotations. The proposal here only argues that in some instances the primary use case when clicking through in an IDE is not to go to the generated code, but to go to some other code location that has more context. It assumes that the author of the code generation mechanism (e.g. the macro) knows best what that location should be.

@dart-github-bot
Copy link
Collaborator

Summary: The issue requests a mechanism to configure IDE click-through targets for generated code, allowing developers to navigate to the original source code instead of the generated code. This is particularly useful for macros and code generation tools like Protocol Buffers, where the generated code is an implementation detail and the original source code provides more context.

@dart-github-bot dart-github-bot added area-intellij Tracking issues for the Dart IntelliJ plugin. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-enhancement A request for a change that isn't a bug labels Aug 19, 2024
@goderbauer
Copy link
Contributor Author

cc @jakemac53 @sethladd We discussed this problem in our last sync.

cc @bwilkerson We've talked about this before as well.

cc @rrousselGit I've heard that you had shown interest in something similar as well.

@bwilkerson
Copy link
Member

FWIW, I absolutely hate the idea that a macro author will be required to generate annotations in order for the user to have a good UX. I'm concerned that it will be too easy for a macro author to neglect to do so (for whatever reason) and for the user to end up with a poor experience.

Unfortunately, I don't currently have any brilliant ideas at the moment for how to determine the source of generated code after the fact.

So, for the sake of argument, let's assume that an annotation is the only way to do this. That still leaves a few questions.

What does the annotation denote? I'd like to propose that the annotation should be as general as possible. In particular, I'd like it to denote the element (class, field, method, parameter, etc.) that the generator used in order to generate the code. It shouldn't denote the 'location to which navigation should take the user' because if we have a more semantic-based annotation we can use it for other things, such as generating dartdoc, hover text, etc. (Unfortunately, that won't work well for targets that aren't in Dart code, so we might need a separate mechanism, or variation of the same annotation, in order to support that use case.)

How should the element be represented? I'd like to propose that we use as symbolic a representation as possible. For example, we could use the hierarchy of elements from the root of the library to the element in question. If we use something like the file, line number, and column number then we risk it becoming out of date, especially if it isn't regenerated by a macro after the user has edited the file containing the element.

How much of the generation of the annotation can be automated? I'd hope that the macro author could pass some representation of the element that caused the macro to generate some code to some method that would return a valid form of the annotation. But I am concerned about the usability of this feature outside of macros.

What do we do as a fallback if the annotation isn't generated? (See the previous discussion for at least some of the options.)

@srawlins srawlins added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-server and removed area-intellij Tracking issues for the Dart IntelliJ plugin. labels Aug 19, 2024
@srawlins
Copy link
Member

My 2c: I think it would be good to allow both targets, but through configuration rather than 2 elements in the context menu. I think per-workspace or per-user or per-IDE session configuration would be great. A toggle, something like "jump to generated sources" controls whether you jump to the actual declaration of the thing-you-clicked-on, or whether you jump to the annotation that triggered the generation of the declaration of the thing-you-clicked-on.

Here are the user stories I think this supports:

  • I'm a developer who is not concerned about how generated code works; when I CTRL+click on a constructor for a class that I wrote (but a generator wrote the constructor), it's because I want to go to the class I wrote, and read or make changes.
  • I'm a power user; When I want to know how code works (that I didn't write), I like to read the implementation rather than documentation.
  • I'm a macro author; please please please jump into generated code; I'll die if I'm trying to work on a macro, but have to work hard to find the generated code.
  • I'm maybe close to a power user; I typically like the "jump to the annotation that triggered the generation" setting. However, I think I found a bug in this macro! I'm trying to debug, or poke around generated code, and I want all CTRL+click actions to jump to the real declaration that I'm clicking on, for now.

I would hope that creating a configurable bit is a standard story for both the IntelliJ plugin and the VS Code plugin (maybe all LSP clients?).

@DanTup has thought about this a lot (and many on the analyzer team).

@rrousselGit
Copy link

rrousselGit commented Aug 20, 2024

FWIW, I absolutely hate the idea that a macro author will be required to generate annotations in order for the user to have a good UX. I'm concerned that it will be too easy for a macro author to neglect to do so (for whatever reason) and for the user to end up with a poor experience.

I'm not worried about that. Users will raise an issue if a macro doesn't add the annotation.
It's a really important thing for users, so they'll ask it and avoid a macro if it doesn't do it :)


Personally I'd rather not have a configuration for this.
What I'd prefer is, "go to definition" should always point to the annotated code. Then, folks who wish to read the generated source would use the Go to augmentation codelense.

I assume that 99% of the time, folks wish to see the annotated element.
And those 1% of the times where someone wishes to see the generated code, the generated code would be opened once in their IDE and permanently stay visible.

When I work on generated code, I don't "go to definition" on the generated code. I open the .g.dart from the file explorer, and keep the tab open for the whole session. If I ever need to look for something specific in the generated code, I typically use cmd+shift+o in VScode, which enables searching a symbol by name.

I guess the only exception to the rule would be "Go to definition" when used within the generated code. If within the generated code, we can assume that we want to jump to another place in the generated code.
If we want to jump back to the source, there's yet another codelense for that already anyway.

@rrousselGit
Copy link

rrousselGit commented Aug 20, 2024

On a different note, I think it'd be valuable to consider tying "go to definition" overrides with other features such as "renaming".
Given a generated Foo from an annotated foo, it'd be cool if renaming Foo() could rename foo too.

The use case is: It is common for users to rename identifiers in the place where they are used.
At the moment to rename generated elements, an extra "go to definition" is needed. To rename Foo(), we have to jump to foo and rename it.

This has some downsides:

  • it takes some extra clicks
  • renaming foo won't rename the various Foo() in the codebase.

Given that this likely would involve an annotation too, tying renaming and definition overrides seem to make sense to me.

@lrhn lrhn added feature-augmentations Implementation of the augmentations feature and removed triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. labels Aug 20, 2024
@DanTup
Copy link
Collaborator

DanTup commented Aug 20, 2024

Personally, I would prefer us to never "redirect" Go-to-Definition. I think if there are additional locations to return, we should return all of them, and just put the one we think is most likely what the user wants at the top (or control this with a preference - see below).

For LSP, requests like Go-to-Definition can return multiple results and the client can choose how to handle it. In VS Code, the user can set whether to jump to the first result, show the list, or both:

image

For some cases (like Go-to-Def on a class name that has an original declaration and augmentations) I think we can already get the additional results, but I'm also unsure what it would look like to create this "link" between other declarations. Some kind of "see also" or "generated by" annotation that points to some kind of encoded element reference? Perhaps a macro API that accepts an Element(?) that can emit the annotation entirely? (or a special dart-doc reference?)

writeGeneratedByAnnotation(parameterElement);

// ...

@GeneratedBy('ClassName.ConstructorName.parameterName') // ?
String myVariable;

While poking around in VS Code, I came across these settings that TypeScript/JavaScript have:

image

One common complaint in JS/TS is that when you Go-to-Def you often end up in the typescript definition files (.d.ts) and not the implementation. So they added new commands "Go to Source Definintion" to jump to the actual source code, and then these tickboxes to set that Go-to-Def should actually use that (although they didn't make it default).

Perhaps doing something similar to control the order of the list (eg. the GeneratedBy result goes at the top or bottom of the results), without just removing the other results from the list would make sense (this doesn't make it awkward for those that want to see both because they frequently jump to both/either)?

@rrousselGit

Personally I'd rather not have a configuration for this.
What I'd prefer is, "go to definition" should always point to the annotated code. Then, folks who wish to read the generated source would use the Go to augmentation codelense.

I don't think "Go to Augmentation" is equivalent - it only lets you move between a declaration and its augmentations, so it wouldn't let you get from the original constructor parameter (in the example above) to a class field that was generated for it. We could ofcourse add additional CodeLenses, although navigating from a CodeLens is client-specific so these (like the existing augmentation codeLenses) would be VS Code-specific unless other clients implemented some specific custom command handling.

I do think that whatever we do, if we change the behaviour of Go-to-Definition to do something other than go to the definition, there should be a way to go back to the strict definition of definition, and that it should be available to generic LSP clients.

@rrousselGit
Copy link

Personally, I would prefer us to never "redirect" Go-to-Definition. I think if there are additional locations to return, we should return all of them, and just put the one we think is most likely what the user wants at the top (or control this with a preference - see below).

I've already tested that flow by using an analyzer_plugin and contributing custom definitions myself.
IMO that was a horrible experience. It's effectively faster to use Go to definition followed by clicking on the Go to source codelense than to have to mentally decide which option you want to pick from 2+ choices.

Reading code isn't an easy task. And using that "peek" window forces us to read code to pick where we want to go. That's painful.

I don't think "Go to Augmentation" is equivalent - it only lets you move between a declaration and its augmentations

Correct. But I'm arguing that I very rarely need to jump to a specific parameter. Because as I explained, when I work on generated files, I typically have the generated file constantly opened.

I only need a way to open the generated file once at the top ; which either the file explorer or Go to augmentation cover already.

@DanTup
Copy link
Collaborator

DanTup commented Aug 20, 2024

I've already tested that flow by using an analyzer_plugin and contributing custom definitions myself.
IMO that was a horrible experience.

I'm not sure I follow. I posted a setting above that lets you change it to automatically jump to the first option (and not show any peek window) if that's what you prefer - but it retains the ability to see the peek list if you want. It supports both, whereas taking the other options out of the results does not.

Correct. But I'm arguing that I very rarely need to jump to a specific parameter. Because as I explained, when I work on generated files, I typically have the generated file constantly opened.

I'm also a little lost here. My response was to the idea that Go-to-Def should only go to the original annotated source because those wanting to see the generated source can use the CodeLens - but the CodeLens doesn't provide that ability. While it may be possible to manually navigate there, I think many users would be surprised if they had a reference to a generated field/method in front of them and we said there was no way to jump to the actual generated definition of that field/method and that it required some manual navigation (assuming I correctly understood what you meant).

One of the neat things about Flutter is that it's all Dart code. I can just jump into things to see how they work - it's a great way to learn. I feel like the same might be true of macros - people using them might want to jump into the generated code and see how the machine works. I don't think we want to make this hard to do (or move it away from where users might expect to be able to do it).

@rrousselGit
Copy link

I'm not sure I follow. I posted a setting above that lets you change it to automatically jump to the first option (and not show any peek window) if that's what you prefer - but it retains the ability to see the peek list if you want. It supports both, whereas taking the other options out of the results does not.

Are you saying that this option would allow jumping to the annotated code by default. And folks could opt out by enabling the "peek" view, so that they'd see the generated code listed too?
If so, I misunderstood. I'm not sure how this would play out, but it could be interesting.

My response was to the idea that Go-to-Def should only go to the original annotated source because those wanting to see the generated source can use the CodeLens - but the CodeLens doesn't provide that ability.

Doesn't the code lense redirect you to the corresponding augmentation in the generated code?

There are some corners where the code lense doesn't show. And we could have the proposed annotation help for those cases. But AFAIK we can jump to the code associated to an annotated class.

@DanTup
Copy link
Collaborator

DanTup commented Aug 20, 2024

Are you saying that this option would allow jumping to the annotated code by default. And folks could opt out by enabling the "peek" view, so that they'd see the generated code listed too?

I wasn't particularly suggesting a default, but I'm suggesting that the server should return all of the results so that (whatever we the default), a user can use that setting above to choose between the three options shown (jump to first, jump to first and show peek list, show peek list). We might additionally want a custom checkbox preference (like the TS/JS one) to let the user specify whether that order is reversed (so they could choose whether "always just go to the first result" means to go the annotated code or the "real" definition).

This gives the most flexibility and it uses an existing VS Code setting users may be familiar with to control this kind of behaviour (and also possibly a custom setting that is fairly close to a custom TS/JS setting they also may be familiar with).

Doesn't the code lense redirect you to the corresponding augmentation in the generated code?

The CodeLenses jump along an augmentation chain. If you're at a class A it will only navigate you to an augmentation of class A (and from there, you can navigate back to class A or the next augmentation of class A). It never goes between different kinds of elements, so:

But AFAIK we can jump to the code associated to an annotated class.

is not quite right. If you put an annotation on a class and it generates a field, the augmentation CodeLenses won't let you move between them. It's "Go to the augmentation of this declaration" and not "Go to the augmentation produced by this macro annotation".

So in the example above, you can not get from the constructor parameter integerValue to the generated field integerValue (or vice-versa) because they neither is an augmentation (or augmented) of the other. There's nothing to stop us adding new CodeLenses, though it comes with the caveat above - it wouldn't be standard LSP (this isn't a reason not to use non-standards to make the VS Code behaviour better, but we shouldn't rely on it for anything too common/important).

@goderbauer
Copy link
Contributor Author

FWIW, I absolutely hate the idea that a macro author will be required to generate annotations in order for the user to have a good UX. I'm concerned that it will be too easy for a macro author to neglect to do so (for whatever reason) and for the user to end up with a poor experience.

I am with you. If there are better ideas for achieving this behavior I'd love to hear them. For me, these annotations will be similar to good documentation. When you write a package nothing is actually forcing you to write docs. Your users will have a bad experience if you don't document anything, though. So, we have lints that remind you to document anything that's public (and even those lints can't enforce that the documentation is actually useful). It's the same with these annotations: You don't have to add them, but will pay with a bad user experience if you don't. Maybe we can also have lints here that remind you to add them to generated code?

I'd hope that the macro author could pass some representation of the element that caused the macro to generate some code to some method that would return a valid form of the annotation.

Love this idea. It would be great if you could pass some representation of the element that the click-through should go to to the macro API and it "just does the right thing" with it so I as a developer wouldn't have to figure out how to format the annotation.

Regarding the various comments about configuration: I do believe the option to configure this should be in the hands of the macro author and not the macro user (there can of course be a user setting to override this). The macro author is the only one that knows about the generated code and how it connects back to the original code. If we were to use macros liberally in flutter for widgets and a beginner would end up in the generated code after clicking through on a widget instantiation we'd lose them. At that point they don't know that there is better code for them to look at and they most likely are not going to hunt through their IDE settings to reconfigure this behavior.

And to reiterate: I do not want to hide the generated code. There should be easy and obvious ways to jump between the original code and the generated code. However, jumping to the generated code from an instantiation or method call is often not the primary use case. I'd like to optimize that primary use case and make sure it is intuitive and takes you to the most actionable place.

@rrousselGit
Copy link

With regards to have adding more definitions, then changing VScode's "multiple definitions" to "Go to" instead of "Peek":

I've tested making another analyzer_plugin and contribute my own definitions.
The setting looks promising. Unfortunately custom definitions end-up last in the list. I guess we lack a priority field when NavigationContributors add definitions.

@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Aug 23, 2024
@goderbauer
Copy link
Contributor Author

Related discussion about IDE navigation for augmented code: #54742

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. feature-augmentations Implementation of the augmentations feature P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants