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

Fix lack of type resolution issue due to lack of imports. #275 #280

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

gayanper
Copy link
Contributor

@gayanper gayanper commented Jul 7, 2023

What it does

The changes will add import statements for found types while traversing symbols in the binary class definition so that the evaluation snippet will properly compiled without needing to have FQN names.

How to test

Unit tests and sample project

Author checklist

@gayanper gayanper force-pushed the fix_generics_binary_eval branch 8 times, most recently from 00b5b08 to 0e82b6b Compare July 9, 2023 08:41
@gayanper gayanper closed this Jul 9, 2023
@gayanper gayanper force-pushed the fix_generics_binary_eval branch from 0e82b6b to fba705b Compare July 9, 2023 09:32
@gayanper gayanper reopened this Jul 9, 2023
@gayanper gayanper force-pushed the fix_generics_binary_eval branch 3 times, most recently from b646719 to 6260d38 Compare July 9, 2023 16:17
@gayanper
Copy link
Contributor Author

@testforstephen will you have time to have a look at this ?

@testforstephen
Copy link
Contributor

this is a valuable contribution, thanks. I'm also interested in this feature #186. I'll find some time to take a look.

@testforstephen
Copy link
Contributor

Hi @gayanper,

The approach you use to dynamically add the import statements for the evaluation snippets looks good. I think we can apply it to the case in the issue #186 as well, where we can resolve the unresolved types and automatically append their fully qualified names in the import statements.

However, I have some questions about the use cases you used in the Java8Tests.java file. Both of the snippets new Observation((Object) subject, (Consumer) action) and ((Subject)this.subject).getName() work fine for me with or without this PR.

Could you provide more use cases that demonstrate the problem you want to fix in this PR? Thank you.

@testforstephen
Copy link
Contributor

Both of the snippets new Observation((Object) subject, (Consumer) action) and ((Subject)this.subject).getName() work fine for me with or without this PR.

It works because I attached debug-lib-sources.jar to debug-lib.jar, where the eval will use SourceBasedSourceGenerator and works fine. Removing the source attachment can reproduce the issue you mentioned.

However, compared to SourceBasedSourceGenerator, the BinaryBasedSourceGenerator still has some limitation. For example, new Observation((Object) subject, (Consumer) action) snippet works fine in the Line 19 breakpoint of Observation for source attached sample, but still not work for binary sample.

@gayanper
Copy link
Contributor Author

However, compared to SourceBasedSourceGenerator, the BinaryBasedSourceGenerator still has some limitation. For example, new Observation((Object) subject, (Consumer) action) snippet works fine in the Line 19 breakpoint of Observation for source attached sample, but still not work for binary sample.

You mean the snippet already in the unit test or something different?

@testforstephen
Copy link
Contributor

I'm using the new unit test added your PR.

@gayanper
Copy link
Contributor Author

@testforstephen do you mean the unit test fail when you execute or it fails when you try run the changes in workspace and try it on a real project debugging scenario?

@testforstephen
Copy link
Contributor

@testforstephen do you mean the unit test fail when you execute or it fails when you try run the changes in workspace and try it on a real project debugging scenario?

The existing test cases you added can pass. My point is that there are other cases not covered well by this PR.

Below is the eval comparison between SourceBasedSourceGenerator and BinaryBasedSourceGenerator.

  • The eval snippet new Observation((Object) subject, (Consumer) action) works fine if the library source jar exists. SourceBasedSourceGenerator is used when the library source is available.
    image

  • The eval snippet new Observation((Object) subject, (Consumer) action) fails if the library source is unavailable. BinaryBasedSourceGenerator is used if the source is unavailable.
    image

Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

LGTM

@gayanper gayanper force-pushed the fix_generics_binary_eval branch from d8d052b to bc94fca Compare September 16, 2023 17:31
The changeset tries to generate missing constructors and add imports
for variable type that are visible in the evaluation __run method.
@gayanper gayanper force-pushed the fix_generics_binary_eval branch from bc94fca to ce1ea14 Compare January 15, 2024 06:12
@gayanper gayanper merged commit 9296bb5 into eclipse-jdt:master Jan 15, 2024
9 checks passed
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