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

Go-to-Definition on super() constructor call goes to class when there's no default constructor #55241

Closed
DanTup opened this issue Mar 19, 2024 · 3 comments
Labels
analyzer-lsp Issues with analysis server's support of Language Server Protocol area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on

Comments

@DanTup
Copy link
Collaborator

DanTup commented Mar 19, 2024

In this code, I think I would expect Go-to-Definition on the super() constructor call in the C() constructor to jump to the A() constructor.

class A {
  A();
}

class B extends A {}

class C extends B {
  C() : super(); // invoke Go-to-Definition on "super"
}

However, it currently jumps to class ^B.

@bwilkerson any opinion on this? Technically the implied constructor in B is what's being called, but since it's synthetic I feel like we should skip it.

However, I'm not sure there aren't edge cases that might make this seem inconsistent (for example if B didn't have A as a subclass, then probably we would need to jump to class ^B).

@DanTup DanTup added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-lsp Issues with analysis server's support of Language Server Protocol labels Mar 19, 2024
@bwilkerson
Copy link
Member

I almost always have an opinion. :-)

I think it's a bit of a wrinkle, but,

  1. As you noted, it would be inconsistent with the behavior when there's no explicit constructor in any of the superclasses.
  2. The field initializers in B (if there are any) will be executed, and skipping B might obscure that fact, especially for newer Dart users. (Although we don't include field initializers in the normal navigation, so that's a fairly weak argument.)

The worst part, IMO, is that once the user navigates to B there's no way to continue along the navigation chain.

The real question is, which would best fit the user's expectations? I have two data points (yours and mine), but it's hard to generalize from there.

@pq pq added the P2 A bug or feature request we're likely to work on label Mar 19, 2024
@lrhn
Copy link
Member

lrhn commented Mar 19, 2024

From a pedantic point of view, going to B is obviously correct.
The default constructor of the B class is the constructor that super denotes, not the constructor in A.

And as mentioned, if B had initializer fields, then that's where they would be executed.

Now, let's make things interesting.

class A { 
  A();
}
mixin M {}
class C extends A with M {
  C() : super();
}

Now, which constructor does C's super() denote? ....
Answer: The implicitly added forwarding constructor of the anonymous mixin application class of A with M.

Nobody wants to go there. Ever. (If there even is a "there" to go. I don't know where I'd put the cursor. On the M in with M probably.)

The denoted super-constructor existing is not enough reason to navigate to it.

If I instead write the same program, just with the mixin application being named instead of anonymous:

class A { 
  A();
}
mixin M {}
abstract class AM = A with M;
class C extends AM {
  C() : super();
}

then what?

Going to class AM is technically correct. Just as correct as going to B above. The class AM is class, it has an unnamed constructor that you can refer to in subclasses, like C does.

I'd do the same thing here as for B. (And this didn't bring me closer to figuring out what that had to be.)

I am leaning towards the current, pedantically correct, behavior.

(I don't think it's possible to display a synthetic default constructor, like B(): super(); // Synthetic default constructor, because there is no place to show it. Otherwise that would be neat.)

@DanTup
Copy link
Collaborator Author

DanTup commented Mar 20, 2024

abstract class AM = A with M;

TIL you can do this 🤯

(I don't think it's possible to display a synthetic default constructor, like B(): super(); // Synthetic default constructor, because there is no place to show it. Otherwise that would be neat.)

We could potentially do this (it's basically a variation of "Closing Labels"), although I'd really like to move closing labels to something more standard like Inlay Hints - but currently we can't control the styling for that (requires microsoft/vscode#151920).

I am less sure now that this should be changed. I do feel like not being able to continue the chain sucks, but you can always invoke Go to Super to get to the super class and then manually locate the constructor from there. It's not perfect, but it seems like there probably isn't any perfect solution here (besides maybe showing synthetic constructors in the editor, but I'm also not convinced that wouldn't be a little weird and cause things to jump around as you're adding constructors).

I'll close this on the assumption we probably don't want to change this then (we can always re-open if additional opinions change things, or if we think we should try implementing some other mechanism like showing the synthetic constructor, or a way to jump to super constructor from a class that doesn't have an explicit constructor).

@DanTup DanTup closed this as completed Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-lsp Issues with analysis server's support of Language Server Protocol area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

4 participants