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.22k stars 1.57k forks source link

[analyzer] `completion.getSuggestions2` to return the `target` of `PropertyAccess` #48870

Open asashour opened 2 years ago

asashour commented 2 years ago

To support https://github.com/flutter/flutter-intellij/issues/6037, for a case like:

Colors.blue.shade|

should the LSP Element have the target of the PropertyAccess, so the actual shade value can be calculated?

stevemessick commented 2 years ago

See also https://youtrack.jetbrains.com/issue/WEB-55625/Dart:-Missing-color-info-in-CompletionSuggestion

Note that this is a feature request for the Flutter plugin for IntelliJ. I'm unclear if changing LSP would help. It may also require small changes to the Dart plugin. Again, that's not clear yet. (I'd have to study the code, which I have not done recently.)

cc @jwren @bwilkerson

bwilkerson commented 2 years ago

We can't change LSP; we don't own the protocol.

We can change the legacy protocol. Let me know what information you need, if anything.

asashour commented 2 years ago

When the user is at the cursor, getSuggestions2 currently returns the below, however it doesn't say which color (e.g. blue). This is the missing information.

{
  "kind": "IDENTIFIER",
  "relevance": 562,
  "completion": "shade100",
  "selectionOffset": 8,
  "selectionLength": 0,
  "isDeprecated": false,
  "isPotential": false,
  "docSummary": "",
  "docComplete": "",
  "declaringType": "MaterialColor",
  "element": {
    "kind": "GETTER",
    "name": "shade100",
    "location": {
      "file": "C:\\use\\flutter\\packages\\flutter\\lib\\src\\material\\colors.dart",
      "offset": 1195,
      "length": 8,
      "startLine": 29,
      "startColumn": 13,
      "endLine": 29,
      "endColumn": 21
    },
    "flags": 0,
    "returnType": "Color"
  },
  "returnType": "Color"
}
bwilkerson commented 2 years ago

... however it doesn't say which color ...

I think we need to be more specific about what information we're proposing to return. In other words, how do we want that information to be represented? Is a string containing 'Colors.blue' sufficient? Or do we need a representation of the component values of the color?

Do we want this to work only for constants defined in Colors, or do we want this to work for any constant whose type is Color?

asashour commented 2 years ago

To satisfy the flutter plugin request, this is relevant to only Colors, I assume that only the blue or Color.blue is sufficient, and the plugin can determine and values as in here, @stevemessick would know better in this regard.

bwilkerson commented 2 years ago

@DanTup

stevemessick commented 2 years ago

The classes Colors and CupertinoColors have a lot of static fields that define named colors. We need to get both Colors and blue so that we can retrieve the correct named color. The return type (note the lack of 's') and declaring type do not help. I don't know how this would be added to the json.

bwilkerson commented 2 years ago

Sounds like we need to add a field to CompletionSuggestion whose value is a string, which, when composed with the name of the element being suggested would form the key used to find the color. If we also want to be able to display an icon when completing after Icons, then the field name should be fairly general (like 'keyPrefix').

stevemessick commented 2 years ago

That sounds good. And yes, dart-code would benefit from having support for icons. I think the IntelliJ plugin handles icons via PSI (the built-in parser).

DanTup commented 2 years ago

In order to have the colors render in VS Code via LSP, we need to set either the label, detail or documentation to exactly the hex code. None of those are particularly great because we already use them all, however in the next version of LSP (due to be published in the coming weeks) we have a new description that overrides the previous detail field - however the original detail field is still checked for hex colors. This means we could include the hex value in this ("hidden") field to get colours without sacrificing the info shown today (although how useful showing Color or MaterialAccentColor as the type for colors, I'm not sure) - as long as don't mind the details showing up all the time:

Apr-26-2022 11-11-23

Here, the top entry is what we do today (minus the icon, since we're not setting CompletionItemKind.Color) - no color. The middle item is what we can do today (that is, replace the type in the detail field (MaterialAccentColor) with a hex code), and the bottom item is what we can do in the next version of LSP (keep the original type and still include a hidden hex color, although the type name now shows all the time - something that might make the completion list too busy so may need some evaluation).

In any case, we would need to get the hex code in the server. We can already compute hex codes for colors but I don't know how easily that can be used from the completion mapping code. If it could, that would allow all colors that can be computed to show previews, not just the trivial ones that can be done with a map.

bwilkerson commented 2 years ago

@DanTup

I like the idea of using hex values, it's much more general than a string like 'Colors.blueAccent'.

Do we also want to support icons? If so, what mechanism would we use?

DanTup commented 2 years ago

Showing icons in the completion list directly isn't currently supported in LSP (see https://github.com/microsoft/language-server-protocol/issues/1225). We do currently support them in the docs that pop out for VS Code:

Screenshot 2022-04-26 at 11 30 08

However, that's done client-side and not in the server because it currently relies on having a copy of all the icons somewhere and we don't have that from the analysis server (instead we ship a copy of them in the VS Code extension, from https://github.com/flutter/tools_metadata).

IntelliJ has code to extract the icons from the font file (https://github.com/flutter/flutter-intellij/pull/5504) but it's Java. I don't know if it makes sense to have some equivalent of that in the analysis server, or if it's feasible for package:flutter (and potentially others) to ship something in a well-known location that could be used to look them up. There's an issue relating to this at https://github.com/dart-lang/sdk/issues/47667.