review: feature: Implementation for Jigsaw modules(Java 9) with support for shadow modules

[Auties00]

Linked issue: https://github.com/INRIA/spoon/issues/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:

1. 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)

2. 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)

3. 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)

4. 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).

5. EnumFactory, on line 62 there was an erroneous use of the wildcard that wouldn't make the code compile, fixed it. (potentially unrelated)

6. Reworked the internals of ModuleFactory to make the unnamed module a standalone component of the AST(consistency)

7. Reworked the internals of PackageFactory to make packages module aware and create shadow modules if needed.

8. CtElementPathBuilder: changed fromElement method to use the root package of the module more efficiently (implementation)

9. 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)

10. CtInheritanceScanner, added the scanCtShadowable as necessary in visitCtModule

11. 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.

12. 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)

13. Reworked CtModelImpl internals to work with the new module structure and decoupled CtRootPackage, consistent with CtUnnamedModule.

14. CtElementImpl replaced factory calls with getFactory()

15. ElementNameMap added null checks to accommodate for modules having a null parent

16. JavaReflectionTreeBuilder, JavaReflectionVisitor, JavaReflectionVisitorImpl, ModuleRuntimeBuilderContext: needed for shadow module construction. No logical changes

17. Changed SetParentTest to check new contract(aka modules have no parent)

New hierarchy in short:

[MartinWitt]

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?

[Auties00]

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

[flacocobot]

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%.

[MartinWitt]

Can you point me to a test case which produces your problem?

[Auties00]

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?

[Auties00]

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.

[MartinWitt]

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. Edit: There are also 95 checkstyle error.

[Auties00]

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. Edit: There are also 95 checkstyle error.

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

[I-Al-Istannen]

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 mvn checkstyle:check instead?

[Auties00]

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 mvn checkstyle:check instead?

Yeah that's probably it. I'm currently on vacation, I'll do it asap

[Auties00]

It's becoming a challenge to make this ready :/ Fixed all the tests again. It would be nice if I could run the CI myself because I can't check if it passes except for the integration tests which wastes a bunch of weeks

[I-Al-Istannen]

It would be nice if I could run the CI myself because I can't check if it passes except for the integration tests which wastes a bunch of weeks

That is sadly a Github setting we (AFAIK) won't get around until you've merged a PR.

---

About your PR

First 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:

• Updating the readme
• Updating plugin versions
• Reworking how modules are handled internally which might break existing assumptions in some places or downstream consumers (i.e. it might be breaking?). This change also has a huge surface area, necessitating careful review.
  • This also seems to make a lot more places module-aware than before.
• Adding new public APIs to work with modules at the CtModel level (I am not sure what exactly we want to allow here)
• Actually build shadow modules
• Performing some completely unrelated changes AFAICT in CtElementImpl
• Reworks how packages are structured internally (or maybe just refactors some things, I am not sure)
• Adds some unexplained null checks in the ElementNameMap, which warrant a comment
• Fiddles with the termination and duplication conditions of the JavaReflectionTreeBuilder

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 request

I 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.

[Auties00]

Updating the readme Updating plugin versions

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.

Reworking how modules are handled internally which might break existing assumptions in some places or downstream consumers (i.e. it might be breaking?). This change also has a huge surface area, necessitating careful review. Actually build shadow modules

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.

Adding new public APIs to work with modules at the CtModel level (I am not sure what exactly we want to allow here)

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.

Performing some completely unrelated changes AFAICT in CtElementImpl

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.

Reworks how packages are structured internally (or maybe just refactors some things, I am not sure)

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).

Adds some unexplained null checks in the ElementNameMap, which warrant a comment

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

Fiddles with the termination and duplication conditions of the JavaReflectionTreeBuilder

Practically nothing has changed there except for the fact that modules are also analyzed

I 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.

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

[Auties00]

Oh well looks like there is a single test failing right now. I'll look into it

[Auties00]

Ready for review. @I-Al-Istannen sorry for the tag, but can you please run the CI?

[Auties00]

Fixed the checkstyle

[Auties00]

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

[slarse]

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.

For the maintainers: should I fix the Javadoc? I really didn't change anything there.

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 python3 chore/check-javadoc-regressions.py --help to see how to use it. The short of it is that for any new public method, you need to fully document it.

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.

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.