groovy
  1. groovy
  2. GROOVY-3401

Ternary operator ?: does not handle newline before ":" gracefully

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.1, 1.9-beta-1, 1.7.11
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      0

      Description

      It seems like Groovy should be able to handle "Case 5" below,
      but instead it reports a compile-time error.

      //-----------------------
      //      Case 1:  OK
      //-----------------------
      bar =  0 ?  "moo" : "cow"
      
      
      //-----------------------
      //      Case 2:  OK
      //-----------------------
      bar =  0 ?
             "moo" : "cow"
      
      //-----------------------
      //      Case 3:  OK
      //-----------------------
      bar =  0 ?  "moo" :
             "cow"
      
      //-----------------------
      //      Case 4:  OK
      //-----------------------
      bar =  0 ?
             "moo" :
             "cow"
      
      //---------------------------------------------------
      //      Case 5:  ERROR.
      //
      //      Compiler says:
      //              expecting ':', found '<newline>'
      //---------------------------------------------------
      bar =  0 ? "moo"
               : "cow"
      
      
      //-----------------------------------------------------
      //      Case 6:  OK
      //
      //          Groovy cannot figure out that this is a
      //          valid ternary without the help of the '\'
      //          line continuation escape.  How come?
      //
      //-----------------------------------------------------
      bar =  0 ? "moo"        \
               : "cow"
      
      

        Issue Links

          Activity

          blackdrag blackdrag made changes -
          Field Original Value New Value
          Fix Version/s 1.7-beta-1 [ 14014 ]
          Guillaume Laforge made changes -
          Fix Version/s 1.7-beta-1 [ 14014 ]
          Fix Version/s 1.7-beta-x [ 15538 ]
          Hide
          blackdrag blackdrag added a comment -

          the general rule for Groovy is to use the operator at the end of the line or use backslash to break the line over. This applies to most operators, including for example +

          Show
          blackdrag blackdrag added a comment - the general rule for Groovy is to use the operator at the end of the line or use backslash to break the line over. This applies to most operators, including for example +
          blackdrag blackdrag made changes -
          Assignee Jochen Theodorou [ blackdrag ]
          Resolution Won't Fix [ 2 ]
          Fix Version/s 1.7-beta-x [ 15538 ]
          Status Open [ 1 ] Closed [ 6 ]
          Hide
          Jon Cox added a comment -

          Jochen,

          I understand the general rule, but here's a more specific one:
          the parser should keep going when there's an unfinished construct
          on the current line with only 1 sensible meaning. It already does
          this when there's an imbalanced [, {, or (. End-of-current-line
          won't be considered as a possible end-of-statement; instead the
          parser keeps pulling in tokens until the incomplete construct
          has either been completed, or is shown unsatisfiable.

          Doing something nice like this for the ternary ?: operator would
          help make it easier to visually scan code and know what you're
          looking at. It also makes it easier to cut/paste Java code.

          Consider what happens when the expressions are longer than the terse
          example I gave in case 5:

          //
          // Currently illegal (but highly readable)
          //
          somethingOrOther = (somethingOrOther.charAt(0) == '\n') ?  somethingOrOther.substring(1)
                                                                  :  somethingOrOther
          

          The benefit of being able to line up the "?" and ":" vertically is clear:
          it helps emphasize to the person reading the code that they're looking at
          the ternary ?: operator. If you have to push everything into 1 line,
          then the "?" and ":" tend to get lost visually.

          Compare that to the punctuation noise you get when a longish expression is put on 1 line:

          //
          // Legal but much harder see the  ?: at a glance
          //
          somethingOrOther =
            (somethingOrOther.charAt(0) == '\n') ? somethingOrOther.substring(1) : somethingOrOther
          

          Making this change seems backwards compatible because it only helps the
          compiler make sense of code that it chokes on today, and makes it
          more compatible with Java.

          Does this line of reasoning make you any more sympathetic to this
          bug report / feature request?

          Cheers,

          • Jon
          Show
          Jon Cox added a comment - Jochen, I understand the general rule, but here's a more specific one: the parser should keep going when there's an unfinished construct on the current line with only 1 sensible meaning. It already does this when there's an imbalanced [, {, or (. End-of-current-line won't be considered as a possible end-of-statement; instead the parser keeps pulling in tokens until the incomplete construct has either been completed, or is shown unsatisfiable. Doing something nice like this for the ternary ?: operator would help make it easier to visually scan code and know what you're looking at. It also makes it easier to cut/paste Java code. Consider what happens when the expressions are longer than the terse example I gave in case 5: // // Currently illegal (but highly readable) // somethingOrOther = (somethingOrOther.charAt(0) == '\n') ? somethingOrOther.substring(1) : somethingOrOther The benefit of being able to line up the "?" and ":" vertically is clear: it helps emphasize to the person reading the code that they're looking at the ternary ?: operator. If you have to push everything into 1 line, then the "?" and ":" tend to get lost visually. Compare that to the punctuation noise you get when a longish expression is put on 1 line: // // Legal but much harder see the ?: at a glance // somethingOrOther = (somethingOrOther.charAt(0) == '\n') ? somethingOrOther.substring(1) : somethingOrOther Making this change seems backwards compatible because it only helps the compiler make sense of code that it chokes on today, and makes it more compatible with Java. Does this line of reasoning make you any more sympathetic to this bug report / feature request? Cheers, Jon
          Jon Cox made changes -
          Resolution Won't Fix [ 2 ]
          Status Closed [ 6 ] Reopened [ 4 ]
          Hide
          blackdrag blackdrag added a comment -

          I change it to improvement then

          Show
          blackdrag blackdrag added a comment - I change it to improvement then
          blackdrag blackdrag made changes -
          Summary Ternary operator ?: does not handle newline beforej ":" gracefully (but it seems like it could/should) Ternary operator ?: does not handle newline before ":" gracefully
          Issue Type Bug [ 1 ] Improvement [ 4 ]
          Assignee Jochen Theodorou [ blackdrag ]
          Hide
          Marco Hunsicker added a comment -

          Looks like I didn't look thoroughly enough when submitting http://jira.codehaus.org/browse/GROOVY-4464

          I would really welcome the proposed change. With the current syntax restriction it's not possible to keep coding with the ternary operator like you're used to from Java, which is somewhat annoying. The backslash workaround is just that, a workaround - that should not be necessary.

          Why break compatibility with Java here? Are there any technical restrictions?

          Show
          Marco Hunsicker added a comment - Looks like I didn't look thoroughly enough when submitting http://jira.codehaus.org/browse/GROOVY-4464 I would really welcome the proposed change. With the current syntax restriction it's not possible to keep coding with the ternary operator like you're used to from Java, which is somewhat annoying. The backslash workaround is just that, a workaround - that should not be necessary. Why break compatibility with Java here? Are there any technical restrictions?
          Guillaume Laforge made changes -
          Link This issue is depended upon by GROOVY-4464 [ GROOVY-4464 ]
          Guillaume Laforge made changes -
          Link This issue is duplicated by GROOVY-4464 [ GROOVY-4464 ]
          Guillaume Laforge made changes -
          Link This issue is depended upon by GROOVY-4464 [ GROOVY-4464 ]
          Hide
          Guillaume Laforge added a comment -

          I had a look at that issue recently, and made a trivial change to the grammar to allow that syntax, but it created various grammar ambiguities, so I didn't pursue. But there's certainly a way to support the ability to loosen the newline rules a little here.

          Show
          Guillaume Laforge added a comment - I had a look at that issue recently, and made a trivial change to the grammar to allow that syntax, but it created various grammar ambiguities, so I didn't pursue. But there's certainly a way to support the ability to loosen the newline rules a little here.
          Hide
          Marco Hunsicker added a comment -

          Thanks for looking into this. As Java allows it and Groovy is derived from it a working Java grammar, I was thinking it should be possible to support this. The optional semi colons complicate grammar processing quite a bit, I guess.

          Show
          Marco Hunsicker added a comment - Thanks for looking into this. As Java allows it and Groovy is derived from it a working Java grammar, I was thinking it should be possible to support this. The optional semi colons complicate grammar processing quite a bit, I guess.
          Hide
          Tim Yates added a comment -

          I've probably got the wrong end of the stick, but isn't it a case of changing line 2687 of org/codehaus/groovy/antlr/groovy.g from:

                    | QUESTION^ nls! assignmentExpression[0] COLON! nls! conditionalExpression[0]
          

          to

                    | QUESTION^ nls! assignmentExpression[0] nls! COLON! nls! conditionalExpression[0]
          

          I added the following test to groovy/operator/TernaryOperatorsTest.groovy, and ant clean test didn't report any errors (and more-importantly didn't fail to compile with a expecting ':', found '<newline>' error)...

          void testLineBreaks() {
              def bar =  0 ?  "moo" : "cow"
              assert bar == 'cow'
          
              bar =  0 ?
                     "moo" : "cow"
              assert bar == 'cow'
          
              bar =  0 ?  "moo" :
                     "cow"
              assert bar == 'cow'
          
              bar =  0 ?
                     "moo" :
                     "cow"
              assert bar == 'cow'
          
              bar =  0 ? "moo"        \
                       : "cow"
              assert bar == 'cow'
          
              // This used to fail
              bar =  0 ? "moo"
                       : "cow"
              assert bar == 'cow'
          }
          

          This change probably breaks something I haven't seen (or affects performance) though... That tends to happen when I think I've got something cracked

          Show
          Tim Yates added a comment - I've probably got the wrong end of the stick, but isn't it a case of changing line 2687 of org/codehaus/groovy/antlr/groovy.g from: | QUESTION^ nls! assignmentExpression[0] COLON! nls! conditionalExpression[0] to | QUESTION^ nls! assignmentExpression[0] nls! COLON! nls! conditionalExpression[0] I added the following test to groovy/operator/TernaryOperatorsTest.groovy , and ant clean test didn't report any errors (and more-importantly didn't fail to compile with a expecting ':', found '<newline>' error)... void testLineBreaks() { def bar = 0 ? "moo" : "cow" assert bar == 'cow' bar = 0 ? "moo" : "cow" assert bar == 'cow' bar = 0 ? "moo" : "cow" assert bar == 'cow' bar = 0 ? "moo" : "cow" assert bar == 'cow' bar = 0 ? "moo" \ : "cow" assert bar == 'cow' // This used to fail bar = 0 ? "moo" : "cow" assert bar == 'cow' } This change probably breaks something I haven't seen (or affects performance) though... That tends to happen when I think I've got something cracked
          Hide
          Guillaume Laforge added a comment -

          I think that's what I tried and it was reporting an ambiguity in the grammar (near the beginning of the build).
          Can you redo a clean build and tell me if you don't see ambiguities at all in the grammar generation phase?

          Show
          Guillaume Laforge added a comment - I think that's what I tried and it was reporting an ambiguity in the grammar (near the beginning of the build). Can you redo a clean build and tell me if you don't see ambiguities at all in the grammar generation phase?
          Hide
          Tim Yates added a comment -

          Whether the change is there for groovy.g or not, it outputs:

          ensureGrammars:
              [antlr] ANTLR Parser Generator   Version 2.7.7 (20060906)   1989-2005
              [antlr] ANTLR Parser Generator   Version 2.7.7 (20060906)   1989-2005
              [antlr] /Users/tyates/Code/Groovy/GROOVYDEV/ternary_newline/groovy-core/src/main/org/codehaus/groovy/antlr/java/java.g:999: warning:Syntactic predicate superfluous for single alternative
              [antlr] /Users/tyates/Code/Groovy/GROOVYDEV/ternary_newline/groovy-core/src/main/org/codehaus/groovy/antlr/java/java.g:1455: warning:empty alternative makes no sense in (...)* or (...)+
          

          So I don't think this change neccessarially adds an ambiguity...

          Again, ANTLR is a bit of a dark-art to me, so I might be missing something glaringly obvious...

          Show
          Tim Yates added a comment - Whether the change is there for groovy.g or not, it outputs: ensureGrammars: [antlr] ANTLR Parser Generator Version 2.7.7 (20060906) 1989-2005 [antlr] ANTLR Parser Generator Version 2.7.7 (20060906) 1989-2005 [antlr] /Users/tyates/Code/Groovy/GROOVYDEV/ternary_newline/groovy-core/src/main/org/codehaus/groovy/antlr/java/java.g:999: warning:Syntactic predicate superfluous for single alternative [antlr] /Users/tyates/Code/Groovy/GROOVYDEV/ternary_newline/groovy-core/src/main/org/codehaus/groovy/antlr/java/java.g:1455: warning:empty alternative makes no sense in (...)* or (...)+ So I don't think this change neccessarially adds an ambiguity... Again, ANTLR is a bit of a dark-art to me, so I might be missing something glaringly obvious...
          Hide
          Guillaume Laforge added a comment -

          Yup, that's the ambiguities of the Java grammar which is okay.
          Perhaps I mistakenly took these for Groovy grammar ambiguities instead!
          I'll commit the changes soon, thanks a lot for investigating this.

          Show
          Guillaume Laforge added a comment - Yup, that's the ambiguities of the Java grammar which is okay. Perhaps I mistakenly took these for Groovy grammar ambiguities instead! I'll commit the changes soon, thanks a lot for investigating this.
          Guillaume Laforge made changes -
          Assignee Guillaume Laforge [ guillaume ]
          Hide
          Jon Cox added a comment -

          If Groovy had a Pet Peeve Cemetery, I'd dance on this one's grave.
          Thanks!

          Show
          Jon Cox added a comment - If Groovy had a Pet Peeve Cemetery, I'd dance on this one's grave. Thanks!
          Hide
          Tim Yates added a comment -

          It should be noted that this does not fix GROOVY-4464;

          script = script.exists()
              ? script
              : new File("${name}.groovy")
          

          is still invalid, as without EOL chars like java, I believe the parser would need to check for ? after almost every line of code, which I'm sure would be a huge performance hit (and I'm not even sure it's possible)

          Show
          Tim Yates added a comment - It should be noted that this does not fix GROOVY-4464 ; script = script.exists() ? script : new File( "${name}.groovy" ) is still invalid, as without EOL chars like java, I believe the parser would need to check for ? after almost every line of code, which I'm sure would be a huge performance hit (and I'm not even sure it's possible)
          Guillaume Laforge made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 1.7.11 [ 17244 ]
          Fix Version/s 1.8.1 [ 17223 ]
          Fix Version/s 1.9-beta-1 [ 17153 ]
          Resolution Fixed [ 1 ]
          Paul King made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Maxim Novikov added a comment -

          It still doesn't work in v.2.2.2.

          [Repro]
          def x = true
          ? 1
          : 0

          [Result]
          Crash with the error message: "unexpected token: ? @ line..."

          [Expected]
          No errors, variable x is equal to 1 after execution

          Show
          Maxim Novikov added a comment - It still doesn't work in v.2.2.2. [Repro] def x = true ? 1 : 0 [Result] Crash with the error message: "unexpected token: ? @ line..." [Expected] No errors, variable x is equal to 1 after execution
          Hide
          Tim Yates added a comment -

          Yeah the fix for that part is in 2.3.0

          https://jira.codehaus.org/browse/GROOVY-4464

          Show
          Tim Yates added a comment - Yeah the fix for that part is in 2.3.0 https://jira.codehaus.org/browse/GROOVY-4464

            People

            • Assignee:
              Guillaume Laforge
              Reporter:
              Jon Cox
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: