Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-4095

Labeled statements have wrong source position

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.1
    • Fix Version/s: 2.6.0-alpha-1
    • Component/s: parser
    • Labels:
      None

      Description

      Example:

      mylabel:
      foo()
      

      Source position of this statement is [1,8]-[1,9], which is the position of the ':' token. Same for all other labeled statements I've checked.

        Issue Links

          Activity

          Hide
          roshandawrani Roshan Dawrani added a comment -

          After the GROOVY-4071 fix, the labeled asserts don't get rendered correctly.
          For the code snippet

          mylabel:
          assert true == false
          

          Here is how it looks now:

          Caught: Assertion failed: 
          
          :
          
          
          	at Try.run(Try.groovy:2)
          

          Should a separate JIRA be opened for this assert message rendering issue?

          Show
          roshandawrani Roshan Dawrani added a comment - After the GROOVY-4071 fix, the labeled asserts don't get rendered correctly. For the code snippet mylabel: assert true == false Here is how it looks now: Caught: Assertion failed: : at Try.run(Try.groovy:2) Should a separate JIRA be opened for this assert message rendering issue?
          Hide
          roshandawrani Roshan Dawrani added a comment -

          As noted in GROOVY-4071, both the source location and message rendering issues are interlinked and once we get the source position right on the AssertStatement, the error message should get its empty portions filled again.

          Show
          roshandawrani Roshan Dawrani added a comment - As noted in GROOVY-4071 , both the source location and message rendering issues are interlinked and once we get the source position right on the AssertStatement, the error message should get its empty portions filled again.
          Hide
          roshandawrani Roshan Dawrani added a comment -

          Hi Jochen, you seem to be back on the mailing lists, so here is a pending question for you.

          I was comparing the source position of assert statement case with some other statements and noticed that for other cases the position of the ':' token was getting set on the statement but the actual position was getting carried by the expressions inside them. Whereas in case of AssertStatement, it was having an impact because Power-Assert code uses the source position from the AssertStatement itself, which is wrong.

          So, by comparison, one potential solution could be to have a AssertExpression wrapped inside a AssertStatement, and make assert code use the source position from AssertExpression instead of AssertExpression.

          Do you think it is a worthwhile thing to try that change? Or, do you see a simpler solution?

          Show
          roshandawrani Roshan Dawrani added a comment - Hi Jochen, you seem to be back on the mailing lists, so here is a pending question for you. I was comparing the source position of assert statement case with some other statements and noticed that for other cases the position of the ':' token was getting set on the statement but the actual position was getting carried by the expressions inside them. Whereas in case of AssertStatement, it was having an impact because Power-Assert code uses the source position from the AssertStatement itself, which is wrong. So, by comparison, one potential solution could be to have a AssertExpression wrapped inside a AssertStatement, and make assert code use the source position from AssertExpression instead of AssertExpression. Do you think it is a worthwhile thing to try that change? Or, do you see a simpler solution?
          Hide
          pniederw Peter Niederwieser added a comment -

          > So, by comparison, one potential solution could be to have a AssertExpression wrapped inside a AssertStatement, and make assert code use the source position from AssertExpression instead of AssertExpression.
          Not sure what you are saying here, but why not just fix the source position of labeled statements? It's never good to have wrong source positions in the AST.

          Show
          pniederw Peter Niederwieser added a comment - > So, by comparison, one potential solution could be to have a AssertExpression wrapped inside a AssertStatement, and make assert code use the source position from AssertExpression instead of AssertExpression. Not sure what you are saying here, but why not just fix the source position of labeled statements? It's never good to have wrong source positions in the AST.
          Hide
          roshandawrani Roshan Dawrani added a comment -

          For now, I am just checking it as an alternative to avoid modifying the ANTLR AST to groovy AST conversion code. I haven't checked it yet, but I think there could be an overlap in the AST formation/conversion code with the map key:value scenario, and I want the changes to be as confined as possible.

          Show
          roshandawrani Roshan Dawrani added a comment - For now, I am just checking it as an alternative to avoid modifying the ANTLR AST to groovy AST conversion code. I haven't checked it yet, but I think there could be an overlap in the AST formation/conversion code with the map key:value scenario, and I want the changes to be as confined as possible.
          Hide
          blackdrag Jochen Theodorou added a comment -

          What does making an expression fix the issue? Also such a change has the problem of possibly breaking code, or not?

          Show
          blackdrag Jochen Theodorou added a comment - What does making an expression fix the issue? Also such a change has the problem of possibly breaking code, or not?
          Hide
          roshandawrani Roshan Dawrani added a comment -

          @Jochen, If instead of AssertStatement we have ExpressionStatement wrapping a AssertExpression, then initially both AssertExpression/ExpressionStatement have the same source location, then when ":" token sets its source information on the ExpressionStatement, the wrapped AssertExpression remains isolated and still continues to have the correct source information. That's how it is working fine for various other labeled expressions.

          Regarding the possibility of breaking code, yes, it will impact the users that hook deep into groovy compiler.

          Let me know if you think that this direction of solving it is not worth exploring. The only reason was to avoid AST formation code because the place where ":" sets its source information on the following statement seems like a generic place.

          Show
          roshandawrani Roshan Dawrani added a comment - @Jochen, If instead of AssertStatement we have ExpressionStatement wrapping a AssertExpression, then initially both AssertExpression/ExpressionStatement have the same source location, then when ":" token sets its source information on the ExpressionStatement, the wrapped AssertExpression remains isolated and still continues to have the correct source information. That's how it is working fine for various other labeled expressions. Regarding the possibility of breaking code, yes, it will impact the users that hook deep into groovy compiler. Let me know if you think that this direction of solving it is not worth exploring. The only reason was to avoid AST formation code because the place where ":" sets its source information on the following statement seems like a generic place.
          Hide
          blackdrag Jochen Theodorou added a comment -

          Well, then no AssertStatement wrapping a AssertExpression would be needed, instead ExpressionStatement could warp it... still.. What you write here will essentially be a problem for all statements, or not? I prefer fixing the issue in the current setup, to not to break code

          Show
          blackdrag Jochen Theodorou added a comment - Well, then no AssertStatement wrapping a AssertExpression would be needed, instead ExpressionStatement could warp it... still.. What you write here will essentially be a problem for all statements, or not? I prefer fixing the issue in the current setup, to not to break code
          Hide
          roshandawrani Roshan Dawrani added a comment -

          AssertStatement wrapping a AssertExpression was a typo that I later corrected - I did mean ExpressionStatement doing the wrapping.

          A basic doubt - does the source information on the statement matter much? Let's take some example - of a ForStatement or BlockStatement or IfStatement, etc - is it not good enough in general if the expressions carry the source information correctly? When does the source info on the statements matter? Don't statements act as holders for expressions?

          It is surely correct that all the labeled statements are affected - but I couldn't reproduce a usage highlighting an issue with some other labeled statement - probably because ACG seems to be taking correct source information from the expressions, or because I am not clear where statement source info really matter.

          Show
          roshandawrani Roshan Dawrani added a comment - AssertStatement wrapping a AssertExpression was a typo that I later corrected - I did mean ExpressionStatement doing the wrapping. A basic doubt - does the source information on the statement matter much? Let's take some example - of a ForStatement or BlockStatement or IfStatement, etc - is it not good enough in general if the expressions carry the source information correctly? When does the source info on the statements matter? Don't statements act as holders for expressions? It is surely correct that all the labeled statements are affected - but I couldn't reproduce a usage highlighting an issue with some other labeled statement - probably because ACG seems to be taking correct source information from the expressions, or because I am not clear where statement source info really matter.
          Hide
          roshandawrani Roshan Dawrani added a comment -

          I am not trying now to push that we should go the AssertExpression way - just trying to understand how much and where Statement source info matters.

          May be it is best to solve this issue through some grammar or AST formation change.

          Show
          roshandawrani Roshan Dawrani added a comment - I am not trying now to push that we should go the AssertExpression way - just trying to understand how much and where Statement source info matters. May be it is best to solve this issue through some grammar or AST formation change.
          Hide
          pniederw Peter Niederwieser added a comment -

          > A basic doubt - does the source information on the statement matter much?
          I can't see why the source position of a statement should be less important than the source position of an expression.

          if {
          foo()
          } else {
          bar()
          }

          No one expression captures the extents of the statement.

          > When does the source info on the statements matter?
          For example, power asserts intentionally use the assert statement's source position s.t. the full statement appears in the output, and not just the boolean expression. Spock also uses statement source positions.

          >or because I am not clear where statement source info really matter
          As long as statements have a source position, it should be correct. Are you proposing to remove source position for statements?

          Show
          pniederw Peter Niederwieser added a comment - > A basic doubt - does the source information on the statement matter much? I can't see why the source position of a statement should be less important than the source position of an expression. if { foo() } else { bar() } No one expression captures the extents of the statement. > When does the source info on the statements matter? For example, power asserts intentionally use the assert statement's source position s.t. the full statement appears in the output, and not just the boolean expression. Spock also uses statement source positions. >or because I am not clear where statement source info really matter As long as statements have a source position, it should be correct. Are you proposing to remove source position for statements?
          Hide
          roshandawrani Roshan Dawrani added a comment -

          Peter - I am not saying one is less important or the other is more, nor I am proposing removal of source information from statements. Why do you have to read between the lines for no reason? I am just trying to become clearer about their use.

          My doubt is about what you totally forgot/skipped to explain in the if-else that you typed - that if source information of 'if boolean expression' or 'if block expressions', or 'else block expressions' are all intact - then I am just not clear how the source info on the if BlockStatement or else BlockStatement or IfStatement matters.

          I can ask questions about what I am not clear about, can't I?

          Show
          roshandawrani Roshan Dawrani added a comment - Peter - I am not saying one is less important or the other is more, nor I am proposing removal of source information from statements. Why do you have to read between the lines for no reason? I am just trying to become clearer about their use. My doubt is about what you totally forgot/skipped to explain in the if-else that you typed - that if source information of 'if boolean expression' or 'if block expressions', or 'else block expressions' are all intact - then I am just not clear how the source info on the if BlockStatement or else BlockStatement or IfStatement matters. I can ask questions about what I am not clear about, can't I?
          Hide
          roshandawrani Roshan Dawrani added a comment -

          Maybe it's too silly a question. Let's please move on. Maybe for tools/IDEs/other frameworks source information on statements is just no less important than the expressions. May for general programming usage, source information on expressions is more visible in error messages and so, and may be that's what was confusing me.

          I agree that it's best to get the source information correct on the statements. Let's move on to see how we can solve it now.

          Thanks.

          Show
          roshandawrani Roshan Dawrani added a comment - Maybe it's too silly a question. Let's please move on. Maybe for tools/IDEs/other frameworks source information on statements is just no less important than the expressions. May for general programming usage, source information on expressions is more visible in error messages and so, and may be that's what was confusing me. I agree that it's best to get the source information correct on the statements. Let's move on to see how we can solve it now. Thanks.
          Hide
          daniel_sun Daniel Sun added a comment -

          Fixed in the parrot branch

          Show
          daniel_sun Daniel Sun added a comment - Fixed in the parrot branch

            People

            • Assignee:
              daniel_sun Daniel Sun
              Reporter:
              pniederw Peter Niederwieser
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development