Highlighting bug for named record fields

Describe the bug
Selecting the name of a record field highlights everything matching the name.

Dart-Code-Log-2024-09-02 08-36-25.txt

To Reproduce
Steps to reproduce the behavior:

Open a new project
Add the repro:

void foo(int variable) {
  var record = (variable: variable,);
}

Select the parameter variable and see it highlighting itself and its use (the last variable on line 2)
Select the record name variable and see every variable highlighted.

Expected behavior
I expect the only highlight to be the record name in the second case.

Please complete the following information:

Workspace Environment

Dart Code extension: 3.99.20240930
Flutter extension: 3.99.20240930 (activated)

App: Visual Studio Code
App Host: desktop
Version: win 1.93.1

Workspace type: Dart (LSP)
Workspace name: bug

Dart (3.6.0-edge.c3b2672a19ef46317e8d71d73e7094eb4c5bca1a): C:\Users\felip_0vh5fa6\dart
Flutter (3.24.3): C:\Users\felip_0vh5fa6\.puro\envs\stable\flutter (No device)

Output from 'dart info'

C:\Users\felip_0vh5fa6\dart\bin\dart.exe info

If providing this information as part of reporting a bug, please review the information
below to ensure it only contains things you're comfortable posting publicly.

General info

Dart 3.6.0-edge.c3b2672a19ef46317e8d71d73e7094eb4c5bca1a (main) (Mon Sep 23 21:51:31 2024 +0000) on "windows_x64"
on windows / "Windows 10 Pro" 10.0 (Build 22631)
locale is pt-BR

Project info

sdk constraint: '>=3.3.0 <4.0.0'
dependencies: collection, dart_either, either_dart, equatable, path, test, time
dev_dependencies: dart_code_metrics, lints

Process info

Memory	CPU	Elapsed time	Command line
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe
0 MB	--		dart.exe

The reason this occurs is that we don't find any occurrences here, so we return null. This causes VS Code to fall back to its built-in behaviour of searching the document.

While we could change this to return an empty list instead of null, in the example above it would prevent the single variable symbol where the cursor is from being highlighted - really we should be returning a single instance that covers that symbol. Probably record named fields just aren't handled correctly by the occurrences code.

@bwilkerson

I've a fix at https://dart-review.googlesource.com/c/sdk/+/387961 that will prevent the bad highlighting (by returning an empty list instead of null, preventing VS Code trying to handle this).

However, because we don't produce any occurrence (not even one for the field name under the cursor), it also means we don't highlight the symbol like we would for other things. Here, "paramName" gets highlighted when I put my cursor in it, but recordFieldName does not:

occurrences.mp4

I think it would be nice to fix that even if there's no element (but currently Occurrences are based entirely on elements). But I think it would even be nice to support linking the field names on records as occurrences if they are the same record type. For example the as in the variables a and c here, and both print statements that use them, I think should be considered references:

void f() {
  var a = (a: 1, b: 2); // a here
  var b = (a: 1, b: 2, c: 3); // NOT a here
  var c = (a: 4, b: 5); // a here

  print(a.a); // a here
  print(b.a); // NOT a here
  print(c.a); // a here
}

@bwilkerson any opinions on this? It means Occurrences (or a variation of) would need to handle named record fields (and possibly indexed fields) specifically (without elements). This might not be compatible with the legacy protocol (which returns the Elements), but we could make something protocol-agnostic and then just filter to the element-based occurences for that.

Although - I do also wonder whether this "problem" goes further than occurrences... For example today I can't do "Find References" on a record name, but it would sure be nice if I could (I've tried this so many times and then remembered it doesn't work). Is there a way we could handle all of those? For example something like a synthetic Element for these record fields (that we could also find references for)?

Whenever we have an extension type that wraps a record (of course also adding the getters) we have the right highlighting. Maybe something similar to this? It would also work for "Find References" I think.

Edit

Today we have dart-lang/language#3839 for tracking extension type to implements its own record type.

The problem is that record types (and function types) are structural types. See dart-lang/sdk#53024 (comment) for more context.

[analysis_server] Return empty document highlights instead of null wh…

Assuming this means we couldn't/shouldn't consider fields with the same name of two records with the same shape to be equivalent generally, what about just supporting it for Document Highlights (which only apply to the current file)?

Eg. instead of using an element as the container for the references, we just produce some synthetic identifier that is a combination of the type (so each differently shaped record would be unique) and the field name/index? It seems a shame that in code like:

var r = (a: 1, b: 2);
print(r.a);

That we don't highlight both as when the cursor is in one, regardless of the specifics. We could perhaps limit it to just the scope where a record variable is defined - however I don't think users would be surprised if identically shaped records in the file were also highlighted as being related, because it's very possible the same type is being passed between functions within the file.

Although, perhaps this is a slippery slope... if they highlight, it might then be unclear why things like Find References don't work... and then a special case just for document highlights might not look like the right choice.

It's a thorny issue, and I agree that we probably need to look at all of our features to make sure that we're handling it consistently.

... synthetic identifier that is a combination of the type (so each differently shaped record would be unique) and the field name/index?

Two records with the same shape (type) could be completely unrelated, so I'm not sure that's enough. Unless the records have named fields, the shape is defined purely by the types of the fields.

It seems a shame that in code like ...

I agree, 100%

But it would also be a shame to highlight references to two unrelated fields at the same time just because they had the same name.

But it would also be a shame to highlight references to two unrelated fields at the same time just because they had the same name.

Yeah, that's fair. Is there any path from a record field name back to where the type for it was defined? For example in the code:

void f(({int a}) r1) { // record 1
  var r2 = (a: 2); // record 2 with same shape
  var r3 = r2;

  print(r1.a); // access 'a' of "record 1"
  print(r2.a); // access 'a' of "record 2"
  print(r3.a); // access 'a' of "record 2" again
}

Is there an easy/quick way from a in r1.a to know that the type comes from the parameter, and that a in r2.a that the type comes from the local variable, and that a in r3.a also comes from the r2 local variable? Although, again, if we implemented that for document highlights, maybe there would be an expectation for Find References/rename etc. to work too, so perhaps solving it in document highlights isn't the right approach.

If there's the possibility of supporting this, should I move this to the SDK (or update dart-lang/sdk#53024 to be more general)? If it's unlikely, we can just close this with the fix above (which stops the incorrect highlighting, but by not providing any highlighting for this case).

Is there an easy/quick way from a in r1.a to know that the type comes from the parameter, ...

Yes. If we have a reference to a field on a record-typed variable then we could look at the declaration of the variable to see whether there's a type. If there is, then we could conclude that the field in the type is related to the field in the access.

... and that a in r2.a that the type comes from the local variable, ...

Yes. We could extend the logic above to look, when there's no type annotation on the variable, to look at the initializer to see whether it's a record literal. If it is, then we could conclude that the accessed field and the field in the literal are related.

... and that a in r3.a also comes from the r2 local variable?

Yes, but it would require having a data flow analysis that we don't currently have.

If there's the possibility of supporting this ...

I hope that we will be able to do a better job with structural types at some point, but I don't know whether it will ever be a high enough priority to actually schedule the work it would take. That's not the answer I'd like to give, but I think that's the reality of the situation.

Thanks for the info! I think it makes sense to close this with the fix above then (we no longer cause VS Code to highlight incorrect references). I'll add a note on dart-lang/sdk#53024 about this issue since they're similar and another issue can be split out if one is to be implemented in a way that doesn't solve the other.

Navigation Menu

Repository navigation

Highlighting bug for named record fields #5294

General info

Project info

Process info

Edit

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Highlighting bug for named record fields #5294

Description

General info

Project info

Process info

Activity

DanTup commented on Oct 1, 2024

DanTup commented on Oct 2, 2024

FMorschel commented on Oct 2, 2024

Edit

bwilkerson commented on Oct 2, 2024

DanTup commented on Oct 2, 2024

bwilkerson commented on Oct 2, 2024

DanTup commented on Oct 3, 2024

bwilkerson commented on Oct 3, 2024

DanTup commented on Oct 3, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions