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

Not a class error for generated constructor in nested class when using ECJ (and lombok) #268

Closed
famod opened this issue Nov 3, 2022 · 12 comments · Fixed by #270
Closed
Assignees
Milestone

Comments

@famod
Copy link
Contributor

famod commented Nov 3, 2022

See this reproducer: code-with-quarkus.zip

Run ./mvnw -e clean package

[ERROR] Failed to execute goal io.smallrye:jandex-maven-plugin:3.0.1:jandex (make-index) on project code-with-quarkus: Not a class -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal io.smallrye:jandex-maven-plugin:3.0.1:jandex (make-index) on project code-with-quarkus: Not a class
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2 (MojoExecutor.java:375)
    ...
Caused by: java.lang.IllegalArgumentException: Not a class
    at org.jboss.jandex.MethodInfo.asClass (MethodInfo.java:609)
    at org.jboss.jandex.Indexer.rebuildNestedType (Indexer.java:1418)
    at org.jboss.jandex.Indexer.resolveTypePath (Indexer.java:1244)
    at org.jboss.jandex.Indexer.resolveTypeAnnotation (Indexer.java:1113)
    at org.jboss.jandex.Indexer.resolveTypeAnnotations (Indexer.java:950)
    at org.jboss.jandex.Indexer.indexWithSummary (Indexer.java:2323)
    at org.jboss.jandex.maven.JandexGoal.indexDirectory (JandexGoal.java:191)
    at org.jboss.jandex.maven.JandexGoal.execute (JandexGoal.java:158)
    ...

(looking at the code involved, it's a bit funny that Indexer was about to throw an exception anyway)

I debugged a bit and it seems it stumbles upon the constructor of AfterCommittedSynchronization (see @RequiredArgsConstructor).
The generated constructor receives @SuppressFBWarnings(justification = "generated code") via lombok.

Context

I'm running the "old" jboss jandex plugin 1.2.3 just fine in a Quarkus 2.13.3.Final project.
Now that Quarkus 2.14.0.Final is available and is using Jandex 3.0.1, it was time to me to switch to the new coordinates and update.
That's when I ran into this issue. This AsyncEventSender is 1:1 from my actual project.

I wasn't able to reproduce this error with vanilla compiler setup, I had to use ECJ (eclipse compiler) instead!

I guess lombok is not even required to provoke this error, but I wanted to keep closer to my real-world setup.
If required, I can try to kick lombok out of the reproducer.

@famod
Copy link
Contributor Author

famod commented Nov 3, 2022

One more note: This is with ECJ 3.30.0 (transitive dep of plexus-compiler-eclipse:2.12.1), but latest 3.31.0 doesn't make a difference.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 3, 2022

Type annotations are tricky, so thanks for providing a reproducer. I should have time to look at it next week.

@Ladicek Ladicek self-assigned this Nov 3, 2022
@Ladicek
Copy link
Contributor

Ladicek commented Nov 7, 2022

So here's a minimal reproducer:

<?xml version="1.0"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd"
         xmlns="http://maven.apache.org/POM/4.0.0"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
    <modelVersion>4.0.0</modelVersion>
    <groupId>com.example</groupId>
    <artifactId>jandex-repro</artifactId>
    <version>1.0.0-SNAPSHOT</version>

    <properties>
        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
        <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>

        <maven.compiler.release>17</maven.compiler.release>

        <version.jandex>3.0.1</version.jandex>
        <version.maven-compiler-plugin>3.10.1</version.maven-compiler-plugin>
        <version.plexus-compiler>2.12.1</version.plexus-compiler>
    </properties>

    <build>
        <plugins>
            <plugin>
                <artifactId>maven-compiler-plugin</artifactId>
                <version>${version.maven-compiler-plugin}</version>
                <configuration>
                    <compilerId>eclipse</compilerId>
                    <compilerArgs>
                        <arg>-parameters</arg>
                    </compilerArgs>
                </configuration>
                <dependencies>
                    <dependency>
                        <groupId>org.codehaus.plexus</groupId>
                        <artifactId>plexus-compiler-api</artifactId>
                        <version>${version.plexus-compiler}</version>
                    </dependency>
                    <dependency>
                        <groupId>org.codehaus.plexus</groupId>
                        <artifactId>plexus-compiler-eclipse</artifactId>
                        <version>${version.plexus-compiler}</version>
                    </dependency>
                    <dependency>
                        <groupId>org.codehaus.plexus</groupId>
                        <artifactId>plexus-compiler-manager</artifactId>
                        <version>${version.plexus-compiler}</version>
                    </dependency>
                </dependencies>
            </plugin>
            <plugin>
                <groupId>io.smallrye</groupId>
                <artifactId>jandex-maven-plugin</artifactId>
                <version>${version.jandex}</version>
                <executions>
                    <execution>
                        <id>make-index</id>
                        <goals>
                            <goal>jandex</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>
</project>
package com.example;

@interface MyAnnotation {
}

public class TopLevelClass {
    private class InnerClass {
        @MyAnnotation
        public InnerClass() {
        }
    }
}

I added a little bit of debugging info and the exception indeed points to the InnerClass constructor.

Interesting observation: if I comment out <maven.compiler.release>17</maven.compiler.release>, it works fine.

The reproducer is so simple I'd very much expect existing tests to catch the problem. There are CI runs with ECJ, except they seem to target very old Java bytecode. Changing maven.compiler.release to 8 still reproduces the problem, while removing it seems to produce class files with version 51.0 (= Java 1.7). Interestingly, it is Java 1.8 where type annotations were introduced. So in addition to type annotation parsing issue, CI needs to be fixed too.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 7, 2022

Actually here's the truly minimal reproducer:

@Target(ElementType.TYPE_USE)
@interface MyAnnotation {
}

public class TopLevelClass {
    private class InnerClass {
        @MyAnnotation
        public InnerClass() {
        }
    }
}

This causes indexing failure even with stock javac. The reason why the original reproducer only fails with ECJ is that ECJ is buggy -- it treats a non-type annotation as a type annotation. If the annotation is explicitly made a type annotation, the indexing problem persists -- and that is because the type annotation is present on type TopLevelClass.@MyAnnotation InnerClass, but Jandex uses void as the constructor type.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 7, 2022

Okay, so I can see some possible fixes:

  1. Break compatibility with previous Jandex versions and consider constructors to have a return type of the declaring class, instead of void. This allows arbitrary type annotations that can be correctly represented in the Jandex object model.
  2. Parse the type annotations on the constructor return type, but attribute them all to the void type. This is ugly, but doesn't break compatibility and allows retrieving the type annotations at least in some way.
  3. Ignore type annotations on constructors where the type target is empty (= the type annotation is present on the return type) and the type path is non-empty (= the type annotation is present on a nested name -- I believe this is the only possibility, but I'm not exactly sure). This is also ugly, plus it loses information.

I guess I'd prefer 2, then 3, then 1 (even though 1 is conceptually nicest).

@n1hility any opinions here? :-)

@famod
Copy link
Contributor Author

famod commented Nov 7, 2022

Oh wow, so I "found" a real gem here. 😉

@Ladicek
Copy link
Contributor

Ladicek commented Nov 7, 2022

Indeed you did :-) Fortunately, the really tricky methods all work fine once supplied with a synthetic ClassType instead of VoidType, so both 2 and 3 are just a few lines (checking for constructor and perhaps arranging the correct return type).

@Ladicek
Copy link
Contributor

Ladicek commented Nov 7, 2022

I'm looking at the details again and a type annotation may have a non-empty path in 3 cases:

  1. in an array type, a component of the array may be annotated;
  2. in a parameterized type, a type argument or a bound of a wildcard bound may be annotated;
  3. in a nested type, several components of the name may be annotated.

In case of constructors, the type of the constructor is always the type of the declaring class, so cases 1 and 2 do not apply. Further, even if the declaring class is nested, the outer components of the class name cannot be denoted [in the constructor name], so case 3 always collapses to a type annotation on the inner-most type. So option 2 above actually doesn't lose any information and we can safely put the type annotations on the VoidType.

Will submit a PR shortly.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 7, 2022

Done. I will release 3.0.2 tomorrow, unless something proves horribly broken.

@Ladicek Ladicek added this to the 3.0.2 milestone Nov 7, 2022
@famod
Copy link
Contributor Author

famod commented Nov 7, 2022

@Ladicek cool, thanks! I suppose Quarkus also needs this update, not just the Maven plugin?

@Ladicek
Copy link
Contributor

Ladicek commented Nov 8, 2022

I believe if you index the JAR with Jandex 3.0.2 (once I'm able to release it...), you should be able to use it with Quarkus 2.14. But I'll update Quarkus to Jandex 3.0.2 anyway, and probably even propose a backport to 2.14.x.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 8, 2022

Yeah, success finally! :-) Jandex 3.0.2 is in Central now.

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

Successfully merging a pull request may close this issue.

2 participants