eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
141 stars 107 forks source link

resolution of statically imported and overloaded Assert.assertThat resolves to the method with a different amount of parameters. #2420

Open jurgenvinju opened 3 weeks ago

jurgenvinju commented 3 weeks ago

We are using JDT for all kinds of useful things; amazing piece of work and thanks for the efforts in keeping it up. I'm running into a minor issue method resolution issue here, but it is messing up a call graph construction algorithm for empirical software engineering research purposes, so I'd love some help.

I have reduced a test class in a specific version of JUnit4 to the following:

package org.junit.tests.experimental.results;

import org.hamcrest.CoreMatchers;
import static org.junit.Assert.assertThat;

public class PrintableResultTest {
    public void myTest(String secondExceptionName) {
        assertThat("blabla", CoreMatchers.containsString(secondExceptionName));
    }
}

Both the call and the import of assertThat are resolved to an alternative overload of assertThat with 3 parameters:

public static <T> void assertThat(String reason, T actual, Matcher<? super T> matcher)

The correct methods in the same class is this:

public static <T> void assertThat(T actual, Matcher<? super T> matcher) 

(it defers to the earlier one in its body)

Both running the binding resolution programmatically during an AST visit, and using hover help and reference resolution in either Eclipse or VScode; all four show the same fawlty resolution. So I suspect it's an issue in core and not in UI or other peripheral JDT projects.

If I change the call to: Assert.assertThat("blabla", CoreMatchers.containsString(secondExceptionName)); and remove the static import, then the right method is resolved with the two parameters. So it seems the interaction between a static import and a call to that statically imported method is essential for this issue to occur.

I ran this programmatically with different source compatibility levels, JLS 8 through JLS 13, with an without preview. The behavior is consistent.

Please let me know if you need more information. I'd be happy to dig in a little more. I've been stepping through the resolution code, but the path goes blind when new ASTs are converted to old ASTs, and a cache lookup suddenly produces an already wrongly resolved binding.

Stepping through the parser, with earlier breakpoints at AST node construction time, it seems at least at first the right number of arguments are considered during resolution. Yet the AST with 2 argument expressions ends up with a resolved IMethodBinding to a method with 3 formal parameters. Somewhere in between the issue is happening, but I'm not enough introduced to the code of the core JDT to see where it is at.

jurgenvinju commented 3 weeks ago

Read somewhere that junit.Assert is treated as the native Assert by JDT for convenience, but this special-casing does not seem to influence this issue.

iloveeclipse commented 3 weeks ago

@mickaelistria : as an AST expert, probably something for you?

srikanth-sankaran commented 3 weeks ago

Haven't looked into it yet but this looks like a problem in overload resolution from the description and not a DOM/AST issue.

mickaelistria commented 3 weeks ago

So the MethodBinding you receive from resolveBinding is incorrect (you can see it references the other method)? If so, can you please check whether its nested binding field, which reference the compiler binding, is correct or not?

jurgenvinju commented 3 weeks ago

Looks like I got myself in a very specific corner here. What do you think? Is this worth exploring or is this too much of a niche situation?

srikanth-sankaran commented 3 weeks ago

Looks like I got myself in a very specific corner here. What do you think? Is this worth exploring or is this too much of a niche situation?

I'll take a look, though may not happen right away.