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

[WIP] fix: Merge ambiguous packages instead of throwing an error #5520

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

WarpspeedSCP
Copy link

@WarpspeedSCP WarpspeedSCP commented Oct 25, 2023

(cherry picked from commit 1e62e4b)

Spoon currently will raise an exception if there are multiple CtPackages with the same name as a CtPackageReference. This can occur when code in two different directories contains the same package name.

Consider large projects such as maven which have a lot of such code; maven has several libraries which all contribute classes to the same package in many cases.

This change attempts to fix the ambiguous package problem by merging all such packages and deleting any leftovers. The first non-empty package is selected as the merge target.

There are no tests; I'd like to be pointed in the right direction regarding this.

fixes: #5475

@WarpspeedSCP WarpspeedSCP changed the title fix: Merge ambiguous packages instead of throwing an error [WIP] fix: Merge ambiguous packages instead of throwing an error Oct 25, 2023
@I-Al-Istannen
Copy link
Collaborator

For testing I would say something along the lines of https://github.com/INRIA/spoon/pull/4139/files#diff-703cba552995d848d774527198c8562dea54acdc8c3657aa49e7a4b4cd39defa.

The problem is, that this is not spec-conform. Junit uses patch-module to tell the JVM to please shut up and accept the beating. Maybe we are at the point where majority rule decided that merging them is a suitable approach though.

I think I would like this to be an environment flag. What do you think @SirYwell @MartinWitt?

@WarpspeedSCP
Copy link
Author

WarpspeedSCP commented Nov 4, 2023

this crops up when one performs analysis by including sources from multiple paths. for example, consider a multimodule project. it has sources for package com.ex.pkg1 at module1/module2 as well as module1/module3.

But when the packages are all compiled and converted to jars, the jvm will happily consider them as if they are in the same jigsaw module if no module info is specified, or if it is all compiled into a fat jar.

if module info is specified, these two sets of classes should end up split across their respective modules anyway. I do wonder if the logic to parse module info and figure out type visibility exists, or if it happens as best effort.

So for example, if you intend to primarily process files of module1/module2, but you also want files of module1/module3 in the classpath so types are resolved, one way would be to add the relevant files from module2 and module3 (with launcher.addInputResource). Then, they will run into this bug when they try to resolve a type that is in a different source root due to the module split. conceptually, they are in the same jigsaw module as far as the developer may be concerned.

We can see this crop up in many big projects (maven, caffeine, and even guava for example), where different classes for the same package, or packages in the same hierarchy, are declared in completely different source roots. So I believe this would be useful for such contexts. The test usecase is definitely also a problem, but I personally think that is a scenario that is worth special casing for the sake of functionality. I was very surprised when I first ran into this spoon exception. It occurred in the caffeine gradle repo, where there are classes of the same package spread across multiple source roots within the same module.

@WarpspeedSCP
Copy link
Author

WarpspeedSCP commented Nov 4, 2023

In my opinion, spoon does not always need to conform to the java spec.

The JVM spec does not seem to explicitly mention how classes are loaded from multiple jar files. However, every IDE, as well as all the JVM implementations I am aware of, are able to correctly pick up classes even if those classes are spread across different source roots.

If the JVM allows this, why should spoon fail to consider files in the same package from a different source root properly?

Note, I am approaching this from the pov of supporting files across build system modules, I am not thinking about the test use case where jigsaw module boundaries are ignored.

Unless I am mistaken, I believe this solution should still be able to account for classes in packages with the same name but in different modules. since the merging occurs only at the package level, there should still be separation across modules.

I will add a test for multiple modules with the same package and classes as well.

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.

A suggestion regarding fixing the ambiguous package problem
3 participants