-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
Conversation
(cherry picked from commit 1e62e4b)
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? |
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 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 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. |
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. |
I'm closing this since I haven't worked on it since my last comment. I'll try to revisit it at some point. |
(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