Fix GH-954: module binding error message not rendering correctly.

[ascopes]

What it does

Fixes GH-954: a message rendering issue for module resolution issues.

How to test

Right now, I am integrating with the compiler from a project of mine, and I am coming across
the following diagnostic being output:

8389908 - Cannot bind message for problem (id: 1300) "{0} cannot be resolved to a module" with arguments: {}

I am still looking for the actual trigger of this error in my code, but it appears that the
log message is misconfigured.

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[ascopes]

ECA signed:

Looks like the ECA failure is because my GitHub account hides my email address by default. I've updated the commit itself correctly though.

[trancexpress]

Can you add a test here, or provide a reproducer? Something like this:

diff --git a/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/ModuleBuilderTests.java b/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/ModuleBuilderTests.java
index 422cfbeb15..bd6677a57c 100644
--- a/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/ModuleBuilderTests.java
+++ b/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/ModuleBuilderTests.java
@@ -9061,6 +9061,28 @@ public class ModuleBuilderTests extends ModifyingResourceTests {
                        deleteProject(p1);
                }
        }
+       /**
+        * See: https://github.com/eclipse-jdt/eclipse.jdt.core/issues/954
+        */
+       public void testModuleCannotBeResolvedErrorMessageGh955() throws Exception {
+               try {
+                       IJavaProject p = setupModuleProject("testGh955", new String[] {
+                                       "src/module-info.java",
+                                       "module testmodule {\n" +
+                                       "       requires aaa;\n" +
+                                       "}\n",
+                       });
+
+                       ResourcesPlugin.getWorkspace().build(IncrementalProjectBuilder.FULL_BUILD, null);
+                       waitForAutoBuild();
+
+                       IMarker[] markers = p.getProject().findMarkers(null, true, IResource.DEPTH_INFINITE);
+                       assertMarkers("Unexpected markers", "aaa cannot be resolved to a module",
+                                       markers);
+               } finally {
+                       deleteProject("testGh955");
+               }
+       }
        protected void assertNoErrors() throws CoreException {
                for (IProject p : getWorkspace().getRoot().getProjects()) {
                        int maxSeverity = p.findMaxProblemSeverity(null, true, IResource.DEPTH_INFINITE);

Note that the above test passes without the change here. I'm not sure how to run into #954.

[ascopes]

@trancexpress outside of a project that is invoking this, I am not sure of the best way of reproducing it, since my use case is building from a custom diagnostic listener and file manager. #958 seems to suggest to me that this uses a totally different code flow to the standard configuration, as there are branches that detect if the default components are being used elsewhere that I can spot.

However, inspecting the existing source code:

public void invalidModule(ModuleReference ref) {
	this.handle(IProblem.UndefinedModule,
		NoArgument, new String[] { CharOperation.charToString(ref.moduleName) },
		ref.sourceStart, ref.sourceEnd);
}

...the message arguments are being provided correctly (third param, being passed as an array), but not the problem arguments (second param, being passed as NoArgument)

It seems that the output ECJ is emitting usually makes use of the formatted string using the message arguments, but using the JSR-199 API with a DiagnosticListener, calling Diagnostic.getMessage(Locale.ROOT) produces the output described in the issue.

This appears tobe because EclipseCompilerImpl$Jsr199ProblemWrapper has a diagnostic field, which, upon being inspected in a debugger, is using the empty array problemArguments rather than the populated messageArguments.

Line 354 of EclipseCompilerImpl defines the code for JSR-199's getMessage(Locale) as this:

Which shows it is using the empty array that has been changed in this PR, rather than the messageArguments that appears to be the intent.

Given that the message localisation string is hardcoded to "The type {0} is not accessible", and this is used for both the "message" and the "problem" that get rendered, it makes sense that both message and problem should be formatted using the same array for this to work correctly.

My guess is that assertMarkers is checking the ECJ-specific message in DefaultProblem, and not the JSR-199 Diagnostic<T : JavaFileObject> diagnostic attribute within that DefaultProblem object, which is the thing being output to JSR-199 compatible DiagnosticListener classes, leading to the malformed error manifesting like so:

If you can advise of a good way to replicate this environment in a test pack, I'll be happy to try and push a test for this, however, since I am unfamiliar with this codebase, I am not sure exactly what the conventions are for testing components via the JSR-199 interface with custom implementations of components rather than the standard tooling ECJ provides.

[trancexpress]

If you can advise of a good way to replicate this environment in a test pack, I'll be happy to try and push a test for this.

I see, please add this test:

diff --git a/org.eclipse.jdt.compiler.tool.tests/src/org/eclipse/jdt/compiler/tool/tests/CompilerToolTests.java b/org.eclipse.jdt.compiler.tool.tests/src/org/eclipse/jdt/compiler/tool/tests/CompilerToolTests.java
index becccd0394..671308880b 100644
--- a/org.eclipse.jdt.compiler.tool.tests/src/org/eclipse/jdt/compiler/tool/tests/CompilerToolTests.java
+++ b/org.eclipse.jdt.compiler.tool.tests/src/org/eclipse/jdt/compiler/tool/tests/CompilerToolTests.java
@@ -1416,6 +1416,30 @@ static final String[] FAKE_ZERO_ARG_OPTIONS = new String[] {
 				);
 	}
 
+	/**
+	 * See: https://github.com/eclipse-jdt/eclipse.jdt.core/issues/954
+	 */
+	public void testModuleCannotBeResolvedErrorMessageGh955() throws Exception {
+		suppressTest(
+				"module-info.java",
+				"""
+				module testmodule {
+					requires aaa;
+				}
+				""",
+				"ERROR 2: aaa cannot be resolved to a module",
+				"""
+				----------
+				1. ERROR in ---OUTPUT_DIR_PLACEHOLDER---/module-info.java (at line 2)
+					requires aaa;
+					         ^^^
+				aaa cannot be resolved to a module
+				----------
+				1 problem (1 error)
+				"""
+				);
+	}
+
 	public void testSupportedCompilerVersions() throws IOException {
 		Set<SourceVersion> sourceVersions = compiler.getSourceVersions();
 		SourceVersion[] values = SourceVersion.values();

[trancexpress]

For the ECA, can you try adding a line like this at the end of the commit message:

Signed-off-by: ... <...@...>

E.g. if I were signing the commit, the line would look like this:

Signed-off-by: Simeon Andreev <[email protected]>

If you are using the Eclipse Git Staging view, you can amend the commit and use the "Add Signed-off-by" view button (assuming you have set user.name and user.mail for your forked git repository).

I'm not sure what hooks there are to try to identify your email, but it should be possible to use the line above too (unless I'm out of touch with the GitHub contribution process).

It could also be that GitHub needs some time to "see" the ECA is signed, I'm not sure.

[ascopes]

@trancexpress updated the commit with your patch.

Also looks like the signoff worked as well, which is good. Thanks for all the help!

[trancexpress]

@iloveeclipse this looks like a simple enough change. Can you review it?

[iloveeclipse]

For the ECA, can you try adding a line like this at the end of the commit message:

Signed-off-by: ... <...@...>

Simeon, "signed of" was only used by gerrit, and is useless in github.
The only check github does if the mail used by ECA signing is the same used as commit author mail.

It could also be that GitHub needs some time to "see" the ECA is signed, I'm not sure.

That was most likely the reason.

[trancexpress]

Thanks for the fix @ascopes !