Merge pull request #146 from kriegaex/gh-145

Make AJC respect `--add-reads` and `--add-exports`

14 files changed, 228 insertions, 77 deletions

diff --git a/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/AjBuildManager.java b/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/AjBuildManager.java
index 08bf00d54..75f8c36cd 100644
--- a/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/AjBuildManager.java
+++ b/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/AjBuildManager.java
@@ -1045,6 +1045,13 @@ public class AjBuildManager implements IOutputClassFileNameProvider, IBinarySour
 				classpaths[i] = cps.get(i);
 			}
 			FileSystem fileSystem = getLibraryAccess(classpaths, filenames);
+
+			// Use upstream method to generate '--add-reads', '--add-exports' info and copy it into our FileSystem instance.
+			// See https://github.com/eclipse/org.aspectj/issues/145.
+			FileSystem fileSystemTemp = buildConfig.getBuildArgParser().getLibraryAccess();
+			fileSystem.moduleUpdates = fileSystemTemp.moduleUpdates;
+			fileSystemTemp.cleanup();
+
 			environment = new StatefulNameEnvironment(fileSystem, state.getClassNameToFileMap(), state);
 			state.setFileSystem(fileSystem);
 			state.setNameEnvironment(environment);
diff --git a/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/StatefulNameEnvironment.java b/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/StatefulNameEnvironment.java
index 0af411be3..dad61e7b2 100644
--- a/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/StatefulNameEnvironment.java
+++ b/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/StatefulNameEnvironment.java
@@ -27,6 +27,7 @@ import org.aspectj.org.eclipse.jdt.internal.compiler.classfmt.ClassFormatExcepti
 import org.aspectj.org.eclipse.jdt.internal.compiler.env.IBinaryType;
 import org.aspectj.org.eclipse.jdt.internal.compiler.env.IModule;
 import org.aspectj.org.eclipse.jdt.internal.compiler.env.IModuleAwareNameEnvironment;
+import org.aspectj.org.eclipse.jdt.internal.compiler.env.IUpdatableModule;
 import org.aspectj.org.eclipse.jdt.internal.compiler.env.NameEnvironmentAnswer;
 import org.aspectj.util.FileUtil;
 
@@ -152,6 +153,16 @@ public class StatefulNameEnvironment implements IModuleAwareNameEnvironment {
 	}
 
 	@Override
+	public void applyModuleUpdates(IUpdatableModule module, IUpdatableModule.UpdateKind kind) {
+		baseEnvironment.applyModuleUpdates(module, kind);
+	}
+
+	@Override
+	public char[][] getUniqueModulesDeclaringPackage(char[][] packageName, char[] moduleName) {
+		return baseEnvironment.getUniqueModulesDeclaringPackage(packageName, moduleName);
+	}
+
+	@Override
 	public char[][] getAllAutomaticModules() {
 		return baseEnvironment.getAllAutomaticModules();
 	}
