Fixes test0063, test0064, test0065, and more - getQualifiedName expec… #663

robstryker · 2024-08-06T03:38:05Z

…ts no generics for type declarations

akurtakov · 2024-08-06T14:18:26Z

Rebase in order to see changes in test results towards master results (1292 failures now).

akurtakov · 2024-08-06T16:26:16Z

It seems to increase the failing tests to 1294 while dom-with-javac head has 1292 failures.

testforstephen · 2024-08-08T05:08:50Z

org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java

@@ -247,15 +249,19 @@ public String getKey(Type t) {
 	}
 	public String getKey(Type t, Name n) {
 		StringBuilder builder = new StringBuilder();
-		getKey(builder, t, n, false);
+		getKey(builder, t, n, false, true);


What is the reason for the last argument being true?

The last argument is true, which represents that yes, we want the type arguments. While some calls to these static key methods must have type arguments, specifically top level calls to get the key for a type binding, others must not. When getting the key for a method, for example, it may then need to get the key for the various types, and it does not want the associated generic types in that key.

But a top level call to get the key for the type must include the types, because we later store these bindings in a series of cache maps and use the key as a, well, as a key. If X<String> has the same key as X<T> or X<Integer> then they would clobber each other.

But again, some contexts exist where the types are not wanted via these utility key methods.

testforstephen · 2024-08-08T05:11:11Z

org.eclipse.jdt.core.javac/src/org/eclipse/jdt/core/dom/JavacBindingResolver.java

 		public JavacTypeBinding getTypeBinding(com.sun.tools.javac.code.Type type) {
+			return getTypeBinding(type, false);


why does it set the last argument to false?

The getTypeBinding(com.sun.tools.javac.code.Type type) entry point is exclusively used when starting with a different Symbol and somehow working yourself up the chain to a ClassType. This means that, when using this entry point, we are almost always using a symbol representing a specific instance of a class. This has repercussions for generics. There's a difference between a ClassDeclaration AST node and the underlying javac symbols that back it. If the class is a generic, a ClassDeclaration will typically be X<T> rather than X<String>.

Anyway, it's important to know if we're dealing with a class declaration or not so we know how to interpret the generic parameters. This entry is exclusively used by code that follows up a trail of Symbol owners, which means the original symbol is NOT a ClassDeclaration object and does not have the generic parameters necessary to make the tests pass.

mickaelistria · 2024-08-08T11:12:30Z

This issue also causes other failures related to JavacTypeBinding .isGeneric() vs isParameterized() result depending on whether the bindings originates from a declaration or not. So I think this goes in the right direction overall. The suggested change can also be used as a basis for covering this parameterized/generic issue.
Should the implementation of equals be updated to?
Instead of a boolean argument to a public getTypeBinding, I would suggest that we use a new different method getTypeBindingForDeclaration(Type).

robstryker · 2024-08-08T16:22:11Z

I want to be clear that I don't think this issue "causes" those other issues. Rather, fixing this issue has allowed the test cases to move forward enough that it has revealed those other issues.

I'm still working on this. Let's see how the next build goes. I'm noticing significant improvement in the many of the test cases moving forward, much more forward, in their specific tests before failing later.

mickaelistria · 2024-08-08T20:46:24Z

org.eclipse.jdt.core.javac/src/org/eclipse/jdt/core/dom/JavacBindingResolver.java

 		public JavacTypeBinding getTypeBinding(com.sun.tools.javac.code.Type type) {
+			return getTypeBinding(type, false);
+		}
+		public JavacTypeBinding getTypeBinding(com.sun.tools.javac.code.Type type, boolean isDeclaration) {


I share concern with @testforstephen that the parameter isn't easy to grok, I think this one should be made private, and a new getTypeBindingForDeclaration(Type) be added instead to slightly improve readability.

Agree. In JDT binding model, the qualified name includes the type arguments only if the binding is parameterized type (an instance of generic type), and no type arguments for generic type (the declaration type). That's easier to understand. However, in our implementation, we require the caller to explicitly specify whether it's a declaration or not on a case-by-case basis, which adds complexity. I'm wondering if there's a simpler logic we could use to determine whether type arguments are needed.

I'm wondering if there's a simpler logic we could use to determine whether type arguments are needed.

FWIW, I've made several attempts to find a solution just based on the Type and TypeSymbol, just looking at the actual type arguments for those, and I didn't find a reliable way to distinguish generic vs parameterized with those.
One example of the issue is that

class A<T> { A<T> someInstance; }

JDT expects that resolving the first A<T> type binding for the class is generic, and the type binding for someInstance is parameterized; while in this case, the Javac model seems to provide the exact same type and typesymbol.

Yes, this was my trouble as well. THe underlying javac implementation seems unable to assist us here.

mickaelistria · 2024-08-08T21:02:21Z

org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java

 		if (type instanceof PackageType) {
 			throw new IllegalArgumentException("Use JavacPackageBinding");
 		}
 		this.type = type;
+		this.isDeclaration = isDeclaration;


Should it only be set when type.isParameterized() is true? In other cases, it seems unnecessary to create different bindings.

mickaelistria · 2024-08-08T21:04:16Z

org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java

@@ -855,7 +882,7 @@ public boolean isFromSource() {

 	@Override
 	public boolean isGenericType() {
-		return this.type.isParameterized() && this.type.getTypeArguments().stream().anyMatch(TypeVar.class::isInstance);
+		return this.type.isParameterized() && getTypeArguments() != NO_TYPE_ARGUMENTS && this.type.getTypeArguments().stream().anyMatch(TypeVar.class::isInstance);


Couldn't it be simplified to

Suggested change

return this.type.isParameterized() && getTypeArguments() != NO_TYPE_ARGUMENTS && this.type.getTypeArguments().stream().anyMatch(TypeVar.class::isInstance);

return this.type.isParameterized() && this.isDeclaration;

?

…ts no generics for type declarations Signed-off-by: Rob Stryker <stryker@redhat.com> Cleanup debugging text Signed-off-by: Rob Stryker <stryker@redhat.com> Bad rebase Signed-off-by: Rob Stryker <stryker@redhat.com>

Signed-off-by: Rob Stryker <stryker@redhat.com>

mickaelistria · 2024-08-12T20:40:21Z

It looks like this has introduced some regressions for rawTypes

javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0163
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0189
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0227
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0233
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0238_2
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0293
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0294
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS4Test.test0297
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0163
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0189
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0227
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0233
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0238_2
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0293
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0294
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15JLS8Test.test0297
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0163
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0189
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0227
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0233
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0238_2
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0238_ASTParser
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0293
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0294
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0297
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.testBug527462
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST3Test.test0280
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST3Test.test0317
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST3Test.test0346
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST4Test.test0280
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST4Test.test0317
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST4Test.test0346
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST8Test.test0280
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST8Test.test0317
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterAST8Test.test0346
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterTest.test0280
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterTest.test0317
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverterTest.test0346

robstryker · 2024-08-13T16:46:46Z

I looked into these and while one or two do look related to this PR, the rest seem completely unrelated? I'll keep digging.

mickaelistria · 2024-08-13T21:49:10Z

I've looked into some of them and all the ones I've checked were definitely related. I've started to work on this with #694 .

robstryker · 2024-08-13T23:30:20Z

From what I can tell, the following all have to do with annotation processing counts:

javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0227
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0233
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0238_2
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0238_ASTParser
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0293
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0294
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0297

Only the following two have to do with this PR:

javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0163
javac specific tests / org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0189

In each test case obviously.

I've gotten these tests to pass locally now and will submit a PR.

#663) * Fixes test0063, test0064, test0065, and more - getQualifiedName expects no generics for type declarations Signed-off-by: Rob Stryker <stryker@redhat.com> Cleanup debugging text Signed-off-by: Rob Stryker <stryker@redhat.com> Bad rebase Signed-off-by: Rob Stryker <stryker@redhat.com> * Fix some regressions Signed-off-by: Rob Stryker <stryker@redhat.com> * Problems determing when a type is or is not generic, and regressions Signed-off-by: Rob Stryker <stryker@redhat.com> --------- Signed-off-by: Rob Stryker <stryker@redhat.com> Co-authored-by: Rob Stryker <stryker@redhat.com>

github-actions bot assigned robstryker Aug 6, 2024

robstryker mentioned this pull request Aug 6, 2024

Add a null check to avoid NPE in getQualifiedNameImpl #657

Closed

robstryker force-pushed the test0063-TypeDeclaration-resolveBinding branch from 0daaa17 to 3003f46 Compare August 6, 2024 03:53

akurtakov force-pushed the test0063-TypeDeclaration-resolveBinding branch from 3003f46 to 88db16a Compare August 6, 2024 14:18

robstryker force-pushed the test0063-TypeDeclaration-resolveBinding branch 2 times, most recently from 3f06f83 to f960958 Compare August 7, 2024 15:10

testforstephen reviewed Aug 8, 2024

View reviewed changes

robstryker force-pushed the test0063-TypeDeclaration-resolveBinding branch from 73395e6 to 0eaf5f0 Compare August 8, 2024 19:37

mickaelistria reviewed Aug 8, 2024

View reviewed changes

Rob Stryker added 3 commits August 12, 2024 12:09

Fix some regressions

aeb3126

Signed-off-by: Rob Stryker <stryker@redhat.com>

Problems determing when a type is or is not generic, and regressions

901a147

Signed-off-by: Rob Stryker <stryker@redhat.com>

robstryker force-pushed the test0063-TypeDeclaration-resolveBinding branch from 0eaf5f0 to 901a147 Compare August 12, 2024 16:09

robstryker merged commit 25dacc3 into eclipse-jdtls:dom-with-javac Aug 12, 2024
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes test0063, test0064, test0065, and more - getQualifiedName expec… #663

Fixes test0063, test0064, test0065, and more - getQualifiedName expec… #663

robstryker commented Aug 6, 2024

akurtakov commented Aug 6, 2024

akurtakov commented Aug 6, 2024

testforstephen Aug 8, 2024

robstryker Aug 8, 2024

testforstephen Aug 8, 2024

robstryker Aug 8, 2024

mickaelistria commented Aug 8, 2024

robstryker commented Aug 8, 2024

mickaelistria Aug 8, 2024

testforstephen Aug 9, 2024

mickaelistria Aug 9, 2024

robstryker Aug 12, 2024

mickaelistria Aug 8, 2024

mickaelistria Aug 8, 2024 •

edited

Loading

mickaelistria commented Aug 12, 2024

robstryker commented Aug 13, 2024

mickaelistria commented Aug 13, 2024

robstryker commented Aug 13, 2024

		public JavacTypeBinding getTypeBinding(com.sun.tools.javac.code.Type type) {
		return getTypeBinding(type, false);

	return this.type.isParameterized() && getTypeArguments() != NO_TYPE_ARGUMENTS && this.type.getTypeArguments().stream().anyMatch(TypeVar.class::isInstance);
	return this.type.isParameterized() && this.isDeclaration;

Fixes test0063, test0064, test0065, and more - getQualifiedName expec… #663

Fixes test0063, test0064, test0065, and more - getQualifiedName expec… #663

Conversation

robstryker commented Aug 6, 2024

akurtakov commented Aug 6, 2024

akurtakov commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelistria commented Aug 8, 2024

robstryker commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelistria Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

mickaelistria commented Aug 12, 2024

robstryker commented Aug 13, 2024

mickaelistria commented Aug 13, 2024

robstryker commented Aug 13, 2024

mickaelistria Aug 8, 2024 •

edited

Loading