Support parsing module-info.java

[koppor]

I am using https://app.moderne.io/ (and also locally)

Go to https://app.moderne.io/, select "JabRef" and run org.openrewrite.java.logging.slf4j.ParameterizedLogging

There are 0 results reported:

However, there should be more than 1:

That source code is also available at https://github.com/jabref/jabref/.

[koppor]

I tried

  - org.openrewrite.java.logging.slf4j.Slf4jBestPractices
  - org.openrewrite.java.logging.ParameterizedLogging:
      methodPattern: org.slf4j.Logger error(..)

The latter to match the original PR content. Still no changes applied to the code base.

I asked at https://rewriteoss.slack.com/archives/C01A843MWG5/p1704459019395649 for a end-to-end test. So that one can set breakpoints and see how the whole system behaves. Currently, I am not able to set a breakpoint in the recipe and debug it with my code base.

[timtebeek]

Turns out the issue is caused by missing type information, as explored through the platform:
https://app.moderne.io/recipes/org.openrewrite.java.search.FindMissingTypes

We should figure out why those types are missing, and that will then likely also solve recipe runs against those classes.

[koppor]

For the record an example:

[koppor]

The output of the gradle plugin is pretty clear:

Using active styles [org.openrewrite.java.Checkstyle]
Failed to resolve dependencies from ::implementation. Some type information may be incomplete
Failed to resolve dependencies from ::testImplementation. Some type information may be incomplete

---

Maybe related, when running org.openrewrite.java.testing.junit5.JUnit5BestPractices in moderne (https://app.moderne.io/results/gEbQC3pSP), I get

io.moderne.worker.RecipeLoadingException: io.moderne.worker.ModularizedRecipeLoader$RecipeDependencyLoadException: Failed to resolve recipe dependencies.
Caused by io.moderne.worker.ModularizedRecipeLoader$RecipeDependencyLoadException: Failed to resolve recipe dependencies.
Caused by org.openrewrite.maven.MavenDownloadingExceptions: null
  org.openrewrite.maven.MavenDownloadingExceptions.append(MavenDownloadingExceptions.java:44)
  org.openrewrite.maven.tree.MavenResolutionResult.resolveDependencies(MavenResolutionResult.java:179)
  org.openrewrite.maven.MavenParser.parseInputs(MavenParser.java:113)
  org.openrewrite.Parser.parse(Parser.java:80)
  org.openrewrite.maven.MavenParser.parse(MavenParser.java:55)

(Maybe kind of follow-up to openrewrite/rewrite-testing-frameworks#428)

[timtebeek]

Thanks for that helpful hint @koppor ! It seems the issue arises on these lines then:
https://github.com/openrewrite/rewrite-gradle-plugin/blob/4300cda451aed798c9f099c4b87ba54df6251c95/plugin/src/main/java/org/openrewrite/gradle/isolated/DefaultProjectParser.java#L1048-L1055

We unhelpfully do not log what exception triggered that; perhaps we should to help fix this issue for you, as I have no idea why it would fail now. Perhaps you can add ,e at the end of line 1053, call ./gradlew pTML and then use Gradle plugin 6.7.0-SNAPSHOT to get the stacktrace?

[koppor]

For googlers/noobs:

• ./gradlew pTML means ./gradlew publishToMavenLocal - fun fact: Even ChatGPT did not know
• Ensure, your settings.gradle file contains mavenLocal() in pluginManagement { repositories { ... } }

---

I get the patch to compile and included. However, the issue does NOT appear locally, only on Moderne. I think, I cannot deploy code to there. 🤣🤣

Interestingly, Moderne points to org.openrewrite.maven.MavenParser, but we are on gradle. For sure, a Maven parser is not able to parse Gradle files. How to tell Moderne that this is a gradle project?

[timtebeek]

I'd missed that detail! You're absolutely right that this fails in a Moderne worker. 🙃
Quite curious case as well, since we should just have that recipe in Moderne.

I've reran the recipe just now without issue though, so perhaps it was a one time issue?
https://app.moderne.io/results/YWGK2PQJW/details/eyJfX3R5cGVuYW1lIjoiR2l0SHViUmVwb3NpdG9yeSIsIm9yaWdpbiI6ImdpdGh1Yi5jb20iLCJwYXRoIjoiSmFiUmVmL2phYnJlZiIsImJyYW5jaCI6Im1haW4ifQ==?referrer=https%3A%2F%2Fapp.moderne.io%2Fresults%2FYWGK2PQJW

[koppor]

Result of today: 913 classes w/ missing type information:

https://app.moderne.io/results/JcIPoMVit

[koppor]

I did not see any exception today. (I should have screenshotted where to find a potential exception)

[timtebeek]

We just fixed a rather nasty bug that lead to a lot of missing types that might factor in here as well

• Include compile classpath in dependencies rewrite-gradle-plugin#270

I'll kick off a new ingestion run for JabRef in the next hour or so (needs a release first) such that we can see if there are improvements.

[koppor]

I checked. No changes. I created an MWE with one (!) Java class and one dependency.

The MWE is available at https://github.com/koppor/mwe-openrewrite-rewrite-gradle-plugin-issue-261/. I created a GitHub action to run the gradle plugin. Output at https://github.com/koppor/mwe-openrewrite-rewrite-gradle-plugin-issue-261/actions.

The build.gradle even fits on a screen:

plugins {
    id 'java'
    id 'org.openrewrite.rewrite' version '6.7.0'
}

repositories {
    mavenCentral()
}

dependencies {
    implementation 'org.slf4j:slf4j-api:2.0.11'
}

rewrite {
    activeRecipe(
        'org.jabref.config.rewrite.cleanup'
    )
    exclusion (
        "build.gradle",
    )
    failOnDryRunResults = true
}

---

Note, this is groovy and not Kotlin!

It must be some dumb configuration error. I know, there is no bom dependency above. The full project has (and it has that issue, too).

[koppor]

I also tried with build.gradle.kts, also fails.

Branch: https://github.com/koppor/mwe-openrewrite-rewrite-gradle-plugin-issue-261/tree/mwe-kotlin

[timtebeek]

What made you add the exclusion on build.gradle above? 🤔 Can't imagine that helping resolve dependencies and types.

    exclusion (
        "build.gradle",
    )

Indeed the missing types issue is still there for Jabref; don't yet know why, but that will affect all recipe runs. :/

[koppor]

Side note - before I forget - all gradle Plugins listed at moderne either have an exception (1) or have at least one issue in finding types (https://app.moderne.io/results/Zlh75XwIK#state=eyJjb2x1bW5zIjp7ImRpbWVuc2lvbnMiOnsiX19jaGVja19fIjp7Im1heFdpZHRoIjotMSwibWluV2lkdGgiOjUwLCJ3aWR0aCI6NTB9LCJhY3Rpb25zIjp7Im1heFdpZHRoIjotMSwibWluV2lkdGgiOjEwMCwid2lkdGgiOjEwMH0sInN0YXR1cyI6eyJtYXhXaWR0aCI6LTEsIm1pbldpZHRoIjoxMjUsIndpZHRoIjoxMDAsImZsZXgiOjAuMTI1fSwiZGV0YWlscyI6eyJtYXhXaWR0aCI6LTEsIm1pbldpZHRoIjoxNTAsIndpZHRoIjoxMDAsImZsZXgiOjAuMjV9LCJvcmdhbml6YXRpb24iOnsibWF4V2lkdGgiOi0xLCJtaW5XaWR0aCI6MTUwLCJ3aWR0aCI6MTAwLCJmbGV4IjowLjI1fSwicmVwb3NpdG9yeSI6eyJtYXhXaWR0aCI6LTEsIm1pbldpZHRoIjoyMDAsIndpZHRoIjoxMDAsImZsZXgiOjF9LCJicmFuY2giOnsibWF4V2lkdGgiOi0xLCJtaW5XaWR0aCI6MTUwLCJ3aWR0aCI6MTAwLCJmbGV4IjowLjI1fSwidG90YWxSZXN1bHRzIjp7Im1heFdpZHRoIjotMSwibWluV2lkdGgiOjEyNSwid2lkdGgiOjEwMCwiZmxleCI6MC4xMjV9LCJ0b3RhbFNlYXJjaGVkIjp7Im1heFdpZHRoIjotMSwibWluV2lkdGgiOjEyNSwid2lkdGgiOjEwMCwiZmxleCI6MC4xMjV9LCJpbmZvTWFya2VycyI6eyJtYXhXaWR0aCI6LTEsIm1pbldpZHRoIjoxMjUsIndpZHRoIjoxMDAsImZsZXgiOjAuMTI1fSwid2FybmluZ01hcmtlcnMiOnsibWF4V2lkdGgiOi0xLCJtaW5XaWR0aCI6MTI1LCJ3aWR0aCI6MTAwLCJmbGV4IjowLjEyNX0sImVycm9yTWFya2VycyI6eyJtYXhXaWR0aCI6LTEsIm1pbldpZHRoIjoxMjUsIndpZHRoIjoxMDAsImZsZXgiOjAuMTI1fSwiZGVidWdNYXJrZXJzIjp7Im1heFdpZHRoIjotMSwibWluV2lkdGgiOjEyNSwid2lkdGgiOjEwMCwiZmxleCI6MC4xMjV9LCJhc3RMb2FkaW5nIjp7Im1heFdpZHRoIjotMSwibWluV2lkdGgiOjEyNSwid2lkdGgiOjEwMCwiZmxleCI6MC4xMjV9LCJyZWNpcGVSdW4iOnsibWF4V2lkdGgiOi0xLCJtaW5XaWR0aCI6MTI1LCJ3aWR0aCI6MTAwLCJmbGV4IjowLjEyNX0sImRlcGVuZGVuY3lSZXNvbHV0aW9uIjp7Im1heFdpZHRoIjotMSwibWluV2lkdGgiOjEyNSwid2lkdGgiOjEwMCwiZmxleCI6MC4xMjV9LCJ3b3JrZXIiOnsibWF4V2lkdGgiOi0xLCJtaW5XaWR0aCI6MTUwLCJ3aWR0aCI6MTAwLCJmbGV4IjowLjI1fSwicXVldWVQb3NpdGlvbiI6eyJtYXhXaWR0aCI6LTEsIm1pbldpZHRoIjoxMjUsIndpZHRoIjoxMDAsImZsZXgiOjAuMTI1fSwibGFzdFVwZGF0ZWQiOnsibWF4V2lkdGgiOi0xLCJtaW5XaWR0aCI6MjAwLCJ3aWR0aCI6MTAwLCJmbGV4IjoxfX19LCJzb3J0aW5nIjp7InNvcnRNb2RlbCI6W3siZmllbGQiOiJ0b3RhbFJlc3VsdHMiLCJzb3J0IjoiYXNjIn1dfX0=)

---

I like self-contained links. Never saw that long links 🤣🤣

[koppor]

What made you add the exclusion on build.gradle above? 🤔

There were erros. However, did not happen today with current main of JabRef.

Can't imagine that helping resolve dependencies and types.

😇🙈

Will try to try later working one the MWE to get build.gradle included and reproducing the issue.

[koppor]

Removed ignoring of build.gradle. Still fails:

 diff --git a/src/main/java/org/jabref/cli/Launcher.java b/src/main/java/org/jabref/cli/Launcher.java
index 403a0b4..c1e3856 100644
--- a/src/main/java/org/jabref/cli/Launcher.java
+++ b/src/main/java/org/jabref/cli/Launcher.java
@@ -3,9 +3,9 @@ org.openrewrite.config.CompositeRecipe
 import org.slf4j.Logger;
 
 public class Launcher {
-    private static Logger LOGGER;
+    private static /*~~(Identifier type is missing or malformed)~~>*/Logger LOGGER;
 
     public static void main(String[] args) {
-        LOGGER.info("Started main");
+        /*~~(Identifier type is missing or malformed)~~>*/LOGGER.info("Started main");
     }
 }

diff --git a/build.gradle b/build.gradle
index c062021..f3bae82 100644
--- a/build.gradle
+++ b/build.gradle
@@ -12,7 +12,7 @@ org.openrewrite.config.CompositeRecipe
 }
 
 rewrite {
-    activeRecipe(
+    /*~~(MethodInvocation type is missing or malformed)~~>*/activeRecipe(
         'org.jabref.config.rewrite.cleanup'
     )
     failOnDryRunResults = true

See https://github.com/koppor/mwe-openrewrite-rewrite-gradle-plugin-issue-261/actions/runs/7688550336/job/20949897316

[shanman190]

So it's expected that FindMissingTypes will report missing type information in the build.gradle files. This is due to extension methods being dynamic dispatched methods in Gradle buildscripts, so the type information just isn't there for the parsers at the moment to connect the dots.

That is really strange though that the logger type isn't being found...

[knutwannheden]

Connecting with the debugger seems like the way forward. I would first check if the JavaParser gets properly created with the classpath and would thus set a breakpoint in the build plugin or the JavaParser.builder() method.

[koppor]

Current state of investigation: When adding the test "locally" to the plugin source code, it determines "only" issues at build.gradle, which IMHO can be ignored.

diff --git a/build.gradle b/build.gradle
index cb9b3cf..cd51727 100644
--- a/build.gradle
+++ b/build.gradle
@@ -6,7 +6,7 @@ org.openrewrite.config.CompositeRecipe
 repositories {
     mavenCentral()
     maven {
-       url = uri("https://oss.sonatype.org/content/repositories/snapshots")
+       url = /*~~(MethodInvocation type is missing or malformed)~~>*/uri("https://oss.sonatype.org/content/repositories/snapshots")
     }
 }
 
@@ -14,6 +14,6 @@
     implementation 'org.slf4j:slf4j-api:2.0.11'
 }
             rewrite {
-                activeRecipe('org.jabref.config.rewrite.cleanup')
+                /*~~(MethodInvocation type is missing or malformed)~~>*/activeRecipe('org.jabref.config.rewrite.cleanup')
             }
             
\ No newline at end of file

---

Thus, I am personally back in the helpless world of how to debug a gradle build without using the project of rewrite-gradle-plugin, but the MWE project.

[knutwannheden]

@koppor If you use mod build you should be able to add the --jvm-debug flag and connect with a debugger started from an IDE for the rewrite-gradle-plugin project.

[knutwannheden]

Here you still use the rewrite-gradle-plugin project, but only to step through its code in the debugger. You would still be debugging what goes wrong when building your project.

[koppor]

@knutwannheden I do not understand. Inside the plugin the MWE works. See openrewrite/rewrite-gradle-plugin#273. - It does not work when the release version is used. See https://github.com/koppor/mwe-openrewrite-rewrite-gradle-plugin-issue-261/actions/runs/7688550336/job/20949897316.

I am reading https://medium.com/grandcentrix/how-to-debug-gradle-plugins-with-intellij-eef2ef681a7b, but I cannot get it to work in the MWE project to step into the source of the gradle plugin.

[koppor]

OK, I managed to set a breakpoint...

Let's see how long it takes me to see the source.

[knutwannheden]

I suspect this is in the JavaParser implementation and will thus also affect the Maven plugin. Have you checked if it can be reproduced using a unit test? That would be helpful to have.

[koppor]

@knutwannheden You are right. I submitted openrewrite/rewrite-maven-plugin#737 for illustration that the issue also happens when using Maven. -- I thought, it could be because of the pre-conditions of classgraph because of JDK16+, but seeing my tests above with Java 11, it should be something different 😅.

[timtebeek]

Had a brief look at this by adding a unit test to our JavaParserTest

@Test
void moduleInfo(){
    rewriteRun(
        java(
            """
            module demo {
                requires java.base;
            }
            """,
          spec -> spec.path("module-info.java")
        )
    );
}

This already fails with

java.lang.AssertionError: Source file was parsed into an LST that contains non-whitespace characters in its whitespace. This is indicative of a bug in the parser. 
~~(non-whitespace)~~>module demo {
    requires java.base;
}<~~

This is likely something to fix in the JavaParsers, as any issues there are likely to cause further issues downstream.

I'll move this issue to reflect that we likely need a fix in openrewrite/rewrite instead.

[koppor]

Had a brief look at this by adding a unit test to our JavaParserTest
[...]
This already fails with

I think, this is "OK". org.openrewrite.java.JavaVisitor#visitCompilationUnit "just" does not handle module, which is IMHO fine.

This is likely something to fix in the JavaParsers, as any issues there are likely to cause further issues downstream.

I would be willing to help, but I think, my setup is not "good enough". I think, I need a gradle minimal project to really have the "correct" classpath. Then, I need the rewrite-gradle-plugin. Then, I need rewrite itself.

I have no clue how to debug and to tell gradle to use a modified source.

[timtebeek]

Hmm; can't say I've done any debugging of openrewrite/rewrite modifications through the gradle plugin before I think; Knut added some hints above; he or Shannon might know more as to the best way to go about that.

Looking at this issue again I'm wondering if there's a simpler fix we can do in the short term: our parser does not support module { ... }, so then perhaps we can skip module-info.java in the accept method.

rewrite/rewrite-java/src/main/java/org/openrewrite/java/JavaParser.java

Lines 290 to 292 in 8988375

	default boolean accept(Path path) {
	return path.toString().endsWith(".java");
	}

Any thoughts on that? Might just get you going, and the files should still be parsed by our plain text parser for the OSS plugins.

[shanman190]

It is definitely possible to debug locally. Since the issue is almost certainly within rewrite proper, I'll illustrate how to get a working environment and debug the Java parsers using the full MWE.

From a process standpoint, I'd do the following:

1. Checkout openrewrite/rewrite@main and make sure to pull the Git tags
2. Open your IDE with the rewrite repository
3. Run ./gradlew assemble publishToMavenLocal (make note of the release version published; you can run this within the IDE or outside of it)
4. Make sure the MWE is using the latest release or snapshot version of the OpenRewrite Gradle plugin.
5. Set the rewrite.rewriteVersion to the snapshot version you discovered as part of step 2.
6. In your MWE, run ./gradlew <task> -Dorg.gradle.debug=true (I tend to use rewriteDryRun)
7. Place an initial breakpoint in the JavaParser#parseInputs method of the Java version you'll be using for the MWE parsing.
8. In your IDE from step 2, connect to the JVM with debugging enabled and you should hit your breakpoint pretty quickly.

NOTE: Make sure that in your MWE that the project repositories also includes mavenLocal first. We want to make sure that the library that Gradle resolves is from your Maven Local and not from Sonatype.

[koppor]

My current workaround:

1. Delete module-info.java
2. Adapt build.gradle to remove any modularization-related statements
3. ./gradlew rewriteRun -x compileJava -x compileTestJava

Then, all changes are applied well.

[timtebeek]

Came up again on a new issue; I've gone back and updated the first issue to report problems parsing module-info.java in #1895 (comment) . There's value in the discussion above, so we can keep this one open too, but figure share this insight already. Based on an earlier attempt full support for parsing and modifying such files might be more effort, but skipping them to remove the need for the above workaround could be fairly straightforward.

[koppor]

Quick thought: Maybe, it is "only" about adding a --module-path java compile parameter, similar to what the checkerframework gradle plugin does:

checkerFramework {
  extraJavacArgs = [
    '--module-path', compileOnly.asPath
  ]
}

[timtebeek]

Had another quick look at this; it's indeed an issue with out Java 11+ parsers, and the tree model we map to. If we use the test from #4054 (comment) and run that with a breakpoint set in ReloadableJava17ParserVisitor#visitCompilationUnit, then we can see that the module declaration ends up in the J.CompilationUnit's Space eof.

We could instead there retrieve the module declaration, if present, and map that to new J tree elements for modules and directives, all with the proper handling for comments and whitespace.

JCModuleDecl module = cu.getModule();
com.sun.tools.javac.util.List<JCDirective> directives = module.getDirectives();
... TODO: Map to new J tree elements ...

That's likely quite a bit more work, and would have to be replicated across the Java 11, 17 and 21 parsers. Anyone welcome to pitch in on that; it's likely not complicated, just hasn't been done up to now.

Alternatively we can do what was proposed previously: skip those module-info.java files in our JavaParsers; which could be a nice stop gap seeing as there's also not been any recipes specifically targeting module-info.java files before, and I doubt we see much trouble from them not being covered by say a ChangePackage for now.

We might then still need further work to get the type attribution to work; potentially including something similar to the --module-path outlined above.

[timtebeek]

Following this PR the parsers should at least no longer trip up;

• Ignore module-info.java files for now #4380

We can continue to track proper support here; after an earlier attempt failed to be merged

• Adding support for java module-info files. (issue: #1895) #1925

[JonathanGiles]

Thanks for working on this - module-info.java is something critical to our libraries, so looking forward to this working!