dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.24k stars 1.58k forks source link

Syntax higlighting issues with object patterns #53478

Open halildurmus opened 1 year ago

halildurmus commented 1 year ago
  final MapEntry(:key) = const MapEntry<String, int>('a', 1);

  print(key);

  if (const MapEntry('a', 1) case MapEntry(:final key)) {
    print(key);
  }

Given the code above, when I place the cursor inside the key variable, the highlight offset seems incorrect (:ke is highlighted instead of key), and the key identifier is not highlighted as expected. However, highlighting works correctly if the cursor is placed at the end of the key variable or inside the key identifier:

bug1

Another issue is observed when placing the cursor inside the key variable; the highlight offset for the key variable declaration inside the if-case statement appears to be inaccurate (:fi is highlighted instead of key): bug2

Dart version:

Dart SDK version: 3.1.1 (stable) (Tue Sep 5 12:20:14 2023 +0000) on "windows_x64"
scheglov commented 1 year ago

I don't know what kind of highlighting this is. I usually keep a possibly related setting disabled.

image

But even when it is enable, highlighting looks correct to me.

image

Maybe some plugin does it? I tested on IntelliJ Build #IC-232.9921.47, built on September 12, 2023. And Dart SDK version: 3.2.0-edge.0ac6f0846afe9a9da72fdbb0a595ac70ec06758b (be) (Mon Sep 11 15:30:02 2023 +0000) on "macos_arm64".

halildurmus commented 1 year ago

I don't know what kind of highlighting this is.

It's called Occurrences Highlight in VSCode: Screenshot 2023-09-14 114454

... But even when it is enable, highlighting looks correct to me. image

Shouldn't the key identifier in Ln 4 also be highlighted?

scheglov commented 1 year ago

@DanTup for VS Code question. @jwren for IntelliJ occurrences implementation.

I don't know for sure why key is not highlighted in print(key). It depends on the way it is implemented in the IDE and VS Code.

Theoretically when you say final MapEntry(:key), the word key means both, the getters name, and the name of the declared local variable. And we do have navigation from :key to the getter, and from print(key) to the :key.

DanTup commented 1 year ago

I think there are two issues here:

  1. The offset for the object pattern fields is off - it's not taking the colon into account
  2. In LSP/VS Code, we don't handle cases where there are two element references at the same location (:key is both a variable declaration and a getter reference) and only return the first group that contains an occurrence at the required offset

It's possible that IntelliJ also has some variation of 2 though.. It's not clear if the legacy protocol is intended to handle two Occurrences that overlap.

DanTup commented 1 year ago

Possibly fixes at https://dart-review.googlesource.com/c/sdk/+/326220, but not certain whether the overlapping Occurrences is the right thing for IntelliJ.

bwilkerson commented 1 year ago

I'm attempting to answer that question now.

scheglov commented 1 year ago

Looking into Dart plugin for IntelliJ source code, I think that it does not use analysis.occurrences at all. It might be using https://github.com/JetBrains/intellij-plugins/blob/21daa3f44caf2b74a0a3b0e2c5fa49375103ea57/Dart/src/com/jetbrains/lang/dart/resolve/DartResolver.java#L28 indirectly, maybe based on navigation information, but I'm not sure. Maybe @jwren knows.

DanTup commented 1 year ago

Looking into Dart plugin for IntelliJ source code, I think that it does not use analysis.occurrences at all.

Ah yes, this was noted at https://github.com/Dart-Code/Dart-Code/issues/4454#issuecomment-1494176327

jwren commented 1 year ago

Looking through the Dart IJ plugin, NotificationAnalysisOccurrencesProcessor processes the analysis.occurences, but I don't see any Occurrences references that plug into the IJ platform for this kind of highlighting.

IIRC, these highlighting are done with a simple local dart resolver for local instances. @alexander-doroshko will know

alexander-doroshko commented 1 year ago

IntelliJ Dart plugin doesn't use org.dartlang.analysis.server.protocol.Occurrences, but it doesn't use its own resolver either :). Occurrences highlighting is based on navigation regions, which come from the server. IDE looks for the word occurrences in the file and checks which of them have the same navigation target. Is there any similar strange behavior in IntelliJ?

DanTup commented 1 year ago

https://github.com/dart-lang/sdk/commit/23e968139222fb4331eac1a20dfaa7c1d95c7f28 should fix this issue for the server occurrences and LSP/VS Code, but I'm not sure if there's still outstanding work for IntelliJ here if it's not using that.