review: feature: Implementation for Jigsaw modules(Java 9) with support for shadow modules #4751
Conversation
| @@ -289,4 +290,14 @@ public Collection<CtExecutableReference<?>> getAllExecutables() { | |||
| } | |||
| return l; | |||
| } | |||
|
|
|||
| // FIXME: 16/06/2022 This is a workaround | |||
|
|
||
| // FIXME: 16/06/2022 This is a workaround | ||
| @Override | ||
| public CtRole getRoleInParent() { |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private static class Modules extends ElementNameMap<CtModule>{ | ||
| @Override | ||
| protected CtElement getOwner() { | ||
| return null; |
Check notice
Code scanning
Return of 'null'
|
|
||
| private CtModuleRequirement createRequires(ModuleDescriptor.Requires instruction) { | ||
| CtModuleReference requiredModule = factory.Module().createReference(instruction.name()); | ||
| Set<CtModuleRequirement.RequiresModifier> modifiers = new HashSet<>(); |
Check notice
Code scanning
'Set' can be replaced with 'EnumSet'
| context.haveToSearchForSubtypes = false; | ||
| //there are some new super types, whose sub inheritance hierarchy has to be checked | ||
| //search their inheritance hierarchy for sub types | ||
| subHierarchyFnc.forEachSubTypeInPackage(new CtConsumer<CtType<?>>() { |
Check notice
Code scanning
Anonymous type can be replaced with method reference
|
I took a quick glance at your pull request. We try to create small PRs that only fix the issue and do not include unrelated changes at spoon. Your code style changes are fine, but they should not be included in this PR. Can you change this so that only changes relevant to the bug fix are included? |
Sure, no problem. It was accidental as intellij automatically styles the code I write in that way. Also can you help me with the issue i mentioned? It should be quite easy to fix but I haven't had time to look into it these last days |
|
flacoco flags 5 suspicious lines as the potential root cause of the test failure. The line 92 of the file src/test/java/spoon/LauncherTest.java has been identified with a suspiciousness value of 92.43%. The line 93 of the file src/test/java/spoon/LauncherTest.java has been identified with a suspiciousness value of 92.43%. The line 98 of the file src/test/java/spoon/LauncherTest.java has been identified with a suspiciousness value of 92.43%. The line 11 of the file src/main/java/spoon/support/reflect/declaration/CtUnnamedModuleImpl.java has been identified with a suspiciousness value of 89.65%. The line 12 of the file src/main/java/spoon/support/reflect/declaration/CtUnnamedModuleImpl.java has been identified with a suspiciousness value of 89.65%. |
|
Can you point me to a test case which produces your problem? |
I've fixed them myself just now. Is there a way though to automatically generate ModelRoleHandlers or should I implement them manually? |
|
It should be ready for review. The only thing that I don't really like is how I check if a module was attributed in JavaReflectionTreeBuilder, but having a property only in the implementation seems to break some tests. If a maintainer can check that, it would be great. |
Auties00
changed the title
|
Your pull request is still really noise and has unrelated changes like the conversion of imports to package imports. Could you clean it up a bit? This helps a lot in the review process. |
I've fixed all of the styling problems. Sorry, but I didn't actually notice that my IDE was continuing to apply my default styling also here. Now it should be fixed. For the checkstyle errors I've tried to run qodana locally but I cannot seem to make it analyze only a particular commit or branch |
You probably want to run |
Yeah that's probably it. I'm currently on vacation, I'll do it asap |
Using a small workaround for getRoleInParent as the implementation is not clear to me(marked both with // FIXME: 16/06/2022), obviously it breaks a whole set of other tests Master branch tests fail: 42 This branch fails: 81
Fixed formatting
All tests should pass
Auties00
force-pushed
the
jigsaw_modules
branch
2 times, most recently
from
3d4d849
to
b962a40
Compare
# Conflicts: # src/main/java/spoon/reflect/meta/impl/ModelRoleHandlers.java # src/main/java/spoon/support/reflect/declaration/CtPackageImpl.java
|
It's becoming a challenge to make this ready :/ |
That is sadly a Github setting we (AFAIK) won't get around until you've merged a PR. About your PRFirst up, thanks for your changes! I hope the rest is not too discouraging :) I was looking at it here and there, but your PR changes multiple different and unrelated things at once. This makes reviewing it quite a chore. You are at least:
And maybe more I missed. While some of them certainly could be bundled together, it currently does way too many things IMHO. I am not smart enough to keep the whole thing paged in main memory and don't have the time to detangle all this into different changesets to properly understand what and why you changed things. My humble requestI can't speak for anybody else on the project and especially not the integrators, but the PR in its current form is a bit of a hard pill to swallow. I'd greatly appreciate if you could split it along at least a few of the points I outlined above (or, even better: You find your own independent changes, as you are probably better at it than I am). Merging some of the smaller ones would also allow you to run workflows, as a nice side effect. |
I'm not changing those, I was working out the merge conflicts between my upstream and the main one as it looks like there were some changes in previous commits that I didn't merge correctly the first time around. It should be fixed in the latest commit, sorry about that.
This PR adds support for shadow modules so it makes sense to make every component module aware while the opportunity is there in my opinion. Previously modules were supported, but you couldn't really do anything useful with them because of the lack of module awareness and shadow support. Talking about backwards compatibility, everything should continue to work and nothing should break except that the parent of a named module is no longer the unnamed module as I explained in the introduction, but no element as they are by definition standalone containers. In practice, this means that old code which checks the parent of a module and expects the unnamed will not work. Though, this is practically a non-issue as previously only the unnamed module had any presence in the various components of the AST so I don't see who is going to check if the parent of the unnamed module is the unnamed module(which doesn't even make sense logically but that's beside the point). If it's necessary to keep this condition in, I can definitely revert it, but it breaks quite clearly the JLS which is an issue that someday will need to be addressed in my opinion. I think that it's better to do it now, before shadow modules are introduced as doing it later may actually break some code.
I added those methods for consistency with the model overall and as some tests even depend on the existence of getters and setters for public fields of the meta-model, unless marked explicitly with @Unsettable for the latter.
The changes in CtElementImpl are actually related. Considering that the parent of a module is null, which was previously not true as explained in the previous points, the get parent and get factory methods need to take that into account.
I've changed how packages are handled internally to make packages module aware while not sacrificing performance, so I'd say that it's relevant. Btw it doesn't break anything and the implementation is practically identical(no checks have been changed).
This is related to the previous point. A module doesn't have an owner which needs to be taken into account. I can definitely add a comment btw
Practically nothing has changed there except for the fact that modules are also analyzed
If it's necessary, I can divide the PR into a rework of the module system and package system for better module awareness and the addition of shadow modules, though in practice practically nothing will change as the changes will still need to be reviewed. If you want I can add a commit message explaining all the changes file per file which should make it very easy to review which I think is better. Though I'm open to accepting any option that is preferable for the project EDIT: I've reviewed all of my code to remove any unnecessary change and updated this PR to contain all changes |
|
Oh well looks like there is a single test failing right now. I'll look into it |
# Conflicts: # src/main/java/spoon/support/util/internal/ElementNameMap.java # src/test/java/spoon/reflect/ast/AstCheckerTest.java
Fixed last test hopefully
|
Ready for review. @I-Al-Istannen sorry for the tag, but can you please run the CI? |
|
Fixed the checkstyle |
|
For the maintainers: should I fix the Javadoc? I really didn't change anything there. The PR should be ready as it passes all integration tests apart from that |
|
I have no context on this whole topic so I can't make specific comments on the code, but I can answer some of the outstanding questions.
You're failing a check that's part of our ongoing effort to improve upon Spoon's API documentation (see #3923). Specifically, you've added public methods without documenting e.g. type and method parameters. A script checks before/after and fails if the amount of Checkstyle violations for Javadoc increases. You can run the script locally, run
Yes, I would say it is necessary. The integrators of Spoon do not merge something we don't understand fully, and understanding this large a change fully in one go is a lot to ask. A smaller change is also easier to debug if there is significant breakage in post-merge tests. So by all means, this needs to be broken down into smaller pieces. @I-Al-Istannen feel free to weigh in. That goes double for the changes to the public API, which we are very conservative in changing. Every single method has to be carefully justified, reviewed and reasoned about. A single PR adding more than a single new method is therefore just on that account asking a whole lot of reviewers. I just want to make clear that I'm in no way trying to discourage you from contributing Spoon. It's great that you want to contribute, without people like you we would not have Spoon. I can tell you've put a lot of effort into this and for that I salute you, but the unfortunate reality is that with all of that effort you've created something too large for us to take in in one go. |
Linked issue: #4746
In Java 9 modules were introduced. This PR aims to add support for shadow modules and fix some inconsistencies about the existing module model without any breaking changes.
A list of changes in short:
Added public method getDeclaringModule inside CtElement, moved from CtPackage. The reason for this change is that it's important for a type to be easily traceable to its parent module. (Module awareness)
CtModule now extends the CtShadowable interface which means that the entire language is supported by shadow models. The getPackage(String) and getAllPackages() methods were also added to the same interface to easily find a package by its fully qualified name or all packages inside a module. (module awareness)
Added all the properties defined in a [module descriptor] (https://docs.oracle.com/javase/17/docs/api/java/lang/module/ModuleDescriptor.html) by the JLS (Module model completeness)
Added method createPackage(CtModule) to Factory and CoreFactory to create a package inside a module. Now createPackage() invokes createPackage(CtModule) using the unnamed module as an argument(default behaviour of previous versions).
EnumFactory, on line 62 there was an erroneous use of the wildcard that wouldn't make the code compile, fixed it. (potentially unrelated)
Reworked the internals of ModuleFactory to make the unnamed module a standalone component of the AST(consistency)
Reworked the internals of PackageFactory to make packages module aware and create shadow modules if needed.
CtElementPathBuilder: changed fromElement method to use the root package of the module more efficiently (implementation)
AllMethodsSameSignatureFunction, OverriddenMethodQuery, SubInheritanceHierarchyFunction, Query, JDTBasedSpoonCompiler, SnippetCompilationHelper, SpoonModelTree: these look like big changes(practically most of the PR) but I've just added at most 2 lines of code to check in all modules and not only in the unnamed module(previously the unnamed module contained contained all modules so it was enough)
CtInheritanceScanner, added the scanCtShadowable as necessary in visitCtModule
Added getModule, addModule and removeModule methods from CtModule for consistency with all other public properties and as it looks like it's required by integration tests.
Added methods getPackage(String), getModule(String), addModule(CtModule) and removeModule(CtModule) inside CtModel to allow for the existence of multiple modules instead of making all modules live under the unnamed module(as far as I understand this was the previous implementation, but it's objectively erroneous for the model to not divide modules)
Reworked CtModelImpl internals to work with the new module structure and decoupled CtRootPackage, consistent with CtUnnamedModule.
CtElementImpl replaced factory calls with getFactory()
ElementNameMap added null checks to accommodate for modules having a null parent
JavaReflectionTreeBuilder, JavaReflectionVisitor, JavaReflectionVisitorImpl, ModuleRuntimeBuilderContext: needed for shadow module construction. No logical changes
Changed SetParentTest to check new contract(aka modules have no parent)
New hierarchy in short:
