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: Search for packages in all available Java modules #3771

Merged

Conversation

slarse
Copy link
Collaborator

@slarse slarse commented Jan 27, 2021

Fix #3769

The issue describes the symptoms and the root cause of them. The tl;dr is that PackageFactory.get(String) only searches for packages in the unnamed module, causing type resolution in named modules to not work correctly. This PR fixes that by searching for packages in all modules.

As noted in #3769, there's probably a whole lot of code that assumes that there's only the unnamed module, and therefore use CtModel.getRootPackage() (which returns the root package of the unnamed module) as the definitive root package, which as of Java 9 is not appropriate.

@slarse slarse changed the title wip: fix: Fix type resolution in named modules wip: fix: Search for packages in all available modules Jan 27, 2021
@slarse
Copy link
Collaborator Author

slarse commented Jan 27, 2021

This is very odd. If I now run all tests locally with IntelliJ IDEAs test runner, I get a test failure on LogTest.NonParameterizedTest.testMavenLauncherLogs().

Expected :Running in FULLCLASSPATH mode. Source folders and dependencies are inferred from the pom.xml file (doc: http://spoon.gforge.inria.fr/launcher.html).
Actual   :Running in FULLCLASSPATH mode. Classpath is manually set (doc: http://spoon.gforge.inria.fr/launcher.html).

If I run that test individually, it does not fail. If I run all tests with Maven, there are no failures either. Similarly, there's no failure in CI. Something odd is going on with that test, and I can't see how it's related to the changes I just performed.

EDIT: This appears to be a problem with IntelliJ IDEA's test runner, in combination with use of the nested test classes in LogTest. IntelliJ executes the tests twice, seemingly with some lingering state, causing failures. Most likely adding a new test case caused some unfortunate reshuffling of state. It's not an issue related to this PR so I'll ignore the local failure.

@slarse slarse changed the title wip: fix: Search for packages in all available modules review: fix: Search for packages in all available modules Jan 27, 2021
@slarse
Copy link
Collaborator Author

slarse commented Jan 27, 2021

Ready for review.

@monperrus
Copy link
Collaborator

This is a serious bug! I'm not that surprised given the relatively low usage of modules in the field.

The fix LGTM, thanks @slarse !

@slarse
Copy link
Collaborator Author

slarse commented Jan 28, 2021

This is a serious bug! I'm not that surprised given the relatively low usage of modules in the field.

Indeed! I'm hoping this solves the most severe issues we're facing in Sorald. But I have a nagging feeling this isn't the last module-related bug we'll see. It's just such a significant metamodel shift from the pre-Java 8 times.

@slarse
Copy link
Collaborator Author

slarse commented Jan 29, 2021

@monperrus Think this could be merged today? It's a blocker for handling Java 9+ projects with Sorald.

@monperrus monperrus changed the title review: fix: Search for packages in all available modules fix: Search for packages in all available Java modules Feb 2, 2021
@monperrus monperrus merged commit 6e8c5b8 into INRIA:master Feb 2, 2021
@monperrus
Copy link
Collaborator

Very important fix wrt modules, thanks a lot @slarse

@slarse slarse deleted the issue/3769-fix-type-resolution-in-named-modules branch May 28, 2021 10:08
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.

bug: Getting the type declarations from type references doesn't work for types in named modules
2 participants