[JEP 511] Module Import Declarations#4910
Conversation
johannescoetzee
force-pushed
the
johannes/module-imports
branch
from
4ea1b40 to
9bf84ba
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4910 +/- ##
===========================================
Coverage 58.417% 58.417%
Complexity 2559 2559
===========================================
Files 689 689
Lines 39526 39526
Branches 7168 7168
===========================================
Hits 23090 23090
Misses 13499 13499
Partials 2937 2937
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| printer.print("static "); | ||
| } | ||
| if (n.isModule()) { | ||
| printer.print("static "); |
There was a problem hiding this comment.
Is the PR finished? If so, as soon as you have fixed this small issue, I will validate this PR. Thank you for your contribution.
There was a problem hiding this comment.
Oops, that was a copy-paste issue. I would've thought that the tests I added would catch this, but apparently this visitor isn't being tested correctly. Tomorrow morning I'll update the test to ensure this is actually covered and fix this typo, but after that this PR is finished if you're satisfied with the rest
There was a problem hiding this comment.
but after that this PR is finished if you're satisfied with the rest
The solution of creating a new method for resolving types does not satisfy me intellectually, but I have no other solution to propose that would be as effective.
There was a problem hiding this comment.
I've fixed the static/module issue. I saw this is in the deprecated PrettyPrintVisitor and it looks like this visitor either isn't being tested at all, or only in few specific testcases. Most testcases use the DefaultPrettyPrinter instead. If we want to keep the PrettyPrintVisitor, ideally we'd refactor it so that it just uses the DefaultPrettyPrinter methods, removing the need to maintain two largely identical implementations. I haven't looked into the configuration options enough to get an idea of how hard porting the configuring aspect will be, but the printing methods themselves should be simple. Regardless, I think refactoring the printer and fixing the testing situation is probably best handled separately.
There was a problem hiding this comment.
It may be time to do some cleaning up and remove this part of the code. Could you create an issue to address this topic? However, I would prefer to postpone resolving this new issue until later, as I do not want to include it in the next version of JavaParser to avoid too much impact on users' code.
johannescoetzee
force-pushed
the
johannes/module-imports
branch
from
9bf84ba to
2dc4812
Compare
jlerbsc
added
the
PR: Added
|
Thank you for this contribution. It is very readable and understandable. |
2 participants
I tried to re-open #4905, but since I force-pushed changes to restructure the commits as requested this option was no longer available.
Fixes #4848
This PR adds parser support for module import statements, as well as support for resolving types from modules.
Changes to ImportDeclaration/CompilationUnit
Constructors and an
addImportmethod with an option to specify whether this is a module import or not (indicated by the value of the newisModulefield in ImportDeclaration) were added. The old versions of these constructors and this method without theisModuleparameter were kept to avoid breaking user code.JarTypeSolver
Javassist does not provide much support for handling modules, so I ended up parsing the module attribute manually. This is a bit cumbersome, but does work and I hope the comments explain what is happening sufficiently well.
ClassLoaderTypeSolver
I couldn't figure out a way to do this without using Java 9+ types and methods, so I used a lot of reflection for the implementation to maintain compatibility with Java 8 (although this will always fail if JavaParser is run with Java 8). I also couldn't figure out a good way to get to a list of modules loaded by a classloader (the mechanisms I could find that looked promising were all JDK-internal or blocked for security reasons). The solution as it is presented here requires users to provide a list of module layers that the typesolver will use to find modules. This is done automatically for the ReflectionTypeSolver.