groovy

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

is duplicated by

Improvement - An improvement or enhancement to an existing feature or task. GROOVY-4464 Loosen white space restrictions

  • Minor - Minor loss of function, or other problem where easy workaround is present.
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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 +
Hide
Permalink
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
Hide
Permalink
blackdrag blackdrag added a comment -

I change it to improvement then

Show
blackdrag blackdrag added a comment - I change it to improvement then
Hide
Permalink
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?
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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.
Hide
Permalink
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
Permalink
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)

People

  • Assignee:
    Guillaume Laforge
    Reporter:
    Jon Cox
Vote (1)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: