fix: Use correct target for statically imported method calls #4991

I-Al-Istannen · 2022-10-30T21:46:20Z

This PR tries to teach spoon that static imports aren't evil and java.lang imports can not be removed in these cases.

This required adjusting a few tests, as they expected spoon to fully-qualify static imports which it now no longer does.
EDIT: This is actually be an interesting question: Should the DJPP remove static imports and fully qualify them? This accidentally worked before for some cases but I now fixed the model representation and it no longer seems to be done by the DJPP.

One of the tests (testNoFQNWhenUsedInInnerClassAndShadowedByLocalVariable) seems to guard against:

Object spoon = null;
spoon.util.EmptyList list = null; // qualifying it here is wrong!

but I don't really see how it would test that as the call needs no qualification at all. I am not sure what exactly to do about that to be quite honest.

Closes #4981.

…tains

MartinWitt · 2022-10-31T14:36:36Z

Nice, going to have a look later.

MartinWitt · 2022-10-31T18:33:58Z

This is actually be an interesting question: Should the DJPP remove static imports and fully qualify them? This accidentally worked before for some cases but I now fixed the model representation and it no longer seems to be done by the DJPP.

Kind of interesting. As a user, if a call the DJPP with fully qualify, I wouldn't expect static imports or? So the DJPP should remove them and fully qualify them iff the user uses the fully qualify mode.
Otherwise LGTM.

…file contains" This reverts commit 4506f9a.

…or now" This reverts commit 0bf6526.

…ged it

…mplicit

I am not quite sure what I want here to be perfectly honest. You don't need to import the methods so the trivial name suffices, but you might still want to fully qualify them?

Previously it decided that a field was not imported even though a star import covering it existed.

This ensures that no imports are added for qualified references.

I-Al-Istannen · 2022-11-01T11:25:33Z

Okay, there are a few new exiting things:

The TargetedExpressionTest expected unqualified names for methods that can be used without being imported as they are declared in sibling classes. With this PR, it qualifies them. Is that desired?
The ImportTest expected a statically imported constant to be qualified when printing with autoImports = true. This is now no longer the case with this PR.
The current test failure is an element mismatch by the scanner as we now model targets of method calls correctly.

I would like some input on 1. and maybe 2. if possible.

The larger-scale changes in this PR are:

The target of statically imported methods and fields is now set to their actual type instead of this. JDT models them as having a this reference as target, but that seems a bit nonsensical and breaks the printer.

The type of the target of statically imported fields is now (heuristically) marked as implicit:

import static foo.bar.Constants.CONSTANT;
int i =                   CONSTANT.foo; // actual source code
int i = foo.bar.Constants.CONSTANT.foo; // AST including implicit elements
                                   ^^^
                                   explicit field read
                          ^^^^^^^^^^^^
                          explicit field read
        ^^^^^^^^^^^^^^^^^^
        implicit type

Also CC @algomaster99 as you unconditionally added CtFieldRead imports in 4f44d9f and I am not sure I broke something there (See here for my changes). Judging by the issue trail you had a lot of fun you might not want to experience again :^) I don't want to offload the work for testing that on you, so a general pointer with if this breaks violently I will find out where you live would be fine.

Import handling in spoon contains horrors beyond my imagination and I am 100% sure I missed critical pieces buried along the way. I looked at the issue (#4981) and thought it would be easy — now I have dug a hole too deep to silently exit…

algomaster99 · 2022-11-02T11:06:34Z

The TargetedExpressionTest expected unqualified names for methods that can be used without being imported as they are declared in sibling classes. With this PR, it qualifies them. Is that desired?

The assertion is checked using toString method which calls the DJPP. If FULLY_QUALIFIED is not explicitly set, then I think we should just print the method name.

The ImportTest expected a statically imported constant to be qualified when printing with autoImports = true. This is now no longer the case with this PR.

This seems fine to me. autoImport=true refers to PRETTY_PRINTING_MODE being set to AUTOIMPORT, right?

so a general pointer with if this breaks violently I will find out where you live would be fine.

It is fine. We will see if it breaks something. Patching the pretty-printer is easy, but making sure that the patch is the best solution is a hard problem. And I live in Sweden, it won't be hard for you to find where I live xD

Import handling in spoon contains horrors beyond my imagination and I am 100% sure I missed critical pieces buried along the way. I looked at the issue (#4981) and thought it would be easy — now I have dug a hole too deep to silently exit…

I can understand. I feel that way about pretty printing in general. I used to open PRs about fixing issues, but then they just remain drafts for too long 😞

I-Al-Istannen · 2022-11-02T12:09:56Z

And I live in Sweden, it won't be hard for you to find where I live xD

That was meant the other way around :P

This seems fine to me. autoImport=true refers to PRETTY_PRINTING_MODE being set to AUTOIMPORT, right?

Yes, that's what I meant. Good :)

The assertion is checked using toString method which calls the DJPP. If FULLY_QUALIFIED is not explicitly set, then I think we should just print the method name.

The problem is that toString creates a pretty printer using the default factory (createPrettyPrinter) and that one defaults to FULLY_QUALIFIED. There doesn't seem to be anything used besides FULLY_QUALIFIED and AUTOIMPORT.

Patching the pretty-printer is easy, but making sure that the patch is the best solution is a hard problem.

Oh yea… Figuring out why things break, why the AST doesn't make any sense and why random checks in random places exist in their exact form — and once you've got that, good luck figuring out which to adjust :P I am fairly certain that fixing the AST and making the implicit type implicit is a good idea.

I can understand. I feel that way about pretty printing in general. I used to open PRs about fixing issues, but then they just remain drafts for too long 😞

🫂 Spoon has a few of such areas…

algomaster99 · 2022-11-02T13:39:13Z

that one defaults to FULLY_QUALIFIED

Oh, then I think it would be desired. It could be a breaking change though 😬

I-Al-Istannen · 2022-11-04T14:39:32Z

It could be a breaking change though

Oh god, it probably is. I wonder whether that is acceptable here as it arguably fixes an inconsistency. I will await judgment from the other maintainers

I-Al-Istannen · 2022-11-04T16:02:53Z

This currently produces incorrect code for references to non-static methods of the enclosing class. I have opened eclipse-jdt/eclipse.jdt.core#515 as I don't really see a nice way to correctly detect this in Spoon.

I-Al-Istannen · 2022-11-16T21:43:03Z

Okay, as JDT doesn't really want to touch this I now fix it in Spoon (I think). I believe it makes sense for us to silently invent new implicit elements to properly attribute the implicitly-qualified method calls.

I am not sure how nice reviewing this PR is in its current state. It changes quite a few things in a few places, but the import cleaner changes don't do much without the implicit this changes.

MartinWitt · 2022-11-20T09:15:17Z

src/test/java/spoon/reflect/visitor/CtScannerTest.java

+		assertEquals(3631, counter.nElement + countOfCommentsInCompilationUnits);
+		assertEquals(2423, counter.nEnter + countOfCommentsInCompilationUnits);
+		assertEquals(2423, counter.nExit + countOfCommentsInCompilationUnits);


Just as a comment, I have no clue why we have a test case where we simply change numbers only for the CI :D

Yea, this one is a bit weird. Maybe a Map<Class<? extends CtElement> , int> would be better? Then you can see which part of the AST you changed and whether that was intentional?

src/main/java/spoon/support/compiler/jdt/JDTTreeBuilderHelper.java

MartinWitt · 2022-11-20T09:18:08Z

Just one comment missing other LGTM.

MartinWitt · 2022-11-24T18:54:48Z

Thanks @I-Al-Istannen

I-Al-Istannen added 5 commits October 30, 2022 14:46

fix: Use correct target for statically imported method calls

67da114

fix: Keep explicit static imports for classes in java.lang package

bc84ac6

test: Add test validating no static imports are removed

b782f5f

fix: Teach a few tests to respect the static import the test file con…

4506f9a

…tains

hack: I doubt this test still does what it says, placate it for now

0bf6526

I-Al-Istannen changed the title ~~wip: fix: Use correct target for statically imported method calls~~ review: fix: Use correct target for statically imported method calls Oct 30, 2022

I-Al-Istannen added 8 commits October 31, 2022 20:22

Revert "fix: Teach a few tests to respect the static import the test …

e809f79

…file contains" This reverts commit 4506f9a.

Revert "hack: I doubt this test still does what it says, placate it f…

d78a309

…or now" This reverts commit 0bf6526.

Do not overwrite the target if setTargetFromStaticImport already chan…

0cc064f

…ged it

Heuristically detect unqualified field accesses and make their type i…

87af714

…mplicit

Adjust test that expected printing with autoImport to be fully qualified

8fa4dd4

Make test expect fully qualified method references

3b2e94e

I am not quite sure what I want here to be perfectly honest. You don't need to import the methods so the trivial name suffices, but you might still want to fully qualify them?

Teach import cleaner to look through wildcard static import

0f7b7ea

Previously it decided that a field was not imported even though a star import covering it existed.

Only add imports for fields if their target type is implicit

71034d7

This ensures that no imports are added for qualified references.

MartinWitt requested a review from SirYwell November 2, 2022 20:19

I-Al-Istannen changed the title ~~review: fix: Use correct target for statically imported method calls~~ wip: fix: Use correct target for statically imported method calls Nov 4, 2022

Emulate outer this access

5800b52

I-Al-Istannen force-pushed the fix/static-imports branch from 534beed to 5800b52 Compare November 16, 2022 21:33

I-Al-Istannen changed the title ~~wip: fix: Use correct target for statically imported method calls~~ review: fix: Use correct target for statically imported method calls Nov 16, 2022

I-Al-Istannen requested a review from MartinWitt November 17, 2022 20:44

MartinWitt reviewed Nov 20, 2022

View reviewed changes

src/main/java/spoon/support/compiler/jdt/JDTTreeBuilderHelper.java Outdated Show resolved Hide resolved

MartinWitt previously approved these changes Nov 20, 2022

View reviewed changes

Clarify "tokens.length == 2" check

ad15225

I-Al-Istannen dismissed MartinWitt’s stale review via ad15225 November 20, 2022 10:04

Merge branch 'master' into fix/static-imports

f9fc42f

MartinWitt approved these changes Nov 24, 2022

View reviewed changes

MartinWitt changed the title ~~review: fix: Use correct target for statically imported method calls~~ fix: Use correct target for statically imported method calls Nov 24, 2022

MartinWitt merged commit 8ba20e9 into INRIA:master Nov 24, 2022

monperrus mentioned this pull request Mar 9, 2023

release Spoon 10.3.0 #5135

Closed

I-Al-Istannen deleted the fix/static-imports branch October 26, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use correct target for statically imported method calls #4991

fix: Use correct target for statically imported method calls #4991

I-Al-Istannen commented Oct 30, 2022 •

edited

Loading

MartinWitt commented Oct 31, 2022

MartinWitt commented Oct 31, 2022

I-Al-Istannen commented Nov 1, 2022

algomaster99 commented Nov 2, 2022

I-Al-Istannen commented Nov 2, 2022

algomaster99 commented Nov 2, 2022

I-Al-Istannen commented Nov 4, 2022

I-Al-Istannen commented Nov 4, 2022

I-Al-Istannen commented Nov 16, 2022

MartinWitt Nov 20, 2022

I-Al-Istannen Nov 20, 2022

MartinWitt commented Nov 20, 2022

MartinWitt commented Nov 24, 2022

fix: Use correct target for statically imported method calls #4991

fix: Use correct target for statically imported method calls #4991

Conversation

I-Al-Istannen commented Oct 30, 2022 • edited Loading

MartinWitt commented Oct 31, 2022

MartinWitt commented Oct 31, 2022

I-Al-Istannen commented Nov 1, 2022

algomaster99 commented Nov 2, 2022

I-Al-Istannen commented Nov 2, 2022

algomaster99 commented Nov 2, 2022

I-Al-Istannen commented Nov 4, 2022

I-Al-Istannen commented Nov 4, 2022

I-Al-Istannen commented Nov 16, 2022

MartinWitt Nov 20, 2022

Choose a reason for hiding this comment

I-Al-Istannen Nov 20, 2022

Choose a reason for hiding this comment

MartinWitt commented Nov 20, 2022

MartinWitt commented Nov 24, 2022

I-Al-Istannen commented Oct 30, 2022 •

edited

Loading