groovy

Casts should have higher precedence than mathematical operators

Details

  • Type: Sub-task Sub-task
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.7.4
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Number of attachments :
    0

Description

The following test case executes successfully when compiled+executed as java. However it throws the exception "Cannot use mod() on this number type: java.math.BigDecimal with value: 10.7" when executed as groovy.

public class TesCastPrecedence {
    @org.junit.Test
    public void testCastWithMod() {
        long result = (long)10.7 % 3L;
        System.out.println(result);
        org.junit.Assert.assertEquals(result, 1L);
    }
}

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jonathan Pearlin added a comment -

The issue with the precedence of casts and binary operations produces other unexpected behavior/exceptions. Consider the following two examples:

def percentage = 5.3
(int)Math.floor(percentage) + "%" //Should produce "5%", but causes GroovyCastException, as the integer cast gets applied to the string instead of the result of calling floor()

def someInt = Integer.MAX_VALUE
(long)someInt + someInt //Should produce 4294967294, but produces -2 (overflow)

In both cases, wrapping with parentheses solves the problem, but should not be necessary.

Show
Jonathan Pearlin added a comment - The issue with the precedence of casts and binary operations produces other unexpected behavior/exceptions. Consider the following two examples: def percentage = 5.3 (int)Math.floor(percentage) + "%" //Should produce "5%", but causes GroovyCastException, as the integer cast gets applied to the string instead of the result of calling floor() def someInt = Integer.MAX_VALUE (long)someInt + someInt //Should produce 4294967294, but produces -2 (overflow) In both cases, wrapping with parentheses solves the problem, but should not be necessary.
Hide
Permalink
blackdrag blackdrag added a comment -

I guess that is actually a bug in our grammar and the ANTLR AST build from the rules. This is a bit funny, since that part should originate from the original java grammar we used and that means that it was probably wrong in there already. But I have not checked if we did there some changes of course... so maybe it is really our fault.... our task to fix it is anyway.

Show
blackdrag blackdrag added a comment - I guess that is actually a bug in our grammar and the ANTLR AST build from the rules. This is a bit funny, since that part should originate from the original java grammar we used and that means that it was probably wrong in there already. But I have not checked if we did there some changes of course... so maybe it is really our fault.... our task to fix it is anyway.
Hide
Permalink
Jonathan Pearlin added a comment -

I should add that we also tested the behavior in 1.6.x and we got the expected results (i.e. we did not see the issues listed above). We actually noticed the problem when upgrading a Grails application from 1.2.0 to 1.3.7 (i.e. Groovy 1.6.7 to 1.7.4) and knew that the only difference in the application was the version of Groovy (i.e. no code changes were made).

Show
Jonathan Pearlin added a comment - I should add that we also tested the behavior in 1.6.x and we got the expected results (i.e. we did not see the issues listed above). We actually noticed the problem when upgrading a Grails application from 1.2.0 to 1.3.7 (i.e. Groovy 1.6.7 to 1.7.4) and knew that the only difference in the application was the version of Groovy (i.e. no code changes were made).
Hide
Permalink
Jonathan Pearlin added a comment -

Correction...Grails 1.3.7 uses Groovy 1.7.8.

Show
Jonathan Pearlin added a comment - Correction...Grails 1.3.7 uses Groovy 1.7.8.
Hide
Permalink
blackdrag blackdrag added a comment -

ok, this is interesting... then I have to check the versions... 1.6.3 works, 1.6.9 works... that means the change was exclusive to trunk, Groovy 1.7.0 then already has the problem, Groovy 1.7-rc-1 as well, beta-1 as well... That means the change went most probably early into the 1.7 brunch, somewhere between the start of the branch and the first beta... a bit more research reveals a bug fix for GROOVY-2605 that may have caused the problem. There this was done:

 expression[int lc_stmt]
     :   
+        (LPAREN typeSpec[true] RPAREN expression[lc_stmt])=>
+            lp:LPAREN^ {#lp.setType(TYPECAST);} typeSpec[true] RPAREN!
+            expression[lc_stmt]
+    |        
        (LPAREN nls IDENT (COMMA nls IDENT)* RPAREN ASSIGN) =>
         m:multipleAssignment[lc_stmt] {#expression=#m;}
     |   assignmentExpression[lc_stmt]
     ;

This means the expression will be first parsed completely and then we will attach the cast. for (X)1+1 this means 1+1 is the expression and the cast will be then done on the result of the cast.

Now the big question is of course how to enable both

Show
blackdrag blackdrag added a comment - ok, this is interesting... then I have to check the versions... 1.6.3 works, 1.6.9 works... that means the change was exclusive to trunk, Groovy 1.7.0 then already has the problem, Groovy 1.7-rc-1 as well, beta-1 as well... That means the change went most probably early into the 1.7 brunch, somewhere between the start of the branch and the first beta... a bit more research reveals a bug fix for GROOVY-2605 that may have caused the problem. There this was done: expression[ int lc_stmt] : + (LPAREN typeSpec[ true ] RPAREN expression[lc_stmt])=> + lp:LPAREN^ {#lp.setType(TYPECAST);} typeSpec[ true ] RPAREN! + expression[lc_stmt] + | (LPAREN nls IDENT (COMMA nls IDENT)* RPAREN ASSIGN) => m:multipleAssignment[lc_stmt] {#expression=#m;} | assignmentExpression[lc_stmt] ; This means the expression will be first parsed completely and then we will attach the cast. for (X)1+1 this means 1+1 is the expression and the cast will be then done on the result of the cast. Now the big question is of course how to enable both
Hide
Permalink
blackdrag blackdrag added a comment -

the revision is 14727

Show
blackdrag blackdrag added a comment - the revision is 14727
Hide
Permalink
Kirk Rasmussen added a comment -

Mini-rant:

Come on guys this is embarrassing that this hasn't been fixed before 2.0 was released. How do you expect people to adopt Groovy if fundamental mathematical expressions don't work?!?

Show
Kirk Rasmussen added a comment - Mini-rant: Come on guys this is embarrassing that this hasn't been fixed before 2.0 was released. How do you expect people to adopt Groovy if fundamental mathematical expressions don't work?!?

People

  • Assignee:
    Unassigned
    Reporter:
    Jeffrey Adamson
Vote (3)
Watch (4)

Dates

  • Created:
    Updated: