Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of += on a private String in a nested class causes IllegalStateException when imported #1146

Closed
OneGeek opened this issue Jul 26, 2023 · 3 comments · Fixed by #1203
Closed
Assignees
Labels

Comments

@OneGeek
Copy link

OneGeek commented Jul 26, 2023

JDK version: Amazon Corretto 11.0.16
Java Source/Target version: 8
ArchUnit version: 1.0.1

At least this version of the JDK, when targeting Java 8, replaces uses of += with StringBuilder in a way that results in a synthetic access method being created that ArchUnit doesn't seem to know what to do with.


Here's a class, that when imported, that triggers the error in ArchUnit

public class Outer {
    private String somePrivateString = "Hello";
    
    class Inner {
        private void methodThatAccessesOuterClassPrivateField() {
            somePrivateString += ", world!";
        }
    }
}

The following test will fail with the below error

@Test
public void broken() {
    new ClassFileImporter().importClass(Outer.class);
}

java.lang.IllegalStateException: Never found a JavaCodeUnit that matches supposed origin CodeUnit{name='access$084', descriptor=(LOuter;Ljava/lang/Object;)Ljava/lang/String;, declaringClassName='Outer'}

Full Stack Trace:
java.lang.IllegalStateException: Never found a JavaCodeUnit that matches supposed origin CodeUnit{name='access$084', descriptor=(Lcom/ekotrope/broken/Outer;Ljava/lang/Object;)Ljava/lang/String;, declaringClassName='com.ekotrope.broken.Outer'}

	at com.tngtech.archunit.core.importer.RawAccessRecord$CodeUnit.resolveFrom(RawAccessRecord.java:132)
	at com.tngtech.archunit.core.importer.AccessRecord$Factory.lambda$createOriginSupplier$0(AccessRecord.java:294)
	at com.tngtech.archunit.thirdparty.com.google.common.base.Suppliers$NonSerializableMemoizingSupplier.get(Suppliers.java:183)
	at com.tngtech.archunit.core.importer.AccessRecord$Factory$RawAccessRecordProcessed.getOrigin(AccessRecord.java:255)
	at com.tngtech.archunit.core.importer.ClassGraphCreator.tryProcess(ClassGraphCreator.java:144)
	at com.tngtech.archunit.core.importer.ClassGraphCreator.lambda$completeCodeUnitDependencies$2(ClassGraphCreator.java:129)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.Collections$2.tryAdvance(Collections.java:4747)
	at java.base/java.util.Collections$2.forEachRemaining(Collections.java:4755)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:274)
	at java.base/java.util.HashMap$KeySpliterator.forEachRemaining(HashMap.java:1621)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
	at com.tngtech.archunit.core.importer.ClassFileImportRecord.forEachRawConstructorCallRecord(ClassFileImportRecord.java:277)
	at com.tngtech.archunit.core.importer.ClassGraphCreator.completeCodeUnitDependencies(ClassGraphCreator.java:128)
	at com.tngtech.archunit.core.importer.ClassGraphCreator.complete(ClassGraphCreator.java:107)
	at com.tngtech.archunit.core.importer.ClassFileProcessor.process(ClassFileProcessor.java:75)
	at com.tngtech.archunit.core.importer.ClassFileImporter.importLocations(ClassFileImporter.java:325)
	at com.tngtech.archunit.core.importer.ClassFileImporter.importClasses(ClassFileImporter.java:284)
	at com.tngtech.archunit.core.importer.ClassFileImporter.importClasses(ClassFileImporter.java:270)
	at com.tngtech.archunit.core.importer.ClassFileImporter.importClass(ClassFileImporter.java:262)
	at com.ekotrope.ArchUnitBugReproTest.broken(ArchUnitBugReproTest.java:15)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
	at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:232)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:55)

This implementation will pass the test

public class Outer {
    private String somePrivateString = "Hello";
    
    class Inner {
        private void methodThatAccessesOuterClassPrivateField() {
            somePrivateString = somePrivateString + ", world!";
        }
    }
}

Debugging into ArchUnit, I saw that the RawAccessRecord containing the CodeUnit mentioned in the error had a target that mentioned StringBuilder, which isn't anywhere in the original code. However, if you look at the bytecode, the compiler has decided to replace the use of += and use a StringBuilder instead. According to the Java Language Spec, this is allowed.

This is a bytecode disassembly showing use of StringBuilder
image


A complete repro is available at https://github.com/OneGeek/archunit-bug-repro/ , which includes tests that import classes implemented with and without the use of +=. Only the one without passes.

@hankem
Copy link
Member

hankem commented Aug 3, 2023

Thanks for reporting this issue in such great detail! I had a quick look and can reproduce it thanks to your demo repository – which highly appreciated. 💚

@KorSin
Copy link
Contributor

KorSin commented Aug 4, 2023

I had a look at it and I think the Stringbuilder is not the problem, at least not directly. To me it seems that the synthetic access is not registered correctly or cannot be resolved. Right now I do not fully understand it, but will investigate further :)

@hankem
Copy link
Member

hankem commented Aug 4, 2023

FYI: git bisect found that this was broken with 566eb4b.

@codecholeric codecholeric self-assigned this Nov 26, 2023
codecholeric added a commit that referenced this issue Dec 3, 2023
We weren't aware that the compiler occasionally generates synthetic
`access$123` methods that call constructors. More precisely for the
following constellation
```
class Outer {
  String field = "";
  class Inner {
    void concat() {
      field += "any";
    }
  }
}
```
the compiler generates bytecode instantiating and using a
`StringBuilder`. But for constructor calls the synthetic access
resolution was not hooked in, because we assumed that those methods
would never call a constructor. This in turn lead to the bug
```
java.lang.IllegalStateException: Never found a JavaCodeUnit that matches supposed origin CodeUnit{name='access$123'...
```
I.e. the method `access$123` was filtered out as synthetic, but the
origin of the constructor call had not been resolved to the actual
`Inner.concat` method.

Resolves: #1146
Resolves: #1194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants