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

MissingOverrideAnnotation: NPE in classDecl.getBody().getMarkers() with Kotlin files #234

Closed
vlsi opened this issue Jan 10, 2024 · 12 comments
Labels
bug Something isn't working parser-kotlin

Comments

@vlsi
Copy link

vlsi commented Jan 10, 2024

How are you running OpenRewrite?

See apache/jmeter#6217, ./gradlew :src:jorphan:rewriteRun

What is the smallest, simplest way to reproduce the problem?

No idea

What did you expect to see?

MissingOverrideAnnotation should either skip Kotlin files or it should add override respectively.

What did you see instead?

The issue might be caused by withBody(null) (see https://github.com/openrewrite/rewrite/blob/fb7dbd6c95ef5492301dcb941e7f8eefd223621e/rewrite-java/src/main/java/org/openrewrite/java/internal/template/AnnotationTemplateGenerator.java#L226 ), however, I'm not sure what triggers it.

The problematic class is https://github.com/apache/jmeter/blob/09c3f810dda7efaf1510ac062454d978d90a0741/src/jorphan/src/main/kotlin/org/apache/jorphan/gui/CardLayoutWithSizeOfCurrentVisibleElement.kt#L31

org.openrewrite.internal.RecipeRunException: java.lang.NullPointerException: Cannot invoke "org.openrewrite.java.tree.J$Block.getStatements()" because the return value of "org.openrewrite.java.tree.J$ClassDeclaration.getBody()" is null
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:329)
        at org.openrewrite.kotlin.internal.KotlinPrinter$KotlinJavaPrinter.visit(KotlinPrinter.java:495)
        at org.openrewrite.kotlin.internal.KotlinPrinter.visit(KotlinPrinter.java:56)
        at org.openrewrite.kotlin.internal.KotlinPrinter.visit(KotlinPrinter.java:40)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:184)
        at org.openrewrite.Tree.print(Tree.java:94)
        at org.openrewrite.Tree.print(Tree.java:90)
        at org.openrewrite.Tree.printTrimmed(Tree.java:109)
        at org.openrewrite.java.internal.template.AnnotationTemplateGenerator.classDeclaration(AnnotationTemplateGenerator.java:227)
        at org.openrewrite.java.internal.template.AnnotationTemplateGenerator.template(AnnotationTemplateGenerator.java:141)
        at org.openrewrite.java.internal.template.AnnotationTemplateGenerator.lambda$template$0(AnnotationTemplateGenerator.java:73)
        at io.micrometer.core.instrument.composite.CompositeTimer.record(CompositeTimer.java:65)
        at org.openrewrite.java.internal.template.AnnotationTemplateGenerator.template(AnnotationTemplateGenerator.java:69)
        at org.openrewrite.java.internal.template.JavaTemplateParser.lambda$parseAnnotations$10(JavaTemplateParser.java:203)
        at org.openrewrite.java.internal.template.JavaTemplateParser.cache(JavaTemplateParser.java:295)
        at org.openrewrite.java.internal.template.JavaTemplateParser.parseAnnotations(JavaTemplateParser.java:202)
        at org.openrewrite.java.internal.template.JavaTemplateJavaExtension$1.visitMethodDeclaration(JavaTemplateJavaExtension.java:281)
        at org.openrewrite.java.internal.template.JavaTemplateJavaExtension$1.visitMethodDeclaration(JavaTemplateJavaExtension.java:56)
        at org.openrewrite.java.tree.J$MethodDeclaration.acceptJava(J.java:3670)
        at org.openrewrite.java.tree.J.accept(J.java:59)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:184)
        at org.openrewrite.java.JavaTemplate.apply(JavaTemplate.java:67)
        at org.openrewrite.java.JavaTemplate.apply(JavaTemplate.java:108)
        at org.openrewrite.staticanalysis.MissingOverrideAnnotation$MissingOverrideAnnotationVisitor.visitMethodDeclaration(MissingOverrideAnnotation.java:86)
        at org.openrewrite.staticanalysis.MissingOverrideAnnotation$MissingOverrideAnnotationVisitor.visitMethodDeclaration(MissingOverrideAnnotation.java:70)
        at org.openrewrite.staticanalysis.MissingOverrideAnnotation_MissingOverrideAnnotationVisitor_KotlinVisitor.visitMethodDeclaration(MissingOverrideAnnotation_MissingOverrideAnnotationVisitor_KotlinVisitor.zig:123)
        at org.openrewrite.java.tree.J$MethodDeclaration.acceptJava(J.java:3670)
        at org.openrewrite.java.tree.J.accept(J.java:59)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
        at org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1375)
        at org.openrewrite.java.JavaVisitor.lambda$visitBlock$4(JavaVisitor.java:401)
        at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
        at org.openrewrite.java.JavaVisitor.visitBlock(JavaVisitor.java:400)
        at org.openrewrite.java.tree.J$Block.acceptJava(J.java:838)
        at org.openrewrite.java.tree.J.accept(J.java:59)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
        at org.openrewrite.java.JavaVisitor.visitClassDeclaration(JavaVisitor.java:488)
        at org.openrewrite.java.tree.J$ClassDeclaration.acceptJava(J.java:1290)
        at org.openrewrite.java.tree.J.accept(J.java:59)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
        at org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1375)
        at org.openrewrite.java.JavaVisitor.lambda$visitBlock$4(JavaVisitor.java:401)
        at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
        at org.openrewrite.java.JavaVisitor.visitBlock(JavaVisitor.java:400)
        at org.openrewrite.java.tree.J$Block.acceptJava(J.java:838)
        at org.openrewrite.java.tree.J.accept(J.java:59)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
        at org.openrewrite.java.JavaVisitor.visitClassDeclaration(JavaVisitor.java:488)
        at org.openrewrite.java.tree.J$ClassDeclaration.acceptJava(J.java:1290)
        at org.openrewrite.java.tree.J.accept(J.java:59)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
        at org.openrewrite.kotlin.KotlinVisitor.lambda$visitCompilationUnit$2(KotlinVisitor.java:57)
        at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
        at org.openrewrite.kotlin.KotlinVisitor.visitCompilationUnit(KotlinVisitor.java:56)
        at org.openrewrite.kotlin.tree.K$CompilationUnit.acceptKotlin(K.java:246)
        at org.openrewrite.kotlin.tree.K.accept(K.java:60)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:184)
        at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$5(RecipeRunCycle.java:164)
        at io.micrometer.core.instrument.AbstractTimer.recordCallable(AbstractTimer.java:147)
        at org.openrewrite.table.RecipeRunStats.recordEdit(RecipeRunStats.java:68)
        at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$6(RecipeRunCycle.java:161)
        at org.openrewrite.scheduling.RecipeStack.reduce(RecipeStack.java:57)
        at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$7(RecipeRunCycle.java:134)
        at org.openrewrite.internal.InMemoryLargeSourceSet.lambda$edit$0(InMemoryLargeSourceSet.java:62)
        at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
        at org.openrewrite.internal.InMemoryLargeSourceSet.edit(InMemoryLargeSourceSet.java:61)
        at org.openrewrite.scheduling.RecipeRunCycle.editSources(RecipeRunCycle.java:133)
        at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:86)
        at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
        at org.openrewrite.Recipe.run(Recipe.java:344)
        at org.openrewrite.Recipe.run(Recipe.java:340)
        at org.openrewrite.Recipe.run(Recipe.java:336)
        at org.apache.jmeter.buildtools.openrewrite.OpenRewriteWork.listResults(OpenRewriteWork.kt:291)
        at org.apache.jmeter.buildtools.openrewrite.OpenRewriteWork.execute(OpenRewriteWork.kt:162)
        at app//org.gradle.workers.internal.DefaultWorkerServer.execute(DefaultWorkerServer.java:63)
        at app//org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:54)
        at app//org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:48)
        at app//org.gradle.internal.classloader.ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:100)
        at app//org.gradle.workers.internal.AbstractClassLoaderWorker.executeInClassLoader(AbstractClassLoaderWorker.java:48)
        at app//org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:49)
        at app//org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:30)
        at app//org.gradle.workers.internal.WorkerDaemonServer.run(WorkerDaemonServer.java:96)
        at app//org.gradle.workers.internal.WorkerDaemonServer.run(WorkerDaemonServer.java:65)
        at app//org.gradle.process.internal.worker.request.WorkerAction$1.call(WorkerAction.java:138)
        at app//org.gradle.process.internal.worker.child.WorkerLogEventListener.withWorkerLoggingProtocol(WorkerLogEventListener.java:41)
        at app//org.gradle.process.internal.worker.request.WorkerAction.lambda$run$0(WorkerAction.java:135)
        at app//org.gradle.internal.operations.CurrentBuildOperationRef.with(CurrentBuildOperationRef.java:80)
        at app//org.gradle.process.internal.worker.request.WorkerAction.run(WorkerAction.java:127)
        at [email protected]/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at [email protected]/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at [email protected]/java.lang.reflect.Method.invoke(Method.java:568)
        at app//org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
        at app//org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
        at app//org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182)
        at app//org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164)
        at app//org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:414)
        at app//org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
        at app//org.gradle.internal.concurrent.AbstractManagedExecutor$1.run(AbstractManagedExecutor.java:47)
        at [email protected]/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at [email protected]/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at [email protected]/java.lang.Thread.run(Thread.java:840)
Caused by: java.lang.NullPointerException: Cannot invoke "org.openrewrite.java.tree.J$Block.getStatements()" because the return value of "org.openrewrite.java.tree.J$ClassDeclaration.getBody()" is null
        at org.openrewrite.kotlin.internal.KotlinPrinter$KotlinJavaPrinter.visitClassDeclaration0(KotlinPrinter.java:675)
        at org.openrewrite.kotlin.internal.KotlinPrinter$KotlinJavaPrinter.visitClassDeclaration(KotlinPrinter.java:646)
        at org.openrewrite.kotlin.internal.KotlinPrinter$KotlinJavaPrinter.visitClassDeclaration(KotlinPrinter.java:482)
        at org.openrewrite.java.tree.J$ClassDeclaration.acceptJava(J.java:1290)
        at org.openrewrite.java.tree.J.accept(J.java:59)
        at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
        ... 108 more
@knutwannheden
Copy link
Contributor

Yes, what you have identified here is indeed problematic. I've made the code more robust there, by not passing in a null to withBody().

The MissingOverrideAnnotation recipe would currently not work correctly anyway for Kotlin, as it adds an @Override annotation. For Kotlin it would have to add the override keyword. I will for now just change it, so it doesn't modify Kotlin sources, as I think the Kotlin compiler would complain anyway, if that is missing.

@knutwannheden
Copy link
Contributor

@vlsi Thanks for reporting!

@knutwannheden
Copy link
Contributor

Note: We try to explicitly declare all parameters as non-null (using our own @NonNullApi annotation using JSR-305 meta annotations). Unfortunately this doesn't work for methods generated by Lombok (e.g. with methods). We could of course make that explicit, but that would also be ugly:

        @With(onParam_ = @NonNull)
        @Getter
        Block body;

@knutwannheden
Copy link
Contributor

@knutwannheden , do you think it makes sense to fix the other two withBody(null) calls?

Yes, that would definitely make sense. I will look into it.

@vlsi
Copy link
Author

vlsi commented Jan 11, 2024

We try to explicitly declare all parameters as non-null (using our own @NonNullApi annotation using JSR-305 ..

That is strange. I guess you have @NonNullApi at the package level, so it should apply to all the methods within the package. It would be weird to annotate every method with @NonNull.

For reference, the decompiled code is

        @NonNull
        public @NonNull ClassDeclaration withBody(final Block body) {
            return this.body == body ? this : new ClassDeclaration(this.padding, this.annotations, this.id, this.prefix, this.markers, this.leadingAnnotations, this.modifiers, this.kind, this.name, this.typeParameters, this.primaryConstructor, this.extendings, this.implementings, this.permitting, body, this.type);
        }

Kotlin already rejects withBody(null):

lateinit var cu: J.ClassDeclaration
cu.withBody(null) // Null can not be a value of a non-null type J.Block

So if you miss null can't be passed to withBody(...) inspection, it might be an issue with the IDE.

@knutwannheden
Copy link
Contributor

Interesting. I would have to look at the byte code. But if I declare the @With annotation as shown or add an explicit @NonNull annotation to the field, it works. Possibly this is due to projectlombok/lombok#2654

@knutwannheden
Copy link
Contributor

I can at least confirm that in my IDEA (2023.3.2) it behaves this way. For Java calls our @NonNullApi at the package (or class level) has no effect on the parameter of the Lombok generated with methods, while for calls from Kotlin it does appear to make a difference.

@knutwannheden
Copy link
Contributor

https://github.com/openrewrite/rewrite/blob/c625f30ca32f8af7a196dd3b00b720919db22dcf/rewrite-java/src/main/java/org/openrewrite/java/internal/template/AnnotationTemplateGenerator.java#L165
https://github.com/openrewrite/rewrite/blob/c625f30ca32f8af7a196dd3b00b720919db22dcf/rewrite-java/src/main/java/org/openrewrite/java/internal/template/AnnotationTemplateGenerator.java#L198

Please note that these are calls to J.NewClass#withBody() and J.MethodDeclaration#withBody() which both actually accept null (constructor calls which don't create anonymous subclasses and default / abstract methods).

@knutwannheden
Copy link
Contributor

I found this IDEA bug: https://youtrack.jetbrains.com/issue/IDEA-278199/IntelliJ-does-not-apply-NonnullByDefault-to-bytecode. I still don't understand why Java and Kotlin behave differently here.

@vlsi
Copy link
Author

vlsi commented Jan 11, 2024

Technically speaking, jsr305 is deprecated, and it should better be replaced with a better-designed jspecify: @NullMarked at the package level + @Nullable at the relevant locations.
So I wouldn't spend my time on jsr305 anymore.

@knutwannheden
Copy link
Contributor

Yes, JSpecify looks nice and has gained quite some traction. While I doubt it would mak much difference here, it could indeed be worth investigating as an alternative. Thanks for bringing this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser-kotlin
Projects
Archived in project
Development

No branches or pull requests

3 participants