Bug 63174 - Type check in return value of around advice appears to be backwards ...
Summary: Type check in return value of around advice appears to be backwards ...
Status: NEW
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: 1.2   Edit
Hardware: All All
: P5 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Adrian Colyer CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-05-20 05:29 EDT by Laurie Hendren CLA
Modified: 2007-10-23 06:00 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Laurie Hendren CLA 2004-05-20 05:29:34 EDT
/* This small program exhibits a bug in the type checking of around advice.
 *  I think
 * the type check is backwards.   You will see that ADVICE1 matches and
 * ajc happily
 * compiles it,  causing a silent conversion from a double to int.
 * Conversly, ADVICE2 causes a compile-time error,  but should correctly match, 
 * and would cause an assignment of an int to a double, which is ok.
 */

public class Main {

  int f ( int i )
    {  return(i+1);
    }

  double g (double r)
    {  return(r + 1.0);
    }

  public static void main (String args[])
    {  Main m = new Main();
       int i = m.f(1);       // should match ADVICE2
       double d = m.g(2.7);  // should cause an error when matched with ADVICE1
    }
}

aspect Aspect {

  // matches a double return value and silently converts to an an int
  // Should trigger a compile-time error.

  // ADVICE1
  int around () : !within(Aspect) && call(double *(..))
  {
    int x=proceed(); //double value from proceed is getting assigned to an int, 
                     //should be an error
    System.out.println("AROUND1: x is " + thisJoinPointStaticPart + " " + x);
    return(x);

  }

  // gives an error                
  // Should correctly compile since an int is being assigned to a double,
  // which is correct
  // ADVICE2
  /*  ---- uncomment to see error that should not happen
  double around () : !within(Aspect) && call(int *(..))
  {
    double d=proceed();
    System.out.println("AROUND2: d is " + thisJoinPointStaticPart + " " + d);
    return(d);

  }
  --------*/

}
Comment 1 Adrian Colyer CLA 2004-05-20 08:30:41 EDT
This is working as designed, though it does raise an issue about conversion 
within the implementation of proceed that I'd like Jim to comment on. First let 
me explain why the implementation behaves as it does:

Take the ADVICE2 case first. This is around advice on a call join point. 
Somewhere out there is a caller who has written a statement like:

int i = m.f(1);

The caller is expecting an 'int' return value from the call. ADVICE2 declares 
around advice on the call that returns a double. This means the double return 
value would have to be converted to an int to give the caller a value of the 
correct type, and double to int conversion is not loss-less so the compiler 
complains.

In the case of ADVICE1 now, I hope you can see that the reverse is true. A 
caller is expecting a return value of type double from the call. The advice 
provides a value of type int. Since int can be promoted to double without loss 
the compiler is happy.

The final piece in the puzzle is that proceed "takes as arguments the context 
exposed by the around's pointcut, and returns whatever the around advice is 
declared to return." So in ADVICE1, since the advice is typed to return an int, 
the proceed is also typed to return an int, and the expression "int x = 
proceed();" is well-formed.

Now here's the issue that is raised by all this: *inside* of the proceed method 
that is generated for ADVICE1, a conversion occurs to convert the double 
returned by the call to the int returned by the proceed. Since around advice is 
allowed to change the return value, you can make a case (and this is what the 
current implementation does), that the programmer has asked for conversion to an 
int, and this is what the proceed is doing. On the flip side, the programmer may 
not have realised (intended) the consequences of the conversion (loss of 
precision) and so you can make a case that we should issue a compiler *warning* 
in this case. This would be a new lint option that we could support post 1.2.

I'm converting this to an enhancement request for consideration of implementing 
such a warning.
Comment 2 Adrian Colyer CLA 2005-03-22 09:05:18 EST
consider for AJ5 M4...
Comment 3 Adrian Colyer CLA 2005-10-28 05:58:39 EDT
moving to 1.5.1, we won't hold 1.5.0 for this.
Comment 4 Andrew Clement CLA 2006-04-04 14:03:39 EDT
didn't make 1.5.1 either!