8000 Support for inlayHints user preferences in language server · Issue #60326 · dart-lang/sdk · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support for inlayHints user preferences in language server #60326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebthom opened this issue Mar 15, 2025 · 10 comments
Open

Support for inlayHints user preferences in language server #60326

sebthom opened this issue Mar 15, 2025 · 10 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. 8000 devexp-server Issues related to some aspect of the analysis server P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug type-ux A user experience or user interface related issue

Comments

@sebthom
Copy link
sebthom commented Mar 15, 2025

Please support user configuration of the inlayHints support for the Dart language server similar to the JS/TS language server or the Haxe Language Servers .

This allows end-users to configure for which elements they want inlay hints to be displayed.

This seems currently not possible according https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/tool/lsp_spec/README.md

@bwilkerson bwilkerson added the area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. label Mar 15, 2025
@bwilkerson
Copy link
Member

@DanTup

@DanTup
Copy link
Collaborator
DanTup commented Mar 17, 2025

I like this idea. We'd need to decide exactly what settings to provide.. If we copy TypeScript (but exclude enumMemberValues because I don't think that's useful for Dart because you generally never reference enums using the index) we'd have:

  • dart.inlayHints.returnTypes.enabled - whether to include return types
  • dart.inlayHints.parameterNames.enabled:
    • none: don't show any parameter names
    • literal: only show parameter names for literal values (x: true) and not those using variables (myParam: myVariable)
    • all: show all parameter names (what we do today)
  • dart.inlayHints.parameterNames.suppressWhenArgumentMatchesName - whether to suppress parameter name labels if the name matches the value (this only applies to all above and prevents myFoo: myFoo)
  • dart.inlayHints.parameterTypes.enabled - whether to include inferred parameter types (eg. stringList.map((/*String*/ s))
  • dart.inlayHints.propertyDeclarationTypes.enabled - whether to include inferred types for property declarations (/*String*/ get foo => '';)
  • dart.inlayHints.variableTypes.enabled - whether to include inferred types for variable declarations (var /*int*/ a = 1)
  • dart.inlayHints.variableTypes.suppressWhenTypeMatchesName - whether to suppress variable types when the type matches the name (var myFoo = MyFoo())

Two questions I have:

  • Do we include class fields as "propertyDeclarations" or "variables"?
  • Should we add a dart.inlayHints.propertyDeclarationTypes.suppressWhenTypeMatchesName because it's not clear to me why that shouldn't have the same option as variables (for example get myFoo => getMyFooInstance() seems like it would be also useful to suppress the duplicate MyFoo?

@bwilkerson
Copy link
Member

dart.inlayHints.parameterNames.suppressWhenArgumentMatchesName

Should we just add a value between literal and all in the values for dart.inlayHints.parameterNames.enabled? Seems like it would be more discoverable, and would reduce the number of options for users to have to consider.

dart.inlayHints.variableTypes.suppressWhenTypeMatchesName

Do we want this to be a fuzzy match? For example, what if the type is String and the variable name is firstString? Or if the type is UniversallyUniqueIdentifier and the variable name is uuid?

Do we include class fields as "propertyDeclarations" or "variables"?

Semantically they're property declarations, so that would be my recommendation.

Should we add a dart.inlayHints.propertyDeclarationTypes.suppressWhenTypeMatchesName ...

Or just generalize the two into a single option that controls any location in which type information might be displayed? What about return types, such as a return type of Customer from a method named getCustomer? What I don't know is whether users want to control all such locations independently.

For that matter, should returnTypes be different than propertyDeclarationTypes? I always think of getters as having a return type, but maybe that's the wrong mental model.

@DanTup
Copy link
Collaborator
DanTup commented Mar 17, 2025

Should we just add a value between literal and all in the values for dart.inlayHints.parameterNames.enabled? Seems like it would be more discoverable, and would reduce the number of options for users to have to consider.

I wondered about that too (this is how TS does it and I was copying them for some consistency). I don't know if they had the suppressWhenTypeMatchesName booleans first, or they wanted to use this consistently for all values without making the others enums (I couldn't find the source for this or any discussion).

If we did this, what should it be called, and should we convert the other bools to be an enum for this? (none, onlyIfDifferentFromName(?), [literals], all)

Do we want this to be a fuzzy match? For example, what if the type is String and the variable name is firstString? Or if the type is UniversallyUniqueIdentifier and the variable name is uuid?
[...]
What about return types, such as a return type of Customer from a method named getCustomer?

My feeling is we should be fuzzy only for things like casing/underscores to avoid hiding any potentially important context?

Semantically they're property declarations, so that would be my recommendation.

I think for semantic tokens we map them as "instance variable declarations" or something (although references to them are properties because they are references to the getter/setter), but I'm not sure if the logic for that is sound. The reason I wondered about variables is that anything we put together in a category can only be togged together, and I wonder if a class field is something more likely to want to be toggled with variables (since they're of a similar form) compared to getter/setters. (Ofc, we could give them their own category and avoid choosing).

Or just generalize the two into a single option that controls any location in which type information might be displayed?

hmm, I think I prefer this to the idea above. I feel like if you've chosen not to include "duplicate" labels, probably you'd want that everywhere?

For that matter, should returnTypes be different than propertyDeclarationTypes? I always think of getters as having a return type, but maybe that's the wrong mental model.

Good question. I copied this from TS and didn't really question it, but I wonder if there's much value in separating properties at all (eg. getters could use returnTypes and setters parameterTypes).

Looking at the Haxe list posted above, that does also seem much simpler than TS with just a few bools:

inlayHints: {
			variableTypes: false,
			parameterNames: false,
			parameterTypes: false,
			functionReturnTypes: false,
			conditionals: false // (I'm not sure what this one does)
		},

So maybe something like:

  • dart.inlayHints.returnTypes.enabled - (functions, getters)
  • dart.inlayHints.parameterNames.enabled:
    none (or false?): don't show any parameter names literal: only show parameter names for literal values (x: true) and not those using variables (myParam: myVariable)
    `all (or true): show all parameter names (what we do today)
  • dart.inlayHints.parameterTypes.enabled - include inferred parameter types (eg. stringList.map((/String/ s))
  • dart.inlayHints.variableTypes.enabled - inferred types for variable + field declarations (var /int/ a = 1)
  • dart.inlayHints.suppressWhenTypeMatchesName - exclude any where the textual part (ignoring underscores, casing) is the same as the adjacent identifier
    By keeping the enabled suffix for the option, if we wanted to support suppressWhenTypeMatchesName on an individual category in future, we could add that (and just use the parent level one as a default if unspecified).

@pq pq added devexp-server Issues related to some aspect of the analysis server type-ux A user experience or user interface related issue type-enhancement A request for a change that isn't a bug P3 A lower priority bug or feature request labels Mar 17, 2025
@bwilkerson
Copy link
Member

Do we want this to be a fuzzy match?

My feeling is we should be fuzzy only for things like casing/underscores to avoid hiding any potentially important context?

The reason this is hard is that it isn't possible (in general) to determine what context, if any, might be sufficient.

I assume we'd consider a name to match as long as the type name is somewhere in the name, such as a variable named highlightColor and a type named Color. What about the other direction, such as a variable named box whose type is BoundingBox? (I'm obviously concerned that it's going to be hard to get the right definition of "matching".)

I know that Erik has done some work to define 'obvious' for types. I wonder whether that might be another factor to take into account. Do we want to inline the type of a variable if the type is obvious based on the initialization expression? For example, consider var x = <String>[];. Or would that be another separate option, such as dart.inlayHints.suppressWhenTypeIsObvious? (I'm also a bit concerned about feature creep.)

To be clear, I'm not opposed to making the feature be configurable. I just don't want it to get out of hand. I like the second list of possible options more than the first.

@DanTup
Copy link
Collaborator
DanTup commented Mar 19, 2025

I assume we'd consider a name to match as long as the type name is somewhere in the name, such as a variable named highlightColor and a type named Color

For some reason I only thought about this the other way around. But I think you're right that the type is probably redundant in this case. I'm less sure about the other way (BoundingBox adds something that isn't visible in box). For ex.:

  • BoudingBox box - the type name still adds context (what kind of box?)
  • Color highlightColor - the type name doesn't add any context
  • A bar - here the type name adds context, but the name does contain it (allowing for casing differences)

I wonder if we should split names into "segments" (where there are capitals, underscores, start/end of identifier, etc.) and then do only complete matches on those? (eg. highlightColor would be split into ["highlight", "color"] and then we'd match color)

I know that Erik has done some work to define 'obvious' for types.

I don't know much about this, but if we have an existing concept for this and we could reuse it here, I think that could be nice (and having its own option makes sense). Perhaps it's something to look at separately though - perhaps with a little configurability (as noted above) the feature would be more useful and we'd get better feedback on additional tweaks like that.

@bwilkerson
Copy link
Member

I find the lack of a good definition of what we mean by "matching" to be concerning. I'd like to hold off on that particular preference until we have a more concrete definition of what it does, but the test of the preferences seem reasonable to me.

I know that Erik has done some work to define 'obvious' for types.

For more context, see #58773.

"Obvious" isn't the same for everyone, but the concept is, I think, well defined for the lints.

@sebthom
Copy link
Author
sebthom commented Apr 14, 2025

I really don’t understand why this seemingly gets turned into a PhD dissertation. Couldn’t this just be done similarly to how the TypeScript language server handles it - just without the settings that don’t make sense for Dart?

Holding off for some “concrete” definitions basically feels like this will never be solved. Is there a dedicated, trackable task whose goal is to figure out these ominous definitions?

Sorry if this comes off as rude, that’s not my intention - I’m just genuinely confused why this has to be so complicated.

@bwilkerson
Copy link
Member

I can only speak for myself, but in my experience having a good design leads to a better UX and a more maintainable implementation.

As for the approach, I think we are, in fact, talking about doing something similar to how the TypeScript server handles it and were discussing what does and doesn't make sense for Dart. And the only part of this that I wanted to hold off on is the implementation of any kind of suppressWhenTypeMatchesName option.

But as for why the conversation stopped without reaching a conclusion, it's because this isn't currently considered to be a high priority for our team. We need to focus on the more critical issues, which at the moment includes some significant performance concerns.

@DanTup
Copy link
Collaborator
DanTup commented Apr 15, 2025

Sorry if it seemed like progress wasn't being made here.

My feelings match Brian's - it's difficult to change things like settings once they're shipped, so spending a little time up-front can save effort (or avoid a less-ideal design when changing things) later. I also thought it would be useful to get feedback/opinions (including yours) on what was proposed before starting something that might not solve the original request.

I do think this would be a great change for inlay hints because I still find them slightly noisy myself (and therefore use them less than I think I would if this was implemented), but there are also many other ideas (and fixes) competing for resources that this needs to be weighed up against.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-server Issues related to some aspect of the analysis server P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug type-ux A user experience or user interface related issue
Projects
None yet
Development

No branches or pull requests

4 participants
0