rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard
verified publisher icon openrewrite

Handle erroneous nodes in open rewrite

Open vudayani opened this issue 1 year ago • 6 comments

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

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

vudayani avatar Aug 13 '24 17:08 vudayani

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.

timtebeek avatar Sep 06 '24 08:09 timtebeek

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...

BoykoAlex avatar Sep 11 '24 16:09 BoykoAlex

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

timtebeek avatar Sep 24 '24 21:09 timtebeek

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 avatar Oct 03 '24 17:10 vudayani

@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.

vudayani avatar Oct 05 '24 07:10 vudayani

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.

timtebeek avatar Oct 06 '24 13:10 timtebeek

Where are we with this @timtebeek ? What do you think? Is there anything specific missing before this can be merged? Would be awesome to finally push this forward into main... :-)

martinlippert avatar Oct 29 '24 13:10 martinlippert

Where are we with this @timtebeek ? What do you think? Is there anything specific missing before this can be merged? Would be awesome to finally push this forward into main... :-)

Thanks for the reminder @martinlippert ; I'd already tagged folks for additional review, but I've now also raised this on an internal channel, as I understand you're eager to get this in for your purposes, as are we to see this finalized.

timtebeek avatar Oct 29 '24 23:10 timtebeek

Thanks a lot @sambsnyd for taking a look, providing feedback, and for your approval here. Looks like we are waiting for approval from @jkschneider now, right? Would be great to finally get this in... :-)

martinlippert avatar Dec 09 '24 16:12 martinlippert