New constructors highlighting

[FMorschel]

After Dart-Code/Dart-Code#4454 was closed, we have a different behaviour than we had previously:

/// This is a doc for [MyClass] and [MyClass.new]
class MyClass {
  MyClass(); // Constructor

  MyClass myMethod<T extends MyClass>() {
    return MyClass();
  }
}

IIRC, previously we had implicit new constructors highlighting the class name too.

But that may be hard to solve. Like, when you hover the explicit new, to only highlight the current cases but then when hovering the actual constructor, it would make both the class name and the explicit new to highlight without . (at least the way we have it currently). I'm not sure what to do here, WDYT?

CC @DanTup @bwilkerson

[DanTup]

previously we had implicit new constructors highlighting the class name too

This was deliberately changed according to Dart-Code/Dart-Code#4454 (comment). The idea was to separate something that is "a type name" and "a constructor" rather than having unnamed constructors and type names blurred together.

We could make this asymmetric and have unnamed constructors act as both (so they highlight both groups) like we did for some patterns, but I'm a little on the fence about whether that is obvious (not to mention because of microsoft/vscode#238301 this isn't handled perfectly).

[FMorschel]

The reason I brought this up is because, when walking myself through code, I had a habit of placing the cursor at the class name because (constructors some times are not at the very top and) vscode has that feature to see the current scope:

So I'd click the line and click on the class name to see where it is being used. But now that is not the case anymore and I must find the constructor.

If you disagree in making that asymmetric, I'm fine with it, just though of bringing it up because it changed the way I usually did this.

[DanTup]

But now that is not the case anymore and I must find the constructor.

I may have misunderstood your issue when I responded to the above. I thought you were asking for the constructor to trigger highlighting of both classes + constructors. However by now having to find the constructor, I wonder if you meant you used to use the type name to also find constructor calls?

Thinking more, this does feel a little more odd to me now, if you consider that named constructors would get their type names highlighted but unnamed constructor calls do not:

This may be an argument for unnamed constructors being considered both type names and references to their constructors (like the pattern example). Doing "Find References" on that location would include the unnamed constructor too, so maybe doing so would still be consistent. @bwilkerson I'm interested in your thoughts (and maybe we can try to document the ideas somewhere to make it easier for us to answer questions like this and be consistent).

[FMorschel]

Yeah, I think I was not clear enough, thanks. But to that point, implicit new constructors being highlighted when at the class name and that being different the other way around might also be confusing for users.

It being implicit is the key here though. If the user is with the cursor over the explicit new (as on my second screenshot), that is what I'd expect, but that is definetly not what I'd expect when placing the cursor at the two implicit new constructors (the line with // Constructor and inside myMethod).

[bwilkerson]

Implicit code creates all kinds of problems for the UX, and there's often no ideal solution.

I suspect that most users think of the name of a class in an invocation of the "unnamed" constructor where the name 'new' is implicit (return MyClass();) to be a reference to the constructor rather than a reference to the class. (I'm not sure that isn't true for the case where the name of the constructor is given explicitly as well, but we treat it as a reference to the class in order to provide more information / navigation options.)

But I won't be surprised to find that I've argued both sides of the question at various times in the past.

... maybe we can try to document the ideas somewhere ...

Absolutely. Both the principles we follow, and, in cases like this, which principles outweighed others when we made our decision.

[FMorschel]

Take for example, this file within the SDK pkg\analysis_server\lib\src\domains\analysis\occurrences_dart.dart:

We have a call to the implicit new constructor for DartUnitOccurrencesComputerVisitor at line 14:

sdk/pkg/analysis_server/lib/src/domains/analysis/occurrences_dart.dart

Lines 13 to 16 in 8d693e0

	void addDartOccurrences(OccurrencesCollector collector, CompilationUnit unit) {
	var visitor = DartUnitOccurrencesComputerVisitor();
	unit.accept(visitor);
	visitor.elementsOffsetLengths.forEach((engineElement, offsetLengths) {

And we have never written the default constructor for it. See these screenshots:

(Only a reference so you easily see where DartUnitOccurrencesComputerVisitor is written)

Now, if we highlight either, the other doesn't show up:

[FMorschel]

Another case I'd like to add to this discussion is implicit new constructors. So for a code like this:

class MyClass {
  void myMethod() {}
}

void myFunction() {
  MyClass^().myMethod();
}

Imagine that the file above is (of course) big, and I want to see where the constructors are:

I can't go to the class name to do that anymore. I must find a constructor first.

[bwilkerson]

We definitely need to think about the design of this, especially in light of the new language features we're starting to work on that will (a) impact the way constructors are declared and (b) introduce more places where a single name means more than one thing.