Incorrect highlighting of other occurrences with "new" default constructor references

While investigating #4436, I noticed this odd highlighting:

==> {
	"jsonrpc": "2.0",
	"id": 213,
	"method": "textDocument/documentHighlight",
	"params": {
		"textDocument": {
			"uri": "file:///Users/danny/Dev/TestStuff/dart-code-metrics/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart"
		},
		"position": {
			"line": 167,
			"character": 7
		}
	},
	"clientRequestTime": 1679565486647
}

<== {
	"id": 213,
	"jsonrpc": "2.0",
	"result": [
		{
			"range": {
				"end": {
					"character": 16,
					"line": 168
				},
				"start": {
					"character": 70,
					"line": 166
				}
			}
		}
	]
}

Small repro:

final a = AAAAAAAAAAAAAAAAAAAAAAA.new;

class AAAAAAAAAAAAAAAAAAAAAAA  {
  AAAAAAAAAAAAAAAAAAAAAAA();
}

Likely related to new but using the length of the class name instead of the length of "new".

@bwilkerson

@bwilkerson the current implementation of Occurrences assumes that all occurrences have the same length:

class Occurrences {
  List<int> offsets;
  int length;
}

This doesn't handle constructors referenced using ".new" (3 vs either 0 or the length of the class name). Some possible fixes:

Add a new List<int> lengths that has the same length as offsets that can be used instead (this is simplest but the API would allow the lengths to be different)
Add a List<Location> location that contains all ranges (this bloats the payload a little, since Location has file/startLine/startColumn/endLine/endColumn, although having line/col isn't necessarily a bad thing)
As above, but create a new class Occurrence (or Range or similar) that is just offset/length instead of using Location

Another option (if we wanted to avoid changing the protocol and updating IntelliJ/AS) would be to drop "new" from the results and only support it for LSP (although we'd need some tweaks along the lines of the above to return the lengths to use in the LSP handler).

Given the following code:

class C {
  C();
  C.named();
}

class D extends C {
  D() : super();
}

void f() {
  [
    C(),
    C.new(),
    C.named(),
  ];
}

Server is currently highlighting the 'C' and not the '.new'. It's also highlighting the 'C' and not the '.named' whenever some reference to 'C' is selected. Effectively, it's treating all of the 'C's as references to the class, not references to the constructor.

That doesn't seem ideal, but it also doesn't seem terrible. Definitely something we should look into at some point, but I'm not sure it has a very high priority.

If we were going to solve it, then my inclination (as a general principle) would be to replace the uses of the legacy protocol object with a protocol-agnostic object that has all of the information needed by both protocols.

@bwilkerson

@bwilkerson using a single-character class name slightly masks the issue here. If you make the class name much longer, when your cursor is in new you get this (which initially made me think server had gotten out of sync):

The highlighting includes new(),\n C (which is the length of the class name).

As a short-term fix, we could just exclude new constructor references from the occurrences (this would avoid any highlighting in this case), and then as a proper fix later do as you suggest and introduce a new type (though we'd still need to decide how to handle the original protocol - either protocol changes or just always dropping new there?).

Oh, I see. Definitely worse than I'd realized.

I'm not sure whether the loss of functionality is better or worse than the highlighting issue. I'm kind of inclined at this point to just leave the highlighting issue as-is until we can do a proper fix, which probably won't take that long to implement.

As for the legacy protocol, I'd want to get Alex's input because I don't know what IntelliJ supports on their side.

@alexander-doroshko

You're right, it's probably better to just fix entirely. Although whatever we do, we'd need to decide whether to have the highlighting issue or drop the new references for any clients of the legacy protocol that haven't been updated to use any new length support we add for occurrences.

@alexander-doroshko if the legacy protocol is updated so that we can provide occurrences with different lengths (eg. if the cursor is in new in FooClass.new, it could highlight the constructor FooClass(); as a reference to match new), or do all occurrences have to be the same length/text as the DAS protocol currently assumes? (I'm assuming IntelliJ is using Occurrences from the server).

IntelliJ's Dart plugin doesn't use Occurrences from the server. Occurrences highlighting works using IntelliJ's internal (language-agnostic) mechanism, but based on the references (aka navigation) info from the server. So it works only when the reference and the target have the same text.

I think we want to switch to the Occurrences from the server eventually.

I think we want to switch to the Occurrences from the server eventually.

If you did, would it support different lengths? (eg. having new in Foo.new and Foo in the constructor declaration Foo() {} being able to be considered a match)

If you did, would it support different lengths?

Yes if we have lengths in Occurrences.

Should MyClass on line 1 and 2 be considered occurrences of the same thing (the name of this class) or different (a class and a constructor)?

Treating them separately seems appropriate to me. In my experience it's very common that I want to find specifically the spots where a constructor is called, which may be just one or two sites while the class name is referenced in dozens of places as the type of a field or a parameter or so on. (For example that combination seems to be very common in the Flutter framework.) So it's quite valuable to be able to look just at the occurrences of the constructor.

Treating them separately seems to be the existing behavior of the IntelliJ Dart plugin's occurrence highlighting, and I regularly find that useful for the same reason.

Conversely when one wants all the references to the class name in general, the class MyClass is usually just a few lines away.

@bwilkerson

I had another quick look at this today and it seems the original bug is no longer there, because new is always highlighted as if it's different to the constructor (and constructors and classes are the same):

Cursor on "new"

Cursor on "AAAAAAAAAAAAAAAAAAAAAAA" default constructor

Before changing anything, I'd like to check we agree on which symbols are grouped together. My suggestion would be as below - I don't think there's anything surprising but the type name might sometimes by "the type name" and sometimes be "the constructor". I think generally the colours here match what Go-to-Definition would do, with one exception - Go-to-Definition on a default implicit constructor would go to the class - however I don't think we should group them together for highlights.

Orange boxes: The type name
Lime Green boxes: The default constructor
Blue boxes: The constructor named "named"

@bwilkerson I'm interested in your opinion on this. The ones I think are less obvious are 2 (it feels similar to 3a in some ways, but we treat it differently here) and 6b (similarly).

I agree with the choices you made. I think there are clear guidelines that make them more obvious choices, and I'll try to write something down to explain the guidelines.

Thanks - I think I have it working as described above now, working on a CL at https://dart-review.googlesource.com/c/sdk/+/406822.

occurrences.mp4

For the legacy protocol we can't support occurrences with multiple lengths (eg. A() and A.new()) in a backwards-compatible way so I'm filtering out occurrences with a different length there. Since IntelliJ is currently using its own implementation I don't think it makes a difference there, but we might want to consider updating the protocol to support that (or, having IntelliJ use LSP-over-Legacy if/when it moves to server-provided occurrences).

... or, having IntelliJ use LSP-over-Legacy if/when it moves to server-provided occurrences

Yes, that.

If IntelliJ isn't using the legacy protocol, then we should remove support for the protocol in order to reduce maintenance costs.

[analysis_server] Support occurrences (Document Highlights) with diff…

If IntelliJ isn't using the legacy protocol, then we should remove support for the protocol in order to reduce maintenance costs.

Makes sense. I've opened dart-lang/sdk#60043 to track this and listed this and Signature Help as possible candidates (there are likely more, so determining what's unused is possibly the first step - I don't know if there analytics that might help with that?).

As for this issue, this is now resolved via dart-lang/sdk@4be9188.

Navigation Menu

Repository navigation

Incorrect highlighting of other occurrences with "new" default constructor references #4454

23 remaining items

Cursor on "new"

Cursor on "AAAAAAAAAAAAAAAAAAAAAAA" default constructor

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Incorrect highlighting of other occurrences with "new" default constructor references #4454

Description

Activity

DanTup commented on Mar 23, 2023

DanTup commented on Mar 23, 2023

bwilkerson commented on Mar 23, 2023

DanTup commented on Mar 27, 2023

bwilkerson commented on Mar 27, 2023

DanTup commented on Mar 27, 2023

alexander-doroshko commented on Apr 3, 2023

DanTup commented on Apr 3, 2023

alexander-doroshko commented on Apr 3, 2023

23 remaining items

gnprice commented on Feb 2, 2024

DanTup commented on Jan 27, 2025

Cursor on "new"

Cursor on "AAAAAAAAAAAAAAAAAAAAAAA" default constructor

bwilkerson commented on Jan 28, 2025

DanTup commented on Jan 30, 2025

bwilkerson commented on Jan 31, 2025

DanTup commented on Feb 4, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions