fix: correct package assumption when there is a star import #3338
Conversation
This is too restrictive, and counts e.g. java.util.* as an offending import, even though it's fine
| ref.setPackage(jdtTreeBuilder.getContextBuilder().compilationUnitSpoon.getDeclaredPackage().getReference()); | ||
| ContextBuilder ctx = jdtTreeBuilder.getContextBuilder(); | ||
| boolean isNoClasspath = jdtTreeBuilder.getFactory().getEnvironment().getNoClasspath(); | ||
| if (isNoClasspath && containsStarImport(ctx.compilationunitdeclaration.imports)) { |
There was a problem hiding this comment.
I'm not sure we need to check if it's noclasspath mode, we shouldn't get here in classpath mode? Perhaps assert isNoClasspath instead?
There was a problem hiding this comment.
Perhaps assert isNoClasspath instead?
good idea.
| } | ||
| } else { | ||
| throw new AssertionError("unexpected declaring type: " + declaring.getClass() + " of " + declaring); | ||
| } | ||
| } | ||
|
|
||
| private static boolean containsStarImport(ImportReference[] imports) { | ||
| return imports != null && Arrays.stream(imports).anyMatch(imp -> imp.toString().endsWith("*")); |
There was a problem hiding this comment.
This will return true if there's any star import, which is too restrictive. For example, if there's an import java.util.*, then this will return true and we won't assign the CU's package to the type. But if the star import is actually available, which should be the case for stdlib packages, depending on JDK version, then it should be fine to guess assign the CU's package to the type.
Is there any way to check which imported packages are actually available at this stage?
There was a problem hiding this comment.
Is there any way to check which imported packages are actually available at this stage?
Do you mean which subpackages under .**are available in the classpath?
There was a problem hiding this comment.
No, I was rather thinking of just checking if the containing package is available. For example, if any type from java.util is available on the classpath, then we conclude that all types from java.util.* are available on the classpath. If that is the case, we assume that had SomeClass actually belonged to java.util, it would too have been on the classpath too, and since it is not we assign it to the compilation unit's package.
But as I typed the above paragraph I realized a few things. First, third party packages are often split across projects leading to packages possibly being partially available, so the assumption "all types from the x.y.* import are available if any type from package x.y is available" is not necessarily true (think e.g. org.apache). Second, differing versions of the standard library may cause issues. Third, accounting for what I just pointed out makes this incredibly complex.
I think the simple and probably sufficient solution is to simply not guess the package if the type is not on the classpath, and there is any * import.
We should never get here in classpath mode
|
@monperrus I'm not sure what the cleanest way to resolve this is. The current state of the PR is to just create a new, empty package reference whenever there is a star import. But that coincides with how unnamed packages are implemented, which is not what this is. it's rather an unknown package. While it works to have unknown packages simply be an unnamed package, it seems like there should be a distinction between an unnamed package and an unknown package, but I'm not sure of the best way to do that. Then again, as this is an isolated problem in noclasspath mode when there also happens to exist star imports in the file, simply having the unknown package default to an unnamed package is acceptable if it's properly documented. |
|
To me, it's acceptable this way. I generally prefer not to over-engineer for corner-cases. If you unWIP, and nobody is against it, I'll merge. |
slarse
changed the title
slarse
changed the title
|
Done. |
|
Not sure why the build failed, should I try to merge master into this branch? |
monperrus
changed the title
|
The CI error is not related. So we merge, thanks a lot for your contribution. |
|
FYI, CI fix at #3356 |
Fix #3337
Hello!
This is an attempt to fix Spoon incorrectly guessing the package of a type in noclasspath mode. See #3337 for details.
I need some help with this PR, see the comments.