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

using method name to find unqualified Stacktraces in Java Stack Trace #498

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

SougandhS
Copy link
Contributor

@SougandhS SougandhS commented Sep 17, 2024

Using method name to find unqualified Stacktraces
Fix for - #115

What it does

Resolves the ambiguity in results generated when fully qualified class name is not provided in java stack trace console

How to test

Author checklist

@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 3 times, most recently from a9fe1ce to 5e2b141 Compare September 17, 2024 10:49
@SougandhS
Copy link
Contributor Author

Hi @jukzi
Made changes for enhancement you proposed -> 115 .
Could you please review this PR ?

@jukzi
Copy link
Contributor

jukzi commented Sep 17, 2024

generally looks good and has a Junit test :-) i plan to actually test the PR later

@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 4 times, most recently from e249738 to 6ffd199 Compare September 17, 2024 13:40
@SougandhS SougandhS requested a review from jukzi September 17, 2024 14:07
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tested the change with
Project.writeDescription(int) line: 1402 in platform workspace and ended up in exception:

java.lang.ClassCastException: class org.eclipse.jdt.internal.core.BinaryType cannot be cast to class org.eclipse.jdt.internal.core.SourceType (org.eclipse.jdt.internal.core.BinaryType and org.eclipse.jdt.internal.core.SourceType are in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @35f7b857)
	at org.eclipse.jdt.internal.debug.ui.console.JavaStackTraceHyperlink$3.runInUIThread(JavaStackTraceHyperlink.java:215)
	at org.eclipse.ui.progress.UIJob.lambda$0(UIJob.java:148)

@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 2 times, most recently from 13316ba to 2e6f29b Compare September 18, 2024 06:44
@SougandhS SougandhS requested a review from jukzi September 18, 2024 07:13
@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 2 times, most recently from 0615fd3 to 629b071 Compare September 18, 2024 07:34
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no exceptions, but feels like it still has issues to find methods with two parameters. here are some examples:
Realm.runWithDefault(Realm, Runnable) line: 339
PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 173
image
should have found org.eclipse.core.databinding.observable.Realm
image
DirectMethodHandleAccessor.invokeImpl(Object, Object[]) line: 155
image

Method.invoke(Object, Object...) line: 580
image
Main.invokeFramework(String[], URL[]) line: 668

@jukzi
Copy link
Contributor

jukzi commented Sep 18, 2024

And i get StringIndexOutOfBoundsException on parameterized types:
LinkedList<E>(Object).wait() line: 339

java.lang.StringIndexOutOfBoundsException: Range [22, 21) out of bounds for length 38
	at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:55)
	at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:52)
	at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:213)
	at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:210)
	at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:98)
	at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromToIndex(Preconditions.java:112)
	at java.base/jdk.internal.util.Preconditions.checkFromToIndex(Preconditions.java:349)
	at java.base/java.lang.String.checkBoundsBeginEnd(String.java:4865)
	at java.base/java.lang.String.substring(String.java:2834)
	at org.eclipse.jdt.internal.debug.ui.console.JavaStackTraceHyperlink$3.runInUIThread(JavaStackTraceHyperlink.java:211)

@jukzi
Copy link
Contributor

jukzi commented Sep 18, 2024

bonus: Nested Class (with $) are also not found:
AbstractQueuedSynchronizer$ConditionNode.block() line: 519
image

@SougandhS
Copy link
Contributor Author

@jukzi thanks for the additional cases, will make changes and fix the issues.

@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 2 times, most recently from 8442ec6 to eaf8353 Compare September 19, 2024 01:25
@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 7 times, most recently from 37a6ebd to 7958ba4 Compare September 19, 2024 09:14
@SougandhS SougandhS requested a review from jukzi September 19, 2024 09:41
@SougandhS
Copy link
Contributor Author

Hi @jukzi ,
Made changes for handling additional scenarios. (included in the test cases)

  • ignore 3 test case failure as it is from java 23 changes.

@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 2 times, most recently from 2d09bba to 8422b30 Compare September 19, 2024 10:46
@SougandhS SougandhS requested a review from jukzi September 19, 2024 11:12
@jukzi
Copy link
Contributor

jukzi commented Sep 19, 2024

Please figure out why the Build fails InstanceMainMethodsTests - may be related to the Java23 merge @jarthana

@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch 2 times, most recently from 424398c to 3ff7bce Compare September 20, 2024 04:33
@jukzi
Copy link
Contributor

jukzi commented Sep 20, 2024

The tests are also failing on master:
https://ci.eclipse.org/jdt/job/eclipse.jdt.debug-github/job/master/119/
If failing tests are unrelated please create an Issue for it and then we could ignore them for the current PR

@SougandhS
Copy link
Contributor Author

The tests are also failing on master: https://ci.eclipse.org/jdt/job/eclipse.jdt.debug-github/job/master/119/ If failing tests are unrelated please create an Issue for it and then we could ignore them for the current PR

these tests are unrelated to the current changes, will raise issue for those now

@SougandhS
Copy link
Contributor Author

SougandhS commented Sep 22, 2024

@jukzi those 3 tests are now passing for all builds now.
shall we close Issue ?

Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested and it works like a charm. Very good improvement. thanks.
Found two other stacktraces that do not work yet(wrong linenumber jumped to):

ScheduledThreadPoolExecutor(ThreadPoolExecutor).runWorker(ThreadPoolExecutor$Worker) line: 1130	
ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1130

image

Please let me know if you want to fix those too in this PR. Otherwise we can also submit the current state as it already is an improvement as is.

@SougandhS
Copy link
Contributor Author

I have tested and it works like a charm. Very good improvement. thanks. Found two other stacktraces that do not work yet(wrong linenumber jumped to):

ScheduledThreadPoolExecutor(ThreadPoolExecutor).runWorker(ThreadPoolExecutor$Worker) line: 1130	
ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1130

image

Please let me know if you want to fix those too in this PR. Otherwise we can also submit the current state as it already is an improvement as is.

hi @jukzi ,fixed those and added additional tests for that in the latest commit.

@SougandhS SougandhS force-pushed the JavaStackStraceAmbiguity branch from 927cabb to fd0ce3e Compare September 23, 2024 14:29
@SougandhS
Copy link
Contributor Author

@jukzi all commits have squashed into one now. (includes additional case's fix)

@jukzi jukzi merged commit 32fa010 into eclipse-jdt:master Sep 23, 2024
10 checks passed
@jukzi
Copy link
Contributor

jukzi commented Sep 23, 2024

@SougandhS thanks! let's try it in real :-)

@SougandhS
Copy link
Contributor Author

Thank you @jukzi :)

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

Successfully merging this pull request may close these issues.

2 participants