Ctrl+Click on constructor declarations does not find references, but the same on method declarations does

[DanTup]

void main() {
  A.named().foo();
  A.named().foo();
}

class A {
  A.named() {} // Ctrl+Click on named does nothing
  void foo() {} // Ctrl+Click on foo finds the references above
}

It seems like VS Code may be sending requests for both definition+references in the case that works, but only definition in the case that doesn't (perhaps the response to definition being empty is suppressing the later?).

[DanTup]

@bwilkerson @scheglov this is (mostly) caused by a bug in the LSP handler, but while fixing that I noticed some slightly odd things with the navigation regions/targets that are also affecting its behaviour. For the above to work as expected, when invoking Go-to-Definition (which uses the navigation data) on a declaration itself, the result should span the position where it was invoked (eg. if you invoke Go-to-Definition on a method name in its declaration, the returned target should cover the method name).

However, in the following code, there's a navigation region for A.named with a target of just named. This means Go-to-Definition on the A will "jump" to named which seems a little unhelpful (it neither jumps to the class, nor includes its own position in the result, which would cause VS Code to trigger Find References).

class A {
  A();
  A.named(); // invoke on "A" here
}

I think there are two ways we could improve this:

• The A in A.named navigates to the class declaration (eg., the class name in a named constructor is always considered a reference to the type, not part of the constructor name - this should ofc only be the case for named constructors)
• The region A.named points to a target that is also the whole of A.named (not only named).

I don't know that one is obviously better than the other (the first makes named constructors more like static methods, and the second treats the full class+constructorName like a single unit) - both seem to have their merits.

There are some tests that are explicitly checking the current behaviour, for example:

assertHasRegionString('A.named();', 'A'.length);
assertHasTarget('named() {}');

But I'm not sure if it was a specific goal to exclude the typename, so was after your opinion(s) before changing.

[scheglov]

My opinion:

1. new A.named(): from A to class A, from named to named in A.named declaration.
2. new A(): from A to the unnamed A() constructor declaration.
3. A.named() declaration: from A to class A, from named - nothing.
4. A() declaration: form A to class A.

I think navigation from a declaration name to the declaration name is useless, and exists as only an artifact of the way navigation is implemented - go down to SimpleIdentifier and then blindly get its element, sometimes look for special cases, and build navigation. First of all, the declaration should not use SimpleIdentifier as its "name". It is not an expression, there is no meaning for its type and element. The declaration might declare an element, which might be used to construct a type. But the name - no. And navigation is useless - we already there.

[DanTup]

My opinion:

Thanks! I think this way is my preference too, so I'll go that way and update the existing tests.

I think navigation from a declaration name to the declaration name is useless, and exists as only an artifact of the way navigation is implemented

VS Code is actually using this as a signal to support the functionality described above. If it asks for a Definition and the result is the same location where it was invoked, it will immediately perform Find References instead:

So if there's reason to remove it for non-LSP, we should probably either have a flag to include it, or handle it in the LSP handler.

[bwilkerson]

3. A.named() declaration: from A to class A, from named - nothing.
4. A() declaration: form A to class A.

I don't have a strong objection, but it doesn't seem useful to me to be able to navigate from the declaration of a constructor to the top of the class in which the constructor is defined.

I think I'd rather have Find References work in those locations.

So if there's reason to remove it for non-LSP, we should probably either have a flag to include it, or handle it in the LSP handler.

I might be wrong, but I don't think we have enough information in the LSP handler. I'm fine with just changing the behavior everywhere and see whether it causes a problem for our IntelliJ / AS users.

[DanTup]

I don't have a strong objection, but it doesn't seem useful to me to be able to navigate from the declaration of a constructor to the top of the class in which the constructor is defined.

Yeah, I'm not sure it's very useful, but the constructor name can still be used for references. It feels more predictable if the declaration + call sites work the same (and if we have call sites also treat the class name as part of the constructor, that makes them inconsistent with static methods - which are often used in place of constructors).

I think having a rule of "Class name is always a reference to the class, unless it's obvious the default constructor" is simpler than having additional exceptions.

I might be wrong, but I don't think we have enough information in the LSP handler

I guess I meant if the navigator returned no results, we could just look at the node at the offset ourselves and see if it's a declaration (or use some other API that may exist). I didn't think long about it though, as it didn't sound like there was a compelling reason to change this right now (and if we do in future, the new tests I'm adding for LSP will make it clear we need to add something for it).

[scheglov]

Well, I have to correct myself. Navigation from A() declaration to class A would be inconsistent with searching constructor references. We don't want to search class references instead, and forcing users to write A.new() declaration is not an option :-)

So, maybe no navigation from A() at all.

[DanTup]

Oh, I realise I somehow misread your point (4) above (which is - at least partly - what Brian was responding to). I had interpreted it as A in a constructor declaration would only navigate to the class if it was a named constructor. For an unnamed constructor, the A() would be treated as if it was the name (this is consistent with Go-to-Definition on constructor calls, where A() goes to the constructor, but A in A.named() goes to the class).

Eg:

class A<T> {
     A();
//   ^         definition is self
     A.named();
//   ^         definition is `A` in `class A`
//     ^^^^^   definition is self 
}

[DanTup]

I've opened https://dart-review.googlesource.com/c/sdk/+/248581/ with the changes described above (treating the class name as a reference to the class unless it is an unnamed constructor, where it is a reference to the constructor).

If any of the changes in the existing tests don't match what you expected/preferred from the above, please let me know!

[scheglov]

So if there's reason to remove it for non-LSP, we should probably either have a flag to include it, or handle it in the LSP handler.

I might be wrong, but I don't think we have enough information in the LSP handler. I'm fine with just changing the behavior everywhere and see whether it causes a problem for our IntelliJ / AS users.

It seems that IntelliJ also sends search.findElementReferences when I ask to "navigate" from a declaration name. So, apparently users like it.

[DanTup]

Ah nice. I guess it probably makes sense leaving it as-is this way in the navigation computer then :)

Fix has landed in dart-lang/sdk@a2f9ad4.