diff --git a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java
index 7a1ad855b..e50ea9757 100644
--- a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java
+++ b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java
@@ -256,29 +256,16 @@ public abstract class AjcTestCase extends TestCase {
 		public String toString() {
 			StringBuilder buff = new StringBuilder();
 			buff.append("message ");
-			if (sourceFileName != null) {
-				buff.append("in file ");
-				buff.append(sourceFileName);
-				buff.append(" ");
-			}
-			if (line != -1) {
-				buff.append("on line ");
-				buff.append(line);
-				buff.append(" ");
-			}
-			if (text != null) {
-				buff.append("containing text '");
-				buff.append(text);
-				buff.append("' ");
-			}
+			if (sourceFileName != null)
+				buff.append("in file ").append(sourceFileName).append(" ");
+			if (line != -1)
+				buff.append("on line ").append(line).append(" ");
+			if (text != null)
+				buff.append("containing text '").append(text).append("' ");
 			if (seeAlsos != null) {
 				buff.append("\n\twith see alsos:");
-				for (ISourceLocation seeAlso : seeAlsos) {
-					buff.append("\t\t");
-					buff.append(seeAlso.getSourceFile().getPath());
-					buff.append(":");
-					buff.append(seeAlso.getLine());
-				}
+				for (ISourceLocation seeAlso : seeAlsos)
+					buff.append("\t\t").append(seeAlso.getSourceFile().getPath()).append(":").append(seeAlso.getLine());
 			}
 			return buff.toString();
 		}
@@ -483,12 +470,10 @@ public abstract class AjcTestCase extends TestCase {
 			addExtra(failureReport, "error", extraErrors);
 			addExtra(failureReport, "fail", extraFails);
 			addExtra(failureReport, "weaveInfo", extraWeaves);
-			failureReport.append("\ncommand was: 'ajc");
+			failureReport.append("\nCommand: 'ajc");
 			String[] args = result.getArgs();
-			for (String arg : args) {
-				failureReport.append(" ");
-				failureReport.append(arg);
-			}
+			for (String arg : args)
+				failureReport.append(" ").append(arg);
 			String report = failureReport.toString();
 			System.err.println(failureReport);
 			fail(assertionFailedMessage + "'\n" + report);
@@ -603,8 +588,7 @@ public abstract class AjcTestCase extends TestCase {
 		StringBuilder cp = new StringBuilder();
 		if (classpath != null) {
 			// allow replacing this special variable, rather than copying all files to allow tests of jars that don't end in .jar
-			cp.append(substituteSandbox(classpath));
-			cp.append(pathSeparator);
+			cp.append(substituteSandbox(classpath)).append(pathSeparator);
 		}
 		if (moduleName == null) {
 			// When running modules, we want more control so don't try to be helpful by adding all jars
@@ -612,10 +596,8 @@ public abstract class AjcTestCase extends TestCase {
 			getAnyJars(ajc.getSandboxDirectory(), cp);
 		}
 		StringBuilder mp = new StringBuilder();
-		if (modulepath != null) {
-			mp.append(substituteSandbox(modulepath));
-			mp.append(pathSeparator);
-		}
+		if (modulepath != null)
+			mp.append(substituteSandbox(modulepath)).append(pathSeparator);
 
 		URLClassLoader sandboxLoader;
 		ClassLoader parentLoader = getClass().getClassLoader().getParent();
@@ -637,7 +619,8 @@ public abstract class AjcTestCase extends TestCase {
 			URL[] sandboxUrls = getURLs(cp.toString());
 			sandboxLoader = createWeavingClassLoader(sandboxUrls, aspectjLoader);
 			// sandboxLoader = createWeavingClassLoader(sandboxUrls,testLoader);
-		} else if(useFullLTW  && useLTW) {
+		}
+		else if(useFullLTW  && useLTW) {
 			if(vmargs == null){
 				vmargs ="";
 			}
@@ -654,6 +637,8 @@ public abstract class AjcTestCase extends TestCase {
 					" -classpath " + cp + pathSeparator + defaultCpAbsolute +
 					" -javaagent:" + javaagent + " " +
 					className + " " + String.join(" ", args);
+				if (Ajc.verbose)
+					System.out.println("\nCommand: '" + command + "'\n");
 				// Command is executed using ProcessBuilder to allow setting CWD for ajc sandbox compliance
 				ProcessBuilder pb = new ProcessBuilder(tokenizeCommand(command));
 				pb.directory( new File(ajc.getSandboxDirectory().getAbsolutePath()));
@@ -667,7 +652,8 @@ public abstract class AjcTestCase extends TestCase {
 				e.printStackTrace();
 			}
 			return lastRunResult;
-		} else if (moduleName != null) {
+		}
+		else if (moduleName != null) {
 			// CODE FOR RUNNING MODULES
 			if(vmargs == null){
 				vmargs ="";
@@ -679,13 +665,11 @@ public abstract class AjcTestCase extends TestCase {
 				if (mp.indexOf("$runtime") != -1) {
 					mp = mp.replace(mp.indexOf("$runtime"),"$runtime".length(),TestUtil.aspectjrtPath().toString());
 				}
-				if (cp.indexOf("aspectjrt")==-1) {
+				if (cp.indexOf("aspectjrt") == -1)
 					cp.append(TestUtil.aspectjrtPath().getPath()).append(pathSeparator);
-				}
-				String command = LangUtil.getJavaExecutable().getAbsolutePath() + " " +vmargs+ (cp.length()==0?"":" -classpath " + cp) + " -p "+mp+" --module "+moduleName   ;
-				if (Ajc.verbose) {
-					System.out.println("Command is "+command);
-				}
+				String command = LangUtil.getJavaExecutable().getAbsolutePath() + " " + vmargs + (cp.length() == 0 ? "" : " -classpath " + cp) + " -p " + mp + " --module " + moduleName;
+				if (Ajc.verbose)
+					System.out.println("\nCommand: '" + command + "'\n");
 				// Command is executed using ProcessBuilder to allow setting CWD for ajc sandbox compliance
 				ProcessBuilder pb = new ProcessBuilder(tokenizeCommand(command));
 				pb.directory( new File(ajc.getSandboxDirectory().getAbsolutePath()));
@@ -699,7 +683,16 @@ public abstract class AjcTestCase extends TestCase {
 				e.printStackTrace();
 			}
 			return lastRunResult;
-		} else if (vmargs!=null && (vmargs.contains("--enable-preview") || vmargs.contains("--add-modules") || vmargs.contains("--limit-modules") || vmargs.contains("--add-reads"))) {
+		}
+		else if (
+			vmargs != null && (
+				vmargs.contains("--enable-preview") ||
+				vmargs.contains("--add-modules") ||
+				vmargs.contains("--limit-modules") ||
+				vmargs.contains("--add-reads") ||
+				vmargs.contains("--add-exports")
+			)
+		) {
 			// If --add-modules supplied, need to fork the test
 			try {
 				//				if (mp.indexOf("$runtime") != -1) {
@@ -709,9 +702,8 @@ public abstract class AjcTestCase extends TestCase {
 					cp.append(pathSeparator).append(TestUtil.aspectjrtPath().getPath());
 				}
 				String command = LangUtil.getJavaExecutable().getAbsolutePath() + " " +vmargs+ (cp.length()==0?"":" -classpath " + cp) + " " + className   ;
-				if (Ajc.verbose) {
-					System.out.println("\nCommand is "+command);
-				}
+				if (Ajc.verbose)
+					System.out.println("\nCommand: '" + command + "'\n");
 				// Command is executed using ProcessBuilder to allow setting CWD for ajc sandbox compliance
 				ProcessBuilder pb = new ProcessBuilder(tokenizeCommand(command));
 				pb.directory( new File(ajc.getSandboxDirectory().getAbsolutePath()));
@@ -725,7 +717,8 @@ public abstract class AjcTestCase extends TestCase {
 				e.printStackTrace();
 			}
 			return lastRunResult;
-		} else {
+		}
+		else {
 			cp.append(DEFAULT_CLASSPATH_ENTRIES);
 			URL[] urls = getURLs(cp.toString());
 			sandboxLoader = new URLClassLoader(urls, parentLoader);
@@ -733,16 +726,13 @@ public abstract class AjcTestCase extends TestCase {
 		ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
 		ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
 
-
 		StringBuilder command = new StringBuilder();
-		command.append("java -classpath ");
-		command.append(cp.toString());
-		command.append(" ");
-		command.append(className);
-		for (String arg : args) {
-			command.append(" ");
-			command.append(arg);
-		}
+		command.append("java -classpath ").append(cp).append(" ").append(className);
+		for (String arg : args)
+			command.append(" ").append(arg);
+		if (Ajc.verbose)
+			System.out.println("\nCommand: '" + command + "'\n");
+
 		//		try {
 		//			// Enable the security manager
 		//			Policy.setPolicy(new MyPolicy());
@@ -1015,27 +1005,17 @@ public abstract class AjcTestCase extends TestCase {
 
 	private void addMissing(StringBuilder buff, String type, List<AjcTestCase.Message> messages) {
 		if (!messages.isEmpty()) {
-			buff.append("Missing expected ");
-			buff.append(type);
-			buff.append(" messages:\n");
-			for (Message message : messages) {
-				buff.append("\t");
-				buff.append(message.toString());
-				buff.append("\n");
-			}
+			buff.append("Missing expected ").append(type).append(" messages:\n");
+			for (Message message : messages)
+				buff.append("\t").append(message.toString()).append("\n");
 		}
 	}
 
 	private void addExtra(StringBuilder buff, String type, List messages) {
 		if (!messages.isEmpty()) {
-			buff.append("Unexpected ");
-			buff.append(type);
-			buff.append(" messages:\n");
-			for (Object message : messages) {
-				buff.append("\t");
-				buff.append(message.toString());
-				buff.append("\n");
-			}
+			buff.append("Unexpected ").append(type).append(" messages:\n");
+			for (Object message : messages)
+				buff.append("\t").append(message.toString()).append("\n");
 		}
 	}
 
@@ -1043,12 +1023,10 @@ public abstract class AjcTestCase extends TestCase {
 	private void getAnyJars(File dir, StringBuilder buff) {
 		File[] files = dir.listFiles();
 		for (File file : files) {
-			if (file.getName().endsWith(".jar")) {
-				buff.append(pathSeparator);
-				buff.append(file.getAbsolutePath());
-			} else if (file.isDirectory()) {
+			if (file.getName().endsWith(".jar"))
+				buff.append(pathSeparator).append(file.getAbsolutePath());
+			else if (file.isDirectory())
 				getAnyJars(file, buff);
-			}
 		}
 	}
 
diff --git a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/CompilationResult.java b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/CompilationResult.java
index 612f16539..f2757bdaa 100644
--- a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/CompilationResult.java
+++ b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/CompilationResult.java
@@ -212,7 +212,7 @@ public class CompilationResult {
 			buff.append(weaveMessage.toString());
 			buff.append("\n");
 		}
-		buff.append("\ncommand was: 'ajc");
+		buff.append("\nCommand: 'ajc");
 		for (String arg : args) {
 			buff.append(' ');
 			buff.append(arg);
diff --git a/pom.xml b/pom.xml
index 71fb2bd66..b5b640a9d 100644
--- a/pom.xml
+++ b/pom.xml
@@ -21,7 +21,7 @@
 		<maven.javadoc.skip>true</maven.javadoc.skip>
 
 		<!-- Dependency versions -->
-		<jdt.core.version>1.9.9</jdt.core.version>
+		<jdt.core.version>1.9.10-SNAPSHOT</jdt.core.version>
 		<asm.version>9.2</asm.version>
 		<lib.ant.version>1.6.3</lib.ant.version>
 		<lib.ant.xerces.version>2.6.2</lib.ant.xerces.version>
diff --git a/tests/bugs199/github_145/add_exports/Application.java b/tests/bugs199/github_145/add_exports/Application.java
new file mode 100644
index 000000000..88bd177aa
--- /dev/null
+++ b/tests/bugs199/github_145/add_exports/Application.java
@@ -0,0 +1,17 @@
+import sun.security.x509.X509CertInfo;
+
+import java.security.cert.CertificateParsingException;
+
+/**
+ * https://github.com/mojohaus/aspectj-maven-plugin/issues/139
+ */
+public class Application {
+  public static void main(String[] args) {
+    try {
+      new X509CertInfo(new byte[0]);
+    }
+    catch (CertificateParsingException e) {
+      System.out.println(e);
+    }
+  }
+}
diff --git a/tests/bugs199/github_145/add_modules/NonModular.java b/tests/bugs199/github_145/add_modules/NonModular.java
new file mode 100644
index 000000000..6bdaffa06
--- /dev/null
+++ b/tests/bugs199/github_145/add_modules/NonModular.java
@@ -0,0 +1,9 @@
+import my.module.Modular;
+
+public class NonModular {
+  Modular modular = new Modular();
+
+  public static void main(String[] args) {
+    System.out.println("Non-modular class can use modular one");
+  }
+}
diff --git a/tests/bugs199/github_145/add_modules/UseJDKExtendedCharsets.java b/tests/bugs199/github_145/add_modules/UseJDKExtendedCharsets.java
new file mode 100644
index 000000000..423d564a8
--- /dev/null
+++ b/tests/bugs199/github_145/add_modules/UseJDKExtendedCharsets.java
@@ -0,0 +1,17 @@
+import java.nio.charset.Charset;
+import sun.nio.cs.ext.ExtendedCharsets;
+
+public class UseJDKExtendedCharsets {
+  static ExtendedCharsets charsets = new ExtendedCharsets();
+  static Charset iso2022jp = charsets.charsetForName("ISO-2022-JP");
+  static Charset jis = charsets.charsetForName("jis");
+  static Charset jis_encoding = charsets.charsetForName("jis_encoding");
+
+  public static void main(String[] args) {
+    // The 3 charsets are aliases of each other
+    assert iso2022jp != null;
+    System.out.println(iso2022jp);
+    assert iso2022jp.equals(jis);
+    assert iso2022jp.equals(jis_encoding);
+  }
+}
diff --git a/tests/bugs199/github_145/add_modules/my.module/module-info.java b/tests/bugs199/github_145/add_modules/my.module/module-info.java
new file mode 100644
index 000000000..beee5b586
--- /dev/null
+++ b/tests/bugs199/github_145/add_modules/my.module/module-info.java
@@ -0,0 +1,3 @@
+module my.module {
+  //exports my.module;
+}
diff --git a/tests/bugs199/github_145/add_modules/my.module/my/module/Modular.java b/tests/bugs199/github_145/add_modules/my.module/my/module/Modular.java
new file mode 100644
index 000000000..febc7f821
--- /dev/null
+++ b/tests/bugs199/github_145/add_modules/my.module/my/module/Modular.java
@@ -0,0 +1,3 @@
+package my.module;
+
+public class Modular {}
diff --git a/tests/bugs199/github_145/add_reads/other.module/module-info.java b/tests/bugs199/github_145/add_reads/other.module/module-info.java
new file mode 100644
index 000000000..3cc4823e7
--- /dev/null
+++ b/tests/bugs199/github_145/add_reads/other.module/module-info.java
@@ -0,0 +1,3 @@
+module other.module {
+  exports other.module;
+}
diff --git a/tests/bugs199/github_145/add_reads/other.module/other/module/Application.java b/tests/bugs199/github_145/add_reads/other.module/other/module/Application.java
new file mode 100644
index 000000000..ff299017e
--- /dev/null
+++ b/tests/bugs199/github_145/add_reads/other.module/other/module/Application.java
@@ -0,0 +1,15 @@
+package other.module;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import my.module.Modular;
+
+public class Application {
+  List<String> list = new ArrayList<>();
+  Modular modular = new Modular();
+
+  public static void main(String[] args) {
+    System.out.println("One modular class can use another one");
+  }
+}
diff --git a/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java b/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java
index b83448935..5b2087ed2 100644
--- a/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java
+++ b/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java
@@ -61,6 +61,22 @@ public class Bugs199Tests extends XMLBasedAjcTestCase {
     runTest("asynchronous proceed for nested around-advice (native, thread pool)");
   }
 
+  public void testAddExports_gh145() {
+    runTest("use --add-exports");
+  }
+
+  public void testAddReads_gh145() {
+    runTest("use --add-reads");
+  }
+
+  public void testAddModules_gh145() {
+    runTest("use --add-modules");
+  }
+
+  public void testAddModulesJDK_gh145() {
+    runTest("use --add-modules with non-public JDK module");
+  }
+
   public static Test suite() {
     return XMLBasedAjcTestCase.loadSuite(Bugs199Tests.class);
   }
diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml b/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml
index 7baeea086..7c38db68e 100644
--- a/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml
+++ b/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml
@@ -515,4 +515,76 @@
  		</run>
  	</ajc-test>
 
+	<ajc-test dir="bugs199/github_145/add_exports" title="use --add-exports" vm="9">
+		<compile
+			files="Application.java"
+			options="-9 --add-exports java.base/sun.security.x509=ALL-UNNAMED"
+		/>
+		<run
+			class="Application"
+			vmargs="--add-exports java.base/sun.security.x509=ALL-UNNAMED"
+		>
+			<stdout>
+				<line text="java.security.cert.CertificateParsingException: java.io.IOException"/>
+			</stdout>
+		</run>
+	</ajc-test>
+
+	<ajc-test dir="bugs199/github_145" title="use --add-reads" vm="9">
+		<compile
+			files="add_modules/my.module/module-info.java add_modules/my.module/my/module/Modular.java"
+			options="-9"
+			outjar="my.module.jar"
+		/>
+		<compile
+			files="add_reads/other.module/module-info.java add_reads/other.module/other/module/Application.java"
+			options="-9 --add-reads other.module=my.module --add-exports my.module/my.module=other.module"
+			modulepath="$sandbox/my.module.jar"
+			outjar="other.module.jar"
+		/>
+		<run
+			class="other.module.Application"
+			vmargs="--add-reads other.module=my.module"
+			modulepath="$sandbox/my.module.jar:$sandbox/other.module.jar"
+		>
+			<stdout>
+				<line text="One modular class can use another one"/>
+			</stdout>
+		</run>
+	</ajc-test>
+
+	<ajc-test dir="bugs199/github_145/add_modules" title="use --add-modules" vm="9">
+		<compile
+			files="my.module/module-info.java my.module/my/module/Modular.java"
+			options="-9"
+			outjar="my.module.jar"
+		/>
+		<compile
+			files="NonModular.java"
+			options="-9 --add-modules my.module --add-exports my.module/my.module=ALL-UNNAMED"
+			modulepath="$sandbox/my.module.jar"
+		/>
+		<run class="NonModular" modulepath="$sandbox/my.module.jar">
+			<stdout>
+				<line text="Non-modular class can use modular one"/>
+			</stdout>
+		</run>
+	</ajc-test>
+
+	<ajc-test dir="bugs199/github_145/add_modules" title="use --add-modules with non-public JDK module" vm="9">
+		<!--
+			Javac, AJC: no errors. ECJ: "invalid module name: jdk.charsets"
+
+			This is because ECJ does not add internal JDK modules as root modules, but AJC does. This was not so much
+			intentional in AJC, but simply due to the hacky implementation. Anyway, it works and makes AJC more
+			Javac-compliant than ECJ, which is nice.
+		-->
+		<compile files="UseJDKExtendedCharsets.java" options="-9 --add-modules jdk.charsets --add-exports jdk.charsets/sun.nio.cs.ext=ALL-UNNAMED" />
+		<run class="UseJDKExtendedCharsets" vmargs="-ea --add-modules jdk.charsets --add-exports jdk.charsets/sun.nio.cs.ext=ALL-UNNAMED">
+			<stdout>
+				<line text="ISO-2022-JP"/>
+			</stdout>
+		</run>
+	</ajc-test>
+
 </suite>