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

Handle erroneous nodes in open rewrite #4412

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

vudayani
Copy link

@vudayani vudayani commented Aug 13, 2024

What's changed?

The PR introduces handling for erroneous nodes in OpenRewrite. Currently, when there is an error in the LST, the parser may attempt to recover by ignoring or removing the erroneous node to produce a valid LST. The changes in this PR address this by adding a mechanism to handle these erroneous nodes instead of ignoring them, ensuring that they are preserved and correctly processed.

What's your motivation?

The motivation behind this PR is to handle scenarios where errors in the source code could lead to discrepancies in the computed results. When any recipe is executed, the edits are computed on a parsed LST without the error statement. By handling erroneous nodes, we ensure that the error lines are retained and reflected correctly in the LST, leading to more accurate edits and transformations.

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

This update is crucial for ensuring consistency in scenarios where error lines could affect the outcome of recipes and transformations.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@vudayani vudayani marked this pull request as ready for review August 13, 2024 17:44
@timtebeek timtebeek added the enhancement New feature or request label Aug 13, 2024
@timtebeek
Copy link
Contributor

Thanks for the continued work on this @vudayani ! I must say it's great to see this come in, also taking into account the out-of-bounds-delivered raw feedback. :) I've fixed a minor conflict with imports on main, and added two missing license headers.

Let us know if/when you consider this ready for review, or whether there's additional work you still plan to do.

@BoykoAlex
Copy link
Contributor

BoykoAlex commented Sep 11, 2024

I think this PR is ready for review and more feedback... Likely , there will be more work to do.
I see some checks failed below - I'll have a look at them...

@timtebeek
Copy link
Contributor

Thanks for the continued work on this @vudayani ! We're on to a downstream failure right now it seems.

`class org.openrewrite.java.tree.J$Erroneous cannot be cast to class org.openrewrite.java.tree.Expression` in `org.openrewrite.java.format.SpacesVisitor`.
AddToTagTest > addElementToSlashClosedTag() STANDARD_ERROR
    SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
    SLF4J: Defaulting to no-operation (NOP) logger implementation
    SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

> Task :rewrite-java-test:test

ChangeStaticFieldToMethodTest > migratesNestedFieldInitializer() FAILED
    java.lang.AssertionError: Failed to parse sources or run recipe
        at org.openrewrite.test.RewriteTest.lambda$defaultExecutionContext$14(RewriteTest.java:631)
        at org.openrewrite.test.RewriteTest$$Lambda$468/0x00007f97ec1beb50.accept(Unknown Source)
        at org.openrewrite.scheduling.RecipeRunCycle.handleError(RecipeRunCycle.java:260)
        at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$7(RecipeRunCycle.java:202)
        at org.openrewrite.scheduling.RecipeRunCycle$$Lambda$679/0x00007f97ec31fba8.apply(Unknown Source)
        at org.openrewrite.scheduling.RecipeStack.reduce(RecipeStack.java:57)
        at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$8(RecipeRunCycle.java:151)
        at org.openrewrite.scheduling.RecipeRunCycle$$Lambda$677/0x00007f97ec31f6e8.apply(Unknown Source)
        at org.openrewrite.internal.InMemoryLargeSourceSet.lambda$edit$0(InMemoryLargeSourceSet.java:66)
        at org.openrewrite.internal.InMemoryLargeSourceSet$$Lambda$678/0x00007f97ec31f948.apply(Unknown Source)
        at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
        at org.openrewrite.internal.InMemoryLargeSourceSet.edit(InMemoryLargeSourceSet.java:65)
        at org.openrewrite.RecipeScheduler$$Lambda$674/0x00007f97ec31ee20.apply(Unknown Source)
        at org.openrewrite.scheduling.RecipeRunCycle.editSources(RecipeRunCycle.java:150)
        at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:87)
        at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
        at org.openrewrite.Recipe.run(Recipe.java:376)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:371)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:125)
        at org.openrewrite.java.ChangeStaticFieldToMethodTest.migratesNestedFieldInitializer(ChangeStaticFieldToMethodTest.java:171)

        Caused by:
        org.openrewrite.internal.RecipeRunException: java.lang.ClassCastException: class org.openrewrite.java.tree.J$Erroneous cannot be cast to class org.openrewrite.java.tree.Expression (org.openrewrite.java.tree.J$Erroneous and org.openrewrite.java.tree.Expression are in unnamed module of loader 'app')
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:287)
            at app//org.openrewrite.java.format.SpacesVisitor.visit(SpacesVisitor.java:1127)
            at app//org.openrewrite.java.format.SpacesVisitor.visit(SpacesVisitor.java:31)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:147)
            at app//org.openrewrite.java.format.AutoFormatVisitor.visit(AutoFormatVisitor.java:73)
            at app//org.openrewrite.java.format.AutoFormatVisitor.visit(AutoFormatVisitor.java:33)
            at app//org.openrewrite.java.JavaVisitor.autoFormat(JavaVisitor.java:92)
            at app//org.openrewrite.java.JavaVisitor.autoFormat(JavaVisitor.java:86)
            at app//org.openrewrite.java.JavaVisitor.autoFormat(JavaVisitor.java:82)
            at app//org.openrewrite.java.internal.template.JavaTemplateJavaExtension$1.maybeReplaceStatement(JavaTemplateJavaExtension.java:471)
            at app//org.openrewrite.java.internal.template.JavaTemplateJavaExtension$1.visitMethodInvocation(JavaTemplateJavaExtension.java:423)
            at app//org.openrewrite.java.internal.template.JavaTemplateJavaExtension$1.visitMethodInvocation(JavaTemplateJavaExtension.java:55)
            at app//org.openrewrite.java.tree.J$MethodInvocation.acceptJava(J.java:3905)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:147)
            at app//org.openrewrite.java.JavaTemplate.apply(JavaTemplate.java:115)
            at app//org.openrewrite.java.ChangeStaticFieldToMethod$1.useNewMethod(ChangeStaticFieldToMethod.java:119)
            at app//org.openrewrite.java.ChangeStaticFieldToMethod$1.visitFieldAccess(ChangeStaticFieldToMethod.java:95)
            at app//org.openrewrite.java.ChangeStaticFieldToMethod$1.visitFieldAccess(ChangeStaticFieldToMethod.java:80)
            at app//org.openrewrite.java.tree.J$FieldAccess.acceptJava(J.java:1940)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1365)
            at app//org.openrewrite.java.JavaVisitor.lambda$visitContainer$37(JavaVisitor.java:1415)
            at app//org.openrewrite.java.JavaVisitor$$Lambda$655/0x00007f97ec2f3468.apply(Unknown Source)
            at app//org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
            at app//org.openrewrite.java.JavaVisitor.visitContainer(JavaVisitor.java:1415)
            at app//org.openrewrite.java.JavaVisitor.visitMethodInvocation(JavaVisitor.java:918)
            at app//org.openrewrite.java.tree.J$MethodInvocation.acceptJava(J.java:3905)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.visitLeftPadded(JavaVisitor.java:1393)
            at app//org.openrewrite.java.JavaVisitor.visitVariable(JavaVisitor.java:1299)
            at app//org.openrewrite.java.tree.J$VariableDeclarations$NamedVariable.acceptJava(J.java:5896)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1365)
            at app//org.openrewrite.java.JavaVisitor.lambda$visitVariableDeclarations$29(JavaVisitor.java:959)
            at app//org.openrewrite.java.JavaVisitor$$Lambda$796/0x00007f97ec497930.apply(Unknown Source)
            at app//org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
            at app//org.openrewrite.java.JavaVisitor.visitVariableDeclarations(JavaVisitor.java:959)
            at app//org.openrewrite.java.tree.J$VariableDeclarations.acceptJava(J.java:5785)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1365)
            at app//org.openrewrite.java.JavaVisitor.lambda$visitBlock$4(JavaVisitor.java:397)
            at app//org.openrewrite.java.JavaVisitor$$Lambda$648/0x00007f97ec2f7778.apply(Unknown Source)
            at app//org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
            at app//org.openrewrite.java.JavaVisitor.visitBlock(JavaVisitor.java:396)
            at app//org.openrewrite.java.tree.J$Block.acceptJava(J.java:838)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.visitMethodDeclaration(JavaVisitor.java:879)
            at app//org.openrewrite.java.tree.J$MethodDeclaration.acceptJava(J.java:3651)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1365)
            at app//org.openrewrite.java.JavaVisitor.lambda$visitBlock$4(JavaVisitor.java:397)
            at app//org.openrewrite.java.JavaVisitor$$Lambda$648/0x00007f97ec2f7778.apply(Unknown Source)
            at app//org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
            at app//org.openrewrite.java.JavaVisitor.visitBlock(JavaVisitor.java:396)
            at app//org.openrewrite.java.tree.J$Block.acceptJava(J.java:838)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.visitClassDeclaration(JavaVisitor.java:484)
            at app//org.openrewrite.java.ChangeStaticFieldToMethod$1.visitClassDeclaration(ChangeStaticFieldToMethod.java:87)
            at app//org.openrewrite.java.ChangeStaticFieldToMethod$1.visitClassDeclaration(ChangeStaticFieldToMethod.java:80)
            at app//org.openrewrite.java.tree.J$ClassDeclaration.acceptJava(J.java:1278)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:317)
            at app//org.openrewrite.java.JavaVisitor.lambda$visitCompilationUnit$10(JavaVisitor.java:497)
            at app//org.openrewrite.java.JavaVisitor$$Lambda$643/0x00007f97ec2f6270.apply(Unknown Source)
            at app//org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
            at app//org.openrewrite.java.JavaVisitor.visitCompilationUnit(JavaVisitor.java:497)
            at app//org.openrewrite.java.tree.J$CompilationUnit.acceptJava(J.java:1549)
            at app//org.openrewrite.java.tree.J.accept(J.java:59)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
            at app//org.openrewrite.TreeVisitor.visit(TreeVisitor.java:147)
            at app//org.openrewrite.Preconditions$Check.visit(Preconditions.java:175)
            at app//org.openrewrite.Preconditions$Check.visit(Preconditions.java:145)
            at app//org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$6(RecipeRunCycle.java:182)
            at app//org.openrewrite.scheduling.RecipeRunCycle$$Lambda$680/0x00007f97ec320000.call(Unknown Source)
            at app//io.micrometer.core.instrument.AbstractTimer.recordCallable(AbstractTimer.java:147)
            at app//org.openrewrite.table.RecipeRunStats.recordEdit(RecipeRunStats.java:67)
            at app//org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$7(RecipeRunCycle.java:178)
            ... 17 more

            Caused by:
            java.lang.ClassCastException: class org.openrewrite.java.tree.J$Erroneous cannot be cast to class org.openrewrite.java.tree.Expression (org.openrewrite.java.tree.J$Erroneous and org.openrewrite.java.tree.Expression are in unnamed module of loader 'app')
                at org.openrewrite.java.format.SpacesVisitor$$Lambda$968/0x00007f97ec4dd928.apply(Unknown Source)
                at org.openrewrite.internal.ListUtils.map(ListUtils.java:147)
                at org.openrewrite.java.format.SpacesVisitor.visitNewArray(SpacesVisitor.java:903)
                at org.openrewrite.java.format.SpacesVisitor.visitNewArray(SpacesVisitor.java:31)
                at org.openrewrite.java.tree.J$NewArray.acceptJava(J.java:4201)
                at org.openrewrite.java.tree.J.accept(J.java:59)
                at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:245)
                ... 110 more

@vudayani
Copy link
Author

vudayani commented Oct 3, 2024

Thanks for your patience @timtebeek
We fixed a couple of tests and are working on a few more. We will let you know once it's ready for review.

@vudayani
Copy link
Author

vudayani commented Oct 5, 2024

@timtebeek It is ready for review and all tests seem to pass locally. Thanks again for your patience, and sorry for the delay in getting back on this.

@timtebeek
Copy link
Contributor

Thanks a lot for all the hard work here @vudayani ! I've tagged Knut for help with review as I'm traveling, and I know Jonathan has been quite busy these past couple of weeks. I hope we can get this in before Wednesday's release, but that'll depend on their feedback as well.

import org.openrewrite.DataTable;
import org.openrewrite.Recipe;

public class CompileErrors extends DataTable<CompileErrors.Row> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expected data table rows to be inserted from FindCompileErrors, similar to the example here, but I'm not seeing any reference. Did I overlook something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed the change from @vudayani to fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Ready to Review
Development

Successfully merging this pull request may close these issues.

4 participants