Fix AspectJ basics

[kriegaex]

Fix basic AspectJ compiler functionality + add integration test

This is a first, basic step into improving AspectJ support for Plexus Compiler. It looks like nobody ever noticed, that target and release parameters are not forwarded to AJC, resulting in all class files being compiled to Java 1.2 target. I.e. that annotation-based @AspectJ syntax was completely unsupported before. Since Plexus was introduced 15 years ago, nobody ever did anything to upgrade the functionality a bit, only AspectJ versions were bumped before.

Furthermore, only *.java files were being considered for compilation, no *.aj files. This was also fixed, but at the cost of duplication with regard to redefining two static methods getSourceFiles and getSourceFilesForSourceRoot. Other compiler components did it similarly, so the necessary refactoring to make the methods non-static and break them down into smaller parts in order to be able to override only the parts of them dealing with source file extensions, is a TODO which involves refactoring the other compiler components, too. I did not do that in this first step.

Another TODO: In contrast to the ECJ adapter, which uses the batch compiler, AJC (an ECJ fork!) is used via the internal AJDT interface, which is designed to be used by the Eclipse IDE. It would have been easier to actually also use the batch compiler right from the start, because then we can more easily map command line parameters and keep the adapter layer thin. Why the original author did that in 2005, remains a mystery.

The new AspectJ integration test uses a mixture of

• native and @AspectJ syntax variants,
• *.java and *.aj aspect source files,
• production (src/main) and test (src/test) aspects.

This is all covered by a single IT. In the end, the test asserts on the complete JUnit console log, which has to look a certain way in order to reflect that

• compilation and test were successful,
• all 3 aspects did what they are supposed to do.

[kriegaex]

@olamy, like we said in the e-mail exchange: I am going to look into AspectJ compilation via Plexus and you can review and merge, if applicable. A lot more needs to be done here, but after looking into this today for the first time, at least I wanted to enable basic AspectJ functionality for the first time in 15+ years. The previous base class unit test only tested AJC as a drop-in replacement for Javac, no actual AspectJ.

[kriegaex]

The IT failing is the one for the ErrorProne integration on some JDK-17-EA configurations. It has been failing since shortly after 17-EA was added to the build matrix. The new AspectJ IT and the other tests are passing. So I guess, this problem needs to be fixed on master:

[olamy]

thanks for work here. only very nitpick comments.

[kriegaex]

Can we please conclude this review? I am waiting for feedback. I think I have addressed all comments with corresponding commits or explanations. Is there anything else I need to do in order to get this merged, or are you guys just busy?

[bmarwell]

Hi Alex, for me the code style changes are still open.
I really can see your point why you want to static import constants (I would do the same in my project), but I think keeping the same style is even more important than that.
Besides, you IDE will help you anyway.
I suggest just importing the class import x.y..ClassFileConstants and then referencing with ClassFileConstants.x.
But I will see if I can get someone else to look over it, so we might get an exception. I feel this could be justified.

Thanks for the patch anyway, you got my +1 (apart from the open nits) for its contents!

[olamy]

Thanks for the contribution Good job (sounds good to me maybe except the import static I don't like much)

[kriegaex]

Some general remarks:

• First of all, thanks for all the reviews, even though I am kind of irritated by the code review style here. It is not how I teach my agile teams to do code reviews, but I am trying to adapt to your expectations here. I know everything you wrote is well meant, so thanks for your guidance.
• I think, a web browser is not the best tool for reviewing code. IMO, it is better to see the big picture, actually run the code, maybe use some IDE features to jump around classes and methods referenced in the changes in order to see more than changed hunks of diff text, partly out of context. Code is a living creature. I like to watch what it is doing and "get a feeling" for it. Even so, just reading the source code here on GitHub is also a nice readability test. Still, I think that the GitHub web UI alone is inadequate in order to do a proper review, especially when trying to suggest better alternatives to the existing code.
• I also think it makes sense to focus on the functionality as such a bit more instead of mainly debating style guide issues.

[kriegaex]

OK, I pushed another commit and hope that all concerns have been adequately addressed. Like I said, Plexus JavaVersion does not cut the cake here, see my lengthier explanation above.

[bmarwell]

LGTM, great PR!

The only think left is to get rid of the 1.x / x versions, as Robert suggested:
#137 (comment)

I think you don't need the map and can just do here:
https://github.com/codehaus-plexus/plexus-compiler/pull/137/files/3626cbb98aeceb241d4296b3e67f5405afa2a53e#diff-510a8a3bd19d49ca71cb48fbf3b4e02fecee74c16aaf71759799d923be50d7b6R632-R643

Somethink like:
return JavaVersion.parse( version.trim() ).asMajor() + ".0"

(of course not .0, but you know what I mean).

[kriegaex]

@bmarwell, @rfscholte, maybe I am the problem, not explaining something I thought was obvious well enough, but I really think the two of you are not getting the point, maybe because you simply look at diffs in a browser, like I said before, instead of taking a few minutes to look at the code in an IDE and maybe jump to the map value constant definitions:

• Using a Map is appropriate, because I actually need to map (hence the data structure name) parsed versions to AspectJ constants.
• Parsing the versions is not the problem here, because parsing is not mapping.
• If I would use JavaVersion.parse( version.trim() ).asMajor() + ".0" or something similar, I would have to repeat it for each single key-value pair added to the map, i.e. it would just add duplicated boilerplate and make the code unreadable.

Look, this is what I need to map it to:

This is not done by simply parsing a string to a data structure which does not help me save any lines of code when trying to map it into what is really needed here. The only way not to use a map here and map directly from the parsed major versions to the corresponding AspectJ constant, would be to redundantly mimic the math to calculate those constants directly in Plexus, pulling internal AspectJ knowledge into Plexus, which is bound to break if for whatever reason AspectJ would be refactored.

Can you see it now? Parsing is not the issue here, mapping is. Parsing would not save any code, it would add more.

Refactoring the map, which perfectly fits the use case, back into an if-else structure with many branches, IMO is the wrong thing to do, because it is simply bad code style. By factoring out the if-else into the map, we have much better readability in the code where it matters. The map is trivial and can just be scrolled over. It is also easy to extend, which is good for maintainability. The lesser evil, which Robert also mentioned, would be switch-case, but I also want to avoid that, if possible. I would only use it against my own judgement, if you use your power to further block this PR, not because my code is wrong or unreadable, but simply because you do not like my code style.

[kriegaex]

I really do not like this way of conducting a code review, which I feel is micro-management and therefore quite disheartening. It feels like I am not a fellow developer but some kind of puppet on strings, being told: "Change the code to exactly how I as a reviewer would have coded it, otherwise the merge will be stalled forever." I know, this is not your intention, guys, but this what it boils down to, intentional or not.

So what shall it be? If you are not suggesting to me but actually commanding me to refactor to switch-case, I shall do so. I am going to think twice before contributing more, though. IMO, this change is not complicated. The talking, me having to explain and re-explain each of my conscious decisions and basically defend them like this was some kind of lawsuit, already was significantly more effort than actually creating the code itself. I know that this ratio of coding vs. debate is necessary sometimes, but here I think it is way out of proportion. As much as I like my own contribution - honestly, it is pretty trivial, not rocket science.

[kriegaex]

I hope that all reviewers are happy now, having gotten their way. I still prefer a Map in order to map A to B, instead of replacing it by a control structure like switch-case, even if the latter is shorter in this case (but only if I use compact one-line style case "10" : return ClassFileConstants.JDK10;). I also feel bad about adding a new dependency for one line of code (parsing the version), but what the heck.

[kriegaex]

I have no idea, why the integration test is now failing, because locally it passes. Unfortunately, I cannot inspect the plexus-compiler-its/target/it/aspectj-compiler/build.log. Maybe you can, if you can log into Travis CI and have more access rights than I do.

I am going to set <streamLogsOnFailures>true</streamLogsOnFailures> for now, so I can see what is happening in the failing IT. If you guys want me to revert that after debugging and prefer to rather set a system property for the build job, whatever you prefer is fine with me.

[kriegaex]

By the way, do you have a plan B for this?

June 15th would be today. Maybe it would make sense to migrate to GitHub Actions. @bmarwell, maybe you would like to set up a build job similar to AspectJ Maven. If I would feel a little less frustrated by the code review process just now, I would create another PR myself, but I think I need a little break. I just want to get this one off the table first.

[kriegaex]

OK, great, pulling in the new plexus-java dependency for the sake of version parsing is causing the integration test to fail:

https://travis-ci.org/github/codehaus-plexus/plexus-compiler/jobs/774585596#L2966:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project aspectj-compiler:
Execution default-compile of goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile failed:
An API incompatibility was encountered while executing org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile: 
java.lang.NoSuchMethodError: org.codehaus.plexus.languages.java.version.JavaVersion.asMajor()Lorg/codehaus/plexus/languages/java/version/JavaVersion;

Sigh... The ITs use Maven Compiler 3.8.1, which seems to depend on plexus-java 0.9.10, which does not have the asMajor method yet. So what do we do now?

• Create a PR for Maven Compiler? That would make this component dependent on the very latest Maven Compiler version.
• Override the version in the AspectJ IT? That would mean that every user trying to compile AspectJ with Plexus would have to do the same. Sounds like a non-starter for me.
• Stop using the method, doing a bit more parsing by ourselves?
• Revert to either my map or the switch-case without parsing, but using a fixed set of version strings like in my map?

This is all so unnecessary. My previous solution just worked.

[kriegaex]

OK, I fixed the IT by kicking out plexus-java again, but retaining the switch-case which seems to be what you want to have instead of a map.

Hoping that now finally I am permitted to stop chasing my own tail, would you like me to squash any commits and force-push in order to have fewer of them in the upstream commit history?

[kriegaex]

FYI, I squashed some commits and amended one. Please note my two new comments.

[kriegaex]

When searching for the problem in the IT earlier today, I noticed that the debug output was confusing me more than helping me. I think, it is better to activate it on a case-by-case basis. The standard build also does not run in Maven debug mode.

[kriegaex]

I changed the error message in order to reflect that unlike before, here it is not always about a source version but could also be a target version (or compliance level, more generally).

[bmarwell]

Does it make sense to add Java 17 already (if ClassFileConstants already supports it, didn't check)?

[kriegaex]

Does it make sense to add Java 17 already (if ClassFileConstants already supports it, didn't check)?

Did you see any in my screenshot in #137 (comment)? 😉

Thanks for double-checking. Last time I merged JDT Core into AspectJ around the time of the 2021-03 release, which had fresh Java 16 support, no Java 17 yet. It would not be final anyway, even if I merged today, so it is not worth the effort. Those merges are quite tricky.

[kriegaex]

Thanks for the merge. However, I am not so sure it was a good idea to squash the 4 commits which remained after I squashed by topic myself into a single one. Now there is no spearation of concerns anymore. Debugging or partially reverting something does not get easier that way. In contrast, each single trivial Dependabot version bump is a commit of its own.

[olamy]

in this case it;s better to have 4 pull requests if there is different concerns in each commit.

[kriegaex]

They are related, but still distinct from one another. 4 separate PRs means more CI builds, more review processes, this PR not being able to depend on the other commits. No problem, Olivier. Probably we just have different philosophies or come from different "schools" of development. I am the agile guy and a proponent of small commits. A long commit history is not a problem, it is actually helpful IMO. But I am content with finally having this PR off the table. So thank you very much again.