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

Wrong method redirect #1278

Closed
andreasdc opened this issue Aug 1, 2023 · 9 comments · Fixed by #1349
Closed

Wrong method redirect #1278

andreasdc opened this issue Aug 1, 2023 · 9 comments · Fixed by #1349
Assignees
Labels
regression Something was broken by a previous change
Milestone

Comments

@andreasdc
Copy link

When you have methods with the same name, but handles different objects in the argument it redirects you to wrong method when you click f3. However when you select the whole name, not just click on it, you are being redirected to the correct method.

@jjohnstn
Copy link
Contributor

jjohnstn commented Aug 2, 2023

Using Eclipse 2023-06 I am unable to reproduce with the following simple source in the Java editor:

public class TestSelect {
	
	public void foo(String s) {
		
	}
	
	public void foo(Integer i) {
		
	}
	
	public void foo(Double d) {
		
	}
	
	public void foo2(Integer i) {
		foo(i);
	}

}

When I click on the foo(i) call, F3 takes me to the middle foo(Integer i) method. Are you able to recreate with this test case and if not is there a test you can post here?

@andreasdc
Copy link
Author

It's the same, but even when I select whole method's name it redirects to the wrong method.

Double test = 1d; if (test instanceof Double test2) { TestSelect.foo(test2); }

`public class TestSelect {

public static void foo(String s) {
	
}

public static void foo(Integer i) {
	
}

public static void foo(Double d) {
	
}

public static void foo2(Integer i) {
	foo(i);
}

`
When you click f3 it redirects you to the method with string argument, probably just because it's the first one.

@jjohnstn
Copy link
Contributor

jjohnstn commented Aug 8, 2023

Hi @andreasdc Thanks for the sample. I can now reproduce it. It also fails for source hover.

@jjohnstn jjohnstn transferred this issue from eclipse-jdt/eclipse.jdt.ui Aug 8, 2023
@jjohnstn jjohnstn added the regression Something was broken by a previous change label Aug 8, 2023
@jjohnstn
Copy link
Contributor

jjohnstn commented Aug 8, 2023

Transferring to jdt.core. New test case:

public class TestSelect {

	public void foo(String s) {

	}

	public void foo(Integer i) {

	}

	public void foo(Double d) {

	}

	public void foo2(Integer i) {
		Object test = 1d;
		if (test instanceof Double test2) {
			foo(test2); // select foo and use F3
		}
	}

}

Selecting the foo call in foo2 above and hitting F3 ends up jumping to the foo(String) method. Hovering also shows the foo(String) method.

I followed the Open Action to the org.eclipse.jdt.internal.codeassist.SelectionEngine.select() method: line 1086 which calls parsedUnit.resolve(); and ends up throwing a SelectionNodeFoundException from org.eclipse.jdt.internal.codeassist.select.SelectionOnMessageSend.resolveType().

Looking at the parent org.eclipse.jdt.internal.compiler.ast.Block for the if block in the debugger, I see:

{
  <SelectOnMessageSend:foo(test2)>;
  Double test2;
}

I am unfamiliar with this code, but wonder why the Double test2; statement doesn't precede the SelectOnMessageSend in the block.

I tried the same test on Eclipse 2022-03 and the test works correctly (F3 goes to foo(Double)). It fails on 2022-06 so some regression between those two releases. The initial sample from @andreasdc had test declared as Double. On 2022-03 and 2022-06 that causes an error marker stating that expression type cannot be sub-type of Pattern type. This error does not get shown in latest M2.

@iloveeclipse
Copy link
Member

See also #1288

@srikanth-sankaran
Copy link
Contributor

The PR #1349 fixes this problem and includes a regression test from here. So I will close this as a duplicate

@srikanth-sankaran
Copy link
Contributor

Duplicate of #1195

@srikanth-sankaran srikanth-sankaran marked this as a duplicate of #1195 Sep 12, 2023
@srikanth-sankaran srikanth-sankaran closed this as not planned Won't fix, can't repro, duplicate, stale Sep 12, 2023
@andreasdc
Copy link
Author

Are you sure it's a duplicate?

@srikanth-sankaran
Copy link
Contributor

Are you sure it's a duplicate?

Yes, It is not a duplicate in the sense of the symptoms being the same, but the underlying problem being the same - that being incorrect recovery of the parse tree in the presence of pattern matching constructs.

As documented here: #1278 (comment), the recovered parse tree is

{
  <SelectOnMessageSend:foo(test2)>;
  Double test2;
}

which is wrong, the pattern binding variable's declaration should precede its use, not follow.

#1349 is a substantial reimplementation of pattern matching support in code selection engine and subsumes the fix for the current problem and includes a regression test from here.

So breathe easy :)

srikanth-sankaran added a commit that referenced this issue Sep 14, 2023
This is a substantial reimplementation of the code selection support for pattern matching constructs. By using auxiliary stacks to record the state of the parser and by using that state to drive the bottom up context recovery and parse tree construction, we now rebuild the parse tree to sufficient detail to ascertain liveness of pattern binding variables at the point of selection.

Fixes #1195
Fixes #769
Fixes #1263
Fixes #1360
Fixes #1364
Fixes #1278
Fixes #1288

Verifies https://bugs.eclipse.org/bugs/show_bug.cgi?id=573257
Verifies https://bugs.eclipse.org/bugs/show_bug.cgi?id=572975
Verifies https://bugs.eclipse.org/bugs/show_bug.cgi?id=576794
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Something was broken by a previous change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants