Optimize VisitorState#getConstantExpression

[Stephan202]

By avoiding a second pass over the string.

[cushon]

I tried merging this an am seeing failures like the ones in https://github.com/google/error-prone/actions/runs/12602521683/job/35125772504?pr=4744

I'm able to reproduce those locally with:

mvn clean install -DskipTests=true
mvn test -e
...
Caused by: org.apache.maven.surefire.booter.SurefireBooterForkException: There was an error in the forked process
com/google/errorprone/InvalidCommandLineOptionException
java.lang.NoClassDefFoundError: com/google/errorprone/InvalidCommandLineOptionException
	at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3578)
	at java.base/java.lang.Class.getMethodsRecursive(Class.java:3719)
	at java.base/java.lang.Class.getMethod0(Class.java:3705)
	at java.base/java.lang.Class.getMethod(Class.java:2393)
	at org.apache.maven.surefire.api.util.ReflectionUtils.tryGetMethod(ReflectionUtils.java:50)
	at org.apache.maven.surefire.common.junit3.JUnit3TestChecker.isSuiteOnly(JUnit3TestChecker.java:60)
	at org.apache.maven.surefire.common.junit3.JUnit3TestChecker.isValidJUnit3Test(JUnit3TestChecker.java:56)
	at org.apache.maven.surefire.common.junit3.JUnit3TestChecker.accept(JUnit3TestChecker.java:52)
	at org.apache.maven.surefire.common.junit4.JUnit4TestChecker.accept(JUnit4TestChecker.java:48)
	at org.apache.maven.surefire.api.util.DefaultScanResult.applyFilter(DefaultScanResult.java:87)
	at org.apache.maven.surefire.junit4.JUnit4Provider.scanClassPath(JUnit4Provider.java:272)
	at org.apache.maven.surefire.junit4.JUnit4Provider.setTestsToRun(JUnit4Provider.java:174)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:132)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.ClassNotFoundException: com.google.errorprone.InvalidCommandLineOptionException
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
	... 18 more

Any ideas what's going wrong?

[Stephan202]

@cushon this one was tricky! First to debug, then to find a (hacky...) workaround. It's a shame that we can't use multiReleaseOutput, as that would likely have avoided these shenanigans. Maybe one of the other watchers of this project knows of a more elegant solution :eyes: :crossed_fingers:. I rebased the branch and added a commit; PTAL :)

NB: instead of introducing a new reset-output-directory step I also tried to move down the default-compile step, but that did not cause the order of the compilation steps to change.

[Stephan202]

Added another commit to apply the Elements change.

One issue I just identified: with my change mvn clean install -DskipTests=true && mvn test -e still doesn't fully work when building with JDK 23+, with test failures due to the wrong Convert.quote being invoked. I think that this is because without reference to the JAR, the META-INF/MANIFEST.MF file is ignored. As-is ./check_api/target/classes/META-INF/MANIFEST.MF also doesn't contain Multi-Release: true, thought the following patch would "fix" that:

diff --git a/check_api/pom.xml b/check_api/pom.xml
index 9e8bc1e8df..a5de90556e 100644
--- a/check_api/pom.xml
+++ b/check_api/pom.xml
@@ -229,17 +229,6 @@
         </executions>
 
       </plugin>
-      <plugin>
-        <groupId>org.apache.maven.plugins</groupId>
-        <artifactId>maven-jar-plugin</artifactId>
-        <configuration>
-          <archive>
-            <manifestEntries>
-              <Multi-Release>true</Multi-Release>
-            </manifestEntries>
-          </archive>
-        </configuration>
-      </plugin>
     </plugins>
   </build>
 
diff --git a/pom.xml b/pom.xml
index ac716fb30f..11238fe292 100644
--- a/pom.xml
+++ b/pom.xml
@@ -147,6 +147,7 @@
             </goals>
             <configuration>
               <bnd><![CDATA[
+                Multi-Release: true
                 Bundle-SymbolicName: com.google.$<replacestring;$<replacestring;${project.artifactId};^error_prone;errorprone>;_;.>
                 Automatic-Module-Name: $<Bundle-SymbolicName>
                 -exportcontents: com.google.errorprone*

... but as said, that doesn't fix mvn clean install -DskipTests=true && mvn test -e on JDK 23+.

A simple fix would be to update the CI configuration to just combine installation and testing:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 25684fd42f..afcc2c6970 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -84,12 +84,9 @@ jobs:
           java-version: ${{ matrix.java }}
           distribution: 'zulu'
           cache: 'maven'
-      - name: 'Install'
+      - name: 'Install and test'
         shell: bash
-        run: mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V
-      - name: 'Test'
-        shell: bash
-        run: mvn test -B
+        run: mvn install -Dmaven.javadoc.skip=true -B -V
       - name: 'Javadoc'
         shell: bash
         run: mvn -P '!examples' javadoc:javadoc

Right now I don't have other ideas. :thinking:

[cushon]

I'm fine with changing the invoke in CI invocation, but if separate mvn install and mvn test don't work that seems like it might also affect non-CI development builds? Or would you not expect this to affect development builds?

(I guess another option here would be to give up on multi-release jars for now and just use reflection for this method. But there isn't a good reflection-based alternative alternative for the problem https://github.com/google/error-prone/commit/08e0d108da02ee6b413822db1628f7948b335b10 solves, so it would be nice to make mr-jars work.)