Clojure

Report warnings if *unchecked-math* and boxing happens

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.6
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Currently, it is difficult to tell that the compiler is using boxed math unless you look at the generated bytecode. The proposed enhancement here is to emit new warnings if *unchecked-math* is on and boxed math is occurring.

Approach: In the compiler, when compiling a StaticMethodExpr, if *unchecked-math* is true and the class is clojure.lang.Numbers and one of the parameters of static method is of type java.lang.Object or java.lang.Number, then emit a warning at compile-time.

In addition, there is a new WarnBoxedMath Java annotation - a small number of methods on Numbers with Object parameters use this annotation to indicate that warning should not take place. The same annotation can be (but is not currently) used to mark methods on Numbers without Object/Number params that should warn. See boxedmath.txt for a list of methods and categories.

Patch: clj-1325-v3.patch

Screened by:

  1. clj-1325-v3.patch
    16/May/14 11:51 AM
    11 kB
    Alex Miller
  2. clj-1325-v2.patch
    16/May/14 11:19 AM
    11 kB
    Alex Miller
  3. clj-1325.patch
    14/May/14 2:39 PM
    11 kB
    Alex Miller
  4. boxedmath.txt
    14/May/14 2:29 PM
    9 kB
    Alex Miller
  5. boxed.diff
    27/Mar/14 11:06 AM
    6 kB
    Alex Miller

Activity

Hide
Alex Miller added a comment -

Moving to 1.7.

Show
Alex Miller added a comment - Moving to 1.7.
Hide
Alex Miller added a comment -

List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.

Show
Alex Miller added a comment - List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.
Hide
Alex Miller added a comment -

Ready for screening.

Show
Alex Miller added a comment - Ready for screening.
Hide
Alex Miller added a comment -

clj-1325-v2.patch is identical to last except for a cleaned up the commit message.

Show
Alex Miller added a comment - clj-1325-v2.patch is identical to last except for a cleaned up the commit message.
Hide
Alex Miller added a comment -

Added v3 patch that just reworks block/indentation style to match surrounding code better.

Show
Alex Miller added a comment - Added v3 patch that just reworks block/indentation style to match surrounding code better.
Hide
Stuart Sierra added a comment -

Screened. Comments:

1) There is no way to get both overflow checks and boxed-math warnings at the same time. Maybe this doesn't matter.

2) The error messages aren't ideal, because they refer to clojure.lang.Numbers, but we can assume that anyone savvy enough to be using *unboxed-math* will also be savvy enough to know what clojure.lang.Numbers is.

3) This doesn't protect me from autoboxing in arbitrary Java method calls, but normal reflection warnings should catch most real-world cases, since few Java APIs overload on primitive and Object.

Show
Stuart Sierra added a comment - Screened. Comments: 1) There is no way to get both overflow checks and boxed-math warnings at the same time. Maybe this doesn't matter. 2) The error messages aren't ideal, because they refer to clojure.lang.Numbers, but we can assume that anyone savvy enough to be using *unboxed-math* will also be savvy enough to know what clojure.lang.Numbers is. 3) This doesn't protect me from autoboxing in arbitrary Java method calls, but normal reflection warnings should catch most real-world cases, since few Java APIs overload on primitive and Object.
Hide
Nicola Mometto added a comment -

With the new :warn-on-boxed, this code reports a warning:

user=> (defn f [x] (inc x))
Boxed math warning, NO_SOURCE_PATH:2:13 - call: public static java.lang.Number clojure.lang.Numbers.unchecked_in
#'user/f

but this does not:

user=> (defn f1 [^long x] (inc x))
#'user/f1

is this intentional?

Show
Nicola Mometto added a comment - With the new :warn-on-boxed, this code reports a warning:
user=> (defn f [x] (inc x))
Boxed math warning, NO_SOURCE_PATH:2:13 - call: public static java.lang.Number clojure.lang.Numbers.unchecked_in
#'user/f
but this does not:
user=> (defn f1 [^long x] (inc x))
#'user/f1
is this intentional?
Hide
Alex Miller added a comment -

The bytecode for those methods is:

f:
  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #34                 // Method clojure/lang/Numbers.unchecked_inc:(Ljava/lang/Object;)Ljava/lang/Number;
       6: areturn

f1: 
  public final java.lang.Object invokePrim(long);
    Code:
       0: lload_1
       1: invokestatic  #36                 // Method clojure/lang/Numbers.unchecked_inc:(J)J
       4: invokestatic  #40                 // Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
       7: areturn

I assume your question is why the call to Numbers.num(long) at the end doesn't cause the warning due to the return type? I had those num() calls in my early list of questionables. This function is tricky because it's called from lots of other methods (many of which already trigger the warning), so it has the potential to cause multiple warnings on a single expression. But this does indeed seem like a common and important case to suggest a return type hint.

Any of these calls that take prims but return a Number or Object require a judgement call and an explicit annotation - there is certainly room for interpretation on some of them.

Adding the return type hint cleans things up pretty well:

public final long invokePrim(long);
    Code:
       0: lload_1
       1: lconst_1
       2: ladd
       3: lreturn
Show
Alex Miller added a comment - The bytecode for those methods is:
f:
  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #34                 // Method clojure/lang/Numbers.unchecked_inc:(Ljava/lang/Object;)Ljava/lang/Number;
       6: areturn

f1: 
  public final java.lang.Object invokePrim(long);
    Code:
       0: lload_1
       1: invokestatic  #36                 // Method clojure/lang/Numbers.unchecked_inc:(J)J
       4: invokestatic  #40                 // Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
       7: areturn
I assume your question is why the call to Numbers.num(long) at the end doesn't cause the warning due to the return type? I had those num() calls in my early list of questionables. This function is tricky because it's called from lots of other methods (many of which already trigger the warning), so it has the potential to cause multiple warnings on a single expression. But this does indeed seem like a common and important case to suggest a return type hint. Any of these calls that take prims but return a Number or Object require a judgement call and an explicit annotation - there is certainly room for interpretation on some of them. Adding the return type hint cleans things up pretty well:
public final long invokePrim(long);
    Code:
       0: lload_1
       1: lconst_1
       2: ladd
       3: lreturn
Hide
Alex Miller added a comment -

I created CLJ-1585 for this.

Show
Alex Miller added a comment - I created CLJ-1585 for this.

People

Vote (1)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: