Skip to content
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

Inlay hints wrong order for records #2414

Closed
gruvw opened this issue Apr 15, 2022 · 10 comments · Fixed by eclipse-jdtls/eclipse.jdt.ls#2181
Closed

Inlay hints wrong order for records #2414

gruvw opened this issue Apr 15, 2022 · 10 comments · Fixed by eclipse-jdtls/eclipse.jdt.ls#2181
Assignees

Comments

@gruvw
Copy link

gruvw commented Apr 15, 2022

Hello, when using the new inlay hints I just discovered that sometimes the order of the arguments was not correct.

Example:
inlay_order_issue

For the moment it only happened to me when using java records syntax.

Environment
  • Operating System: Ubuntu
  • JDK version: 17
  • Visual Studio Code version: 1.66.2
  • Java extension version: v1.5.0
@FieteO
Copy link
Contributor

FieteO commented Apr 15, 2022

I think this is caused by the getDeclaredFields() method not returning the fields in a particular order (called in the InlayHintVisitor).
From the javadoc:

	/**
	 * Returns a list of bindings representing all the fields declared
	 * as members of this class, interface, or enum type.
	 *
	 * <p>These include public, protected, default (package-private) access,
	 * and private fields declared by the class, but excludes inherited fields.
	 * Synthetic fields may or may not be included. Fields from binary types that
	 * reference unresolved types may not be included.</p>
	 *
	 * <p>Returns an empty list if the class, interface, or enum declares no fields,
	 * and for other kinds of type bindings that do not directly have members.</p>
	 *
	 * <p>The resulting bindings are in no particular order.</p>
	 *
	 * @return the list of bindings for the field members of this type,
	 *   or the empty list if this type does not have field members
	 */
	public IVariableBinding[] getDeclaredFields();

@jdneo
Copy link
Collaborator

jdneo commented Apr 18, 2022

Hi @angelozerr,

Do you have any idea how we can support inlay hints for Record class? The current problem is that getDeclaredFields() does not return fields with declared order.

@angelozerr
Copy link

No sorry I cannot help you for this topic.

@jdneo
Copy link
Collaborator

jdneo commented Apr 19, 2022

The correct parameter names are stored in the internal MethodBinding. But I didn't find a way to access it.
image

@rgrunber
Copy link
Member

rgrunber commented Apr 20, 2022

It doesn't look like there's currently a way to access it :\ , although it looks possible to add the API to MethodDeclaration (IMethodDeclaration).

The only other way I can see to do this is by getting access to the compilation unit where the record is declared, and go to the RecordDeclaration node. It has a method recordComponents() that lists the fields in order (since it just represents the record declaration). This seems tricky because the record invocation(s) and declaration can be different files.

@jdneo
Copy link
Collaborator

jdneo commented Apr 20, 2022

This seems tricky because the record invocation(s) and declaration can be different files.

Yes, that means we need first find the record type in the Java project, and then get its type root, which would be too expensive for displaying inlay hints.

How about we try to add the API at upstream? Before the new API is available, we disable the inlay hint for record method invocations temporarily.

@jdneo
Copy link
Collaborator

jdneo commented May 24, 2022

@rgrunber Will you add that required API at upstream?

@rgrunber rgrunber changed the title Inlay Hints Wrong Order Inlay hints wrong order for records Jun 16, 2022
@rgrunber
Copy link
Member

JDT Core : eclipse-jdt/eclipse.jdt.core@master...rgrunber:param-names-api
JDT UI : eclipse-jdt/eclipse.jdt.ui@master...rgrunber:param-names-records

The JDT Core change is all that's needed to expose the necessary API. We can probably write the same code as in JDT UI.

param-names-hints-records-fix

I'll go ahead and propose these changes.

@rgrunber
Copy link
Member

rgrunber commented Jul 21, 2022

The API to do this on JDT Core has been available since https://download.eclipse.org/eclipse/updates/4.25-I-builds/I20220719-1800/ . I've also merged a change to fix the issue on Eclipse, so we should be able to go ahead and fix it in JDT-LS now.

See https://github.com/eclipse-jdt/eclipse.jdt.ui/pull/149/files for an example.

@jdneo
Copy link
Collaborator

jdneo commented Jul 28, 2022

Thank you @rgrunber, I'll follow up the fix at the JDT.LS side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants