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

HCL Parser is confused with comment lines that contain `// <comment>`

Open rahul3509 opened this issue 1 year ago • 4 comments

What version of OpenRewrite are you using?

I am using

  • OpenRewrite latest
  • rewrite-hcl latest

How are you running OpenRewrite?

I am using the Gradle plugin, and my project is a single module project.

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

package org.openrewrite.hcl.tree;

import org.junit.jupiter.api.Test;
import org.openrewrite.Issue;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.hcl.Assertions.hcl;

class HclCommentTest implements RewriteTest {

    @Test
    void comment() {
        rewriteRun(
          hcl(
            """
              # test
              /*
               multiline
              */
              resource {
                  # test
                  // test
                  a = 1 //test
              }
              """
          )
        );
    }
}

I see this error when is try to use //test next to an attribute value

What is the full stack trace of any errors you encountered?

org.opentest4j.AssertionFailedError: [When parsing and printing the source code back to text without modifications, the printed source didn't match the original source code. This means there is a bug in the parser implementation itself. Please open an issue to report this, providing a sample of the code that generated this error for "file.tf":
diff --git a/file.tf b/file.tf
index 54ff754..bfd3530 100644
--- a/file.tf
+++ b/file.tf
@@ -5,5 +5,4 @@ 
 resource {
     # test
     // test
-    a = 1 //test
-}
\ No newline at end of file
+    a = 1}
\ No newline at end of file
] 
expected: 
  "# test
  /*
   multiline
  */
  resource {
      # test
      // test
      a = 1 //test
  }"
 but was: 
  "# test
  /*
   multiline
  */
  resource {
      # test
      // test
      a = 1}"
	at app//org.openrewrite.test.RewriteTest.assertContentEquals(RewriteTest.java:615)
	at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:308)
	at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)
	at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:125)
	at app//org.openrewrite.hcl.tree.HclCommentTest.comment(HclCommentTest.java:28)
	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.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:766)
	at app//org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor$$Lambda$222/0x000000f001118200.apply(Unknown Source)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall$$Lambda$223/0x000000f001118620.apply(Unknown Source)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$$Lambda$416/0x000000f00114d378.apply(Unknown Source)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$8(TestMethodTestDescriptor.java:217)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor$$Lambda$427/0x000000f00114f850.execute(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:213)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:138)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:68)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:156)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$337/0x000000f00113cab8.execute(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:146)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$336/0x000000f00113c890.invoke(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:144)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$335/0x000000f00113c468.execute(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:143)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:100)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService$$Lambda$341/0x000000f00113d5d0.accept(Unknown Source)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:160)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$337/0x000000f00113cab8.execute(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:146)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$336/0x000000f00113c890.invoke(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:144)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$335/0x000000f00113c468.execute(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:143)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:100)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService$$Lambda$341/0x000000f00113d5d0.accept(Unknown Source)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:160)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$337/0x000000f00113cab8.execute(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:146)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$336/0x000000f00113c890.invoke(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:144)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$335/0x000000f00113c468.execute(Unknown Source)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:143)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:100)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
	at app//org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at app//org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
	at app//org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:107)
	at app//org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
	at app//org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
	at app//org.junit.platform.launcher.core.EngineExecutionOrchestrator$$Lambda$287/0x000000f001123db0.accept(Unknown Source)
	at app//org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
	at app//org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
	at app//org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
	at app//org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
	at app//org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:124)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:94)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:63)
	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 org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:92)
	at jdk.proxy1/jdk.proxy1.$Proxy4.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:200)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:132)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:103)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:63)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:121)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)

rahul3509 avatar Oct 02 '24 11:10 rahul3509

hi @rahul3509 ; thanks for the report! It looks like we did specify // for comments as well: https://github.com/openrewrite/rewrite/blob/6602fe7ddc634787070bd58c1aa96081fb6c6c7c/rewrite-hcl/src/main/antlr/HCLLexer.g4#L53-L54

The parser seems to stumble over adding those trailing comments, which is quite unfortunate. Did you already explore a possible fix?

timtebeek avatar Oct 16 '24 10:10 timtebeek

Hi @timtebeek, I am currently debugging the issue, i am new to antlr so most of my free time is going into understanding ANTLRv4. Although I have observed another odd behaviour,

`@Test
void inLineCommentsNextLineAttribute() {
    rewriteRun(
      hcl(
        """
          # test
          /*
           multiline
          */
          resource {
              # test
              // test
              a = 1
              // test 3
          }
          """
      )
    );
}`

This type of comment is also giving the same error. completely ignoring the // test 3 comment

rahul3509 avatar Oct 17 '24 05:10 rahul3509

Thanks for diving in! Looks like we're failing to pick up on trailing comments. I'm traveling right now so I can't look closely, but do keep me posted about your progress! I should be back by Monday.

timtebeek avatar Oct 17 '24 05:10 timtebeek

While debugging I observed that comments are added to prefix object where it considers both whitespaces and comments as equal probability of occurrence. Forwarding both white spaces and comments to space.java and rest of the HCL code is added to body object, right now I am looking into this piece of code will keep you posted. Happy journey.

rahul3509 avatar Oct 17 '24 05:10 rahul3509

I was able to pin point the methods responsible to identify comments, as i mentioned in the previous comment prefix object is what stores the comments, this code here https://github.com/openrewrite/rewrite/blob/6602fe7ddc634787070bd58c1aa96081fb6c6c7c/rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/HclParserVisitor.java#L689 the convert method and prefix method are the once that process the tokens and identify what prefix and sending the prefix to space.java, I was able to identify that the tokens related to the trailing comments are getting skipped. Will look into it.

rahul3509 avatar Oct 23 '24 03:10 rahul3509

Great to follow your progress here @rahul3509 Sounds like you're on the right track. Look forward to what you'll come up with.

timtebeek avatar Oct 23 '24 07:10 timtebeek

Hey @timtebeek
https://github.com/openrewrite/rewrite/blob/6602fe7ddc634787070bd58c1aa96081fb6c6c7c/rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/HclParserVisitor.java#L180 here there is a call for the spaces and comments present after the a=1 and https://github.com/openrewrite/rewrite/blob/6602fe7ddc634787070bd58c1aa96081fb6c6c7c/rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/HclParserVisitor.java#L717 This function will get the source before some delimiter, not when it gives the sourceBefore function the call for '}' end of the block and cursor for or after 'a=1' it is not able to find '}' and returning an space.Empty you can find that case in line number 720 Now I am most certain that there is problem with positionOfNext method which will find the delimiter index. Will investigate further

rahul3509 avatar Oct 24 '24 07:10 rahul3509

hi @timtebeek , can you tell me why case '//': is empty. And also when the value is '//' it is matching with '/*' which shouldn't happen. image

rahul3509 avatar Oct 24 '24 11:10 rahul3509

I think you're on to the issue there; I don't have more insights, but I would expect that case "//" to set inSingleLineComment = true, probably just as you would. Are you seeing any improvement making that change?

timtebeek avatar Oct 24 '24 11:10 timtebeek

worked, I think we fixed the issue. One weird thing that is happening here in switch case is that. when source.substring(delimIndex, delimIndex + 2) value is // but it matches with case '/* and starts to execute

inMultiLineComment = true; delimIndex++; break;

rahul3509 avatar Oct 24 '24 12:10 rahul3509

How do i verify that it doesn't break anything else? will running all the tests be enough?

rahul3509 avatar Oct 24 '24 12:10 rahul3509

Great! Thanks again for diving in. Running all the tests is a good start. Beyond that we can run against a few large projects to test

timtebeek avatar Oct 24 '24 12:10 timtebeek

Can you tell what are the next steps to do? All tests passed.

rahul3509 avatar Oct 24 '24 12:10 rahul3509

Great! Now would be the time to push a commit to a branch on your fork at https://github.com/rahul3509/rewrite, and then follow the prompts to create a pull request here. I'll review and polish up your change from there. Let me know if you need help with any of those steps; here's a doc if this is your first time creating a pull request against OSS. Thanks a lot!

timtebeek avatar Oct 25 '24 14:10 timtebeek

I've pushed up a PR just now crediting you for the fix @rahul3509 ; Figured easiest to see this resolved quickly as @mccartney was interested in a fix to this and other issues as well.

  • https://github.com/openrewrite/rewrite/pull/4616

timtebeek avatar Oct 26 '24 22:10 timtebeek