This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 237553 - Wrong hint for byte/short literals
Summary: Wrong hint for byte/short literals
Status: RESOLVED FIXED
Alias: None
Product: java
Classification: Unclassified
Component: Hints (show other bugs)
Version: 7.4
Hardware: PC All
: P3 normal with 1 vote (vote)
Assignee: Svata Dedic
URL:
Keywords: REGRESSION
: 239984 240646 (view as bug list)
Depends on: 229951
Blocks:
  Show dependency tree
 
Reported: 2013-10-23 13:27 UTC by unai
Modified: 2014-03-06 16:20 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Screenshot taken before applying the lightbulb-suggested refactorings (26.51 KB, image/png)
2013-10-24 19:19 UTC, unai
Details
Screenshot taken after applying the lightbulb-suggested refactorings (23.85 KB, image/png)
2013-10-24 19:20 UTC, unai
Details
Screenshot about the (short)32000, taken before refactoring (11.65 KB, image/png)
2013-10-24 19:34 UTC, unai
Details
Screenshot about the (short)32000, taken after refactoring (7.70 KB, image/png)
2013-10-24 19:34 UTC, unai
Details

Note You need to log in before you can comment on or make changes to this bug.
Description unai 2013-10-23 13:27:11 UTC
[ BUILD # : 201310111528 ]
[ JDK VERSION : 1.7.0_40 ]

For byte and short literals, such as
(byte)0xA1
(short)32000
etc
you get a "Unnecessary cast to byte/short" hint (wrong!).

If you apply the suggested refactoring (Remove redundant cast to byte/short),
then you (obviously) get the "incompatible types: possible lossy conversion
from int to byte/short" warning, whenever you pass that literal as an actual
parameter to a function accepting byte/short parameters.
Comment 1 Jiri Prox 2013-10-23 21:04:35 UTC
Duplicate of issue 237325?
Comment 2 unai 2013-10-23 21:33:25 UTC
Are you sure this is a duplicate of issue 237325 (marked as resolved!)?

Svata Dedic commented:
"StringBuffer does not have a char constructor, but if char is used in new expression, it is silently promoted to int and the int-constructor is called in fact. 
The user who writes new StringBuffer('f') is very likely to expect that the SB will be initialized with 'f' as the initial content. Instead of that, the SB will receive the (int)'f' as its initial capacity - that's why the warning exist."

But here the warning appears *not* where a silent to-int promotion would be implicit, but where an actual byte/short is expected.
E.g.:

short s=(short)1234;//warning
byte b=(byte)255;//warning

And:

fun((byte)1);//warning
[...]
void printB(byte b)
{
    System.out.println(b&0xFF);
}

Then if you apply the suggested refactoring you get:

fun(1);//warning about loss of precision, we have an unwanted int-to-byte demotion here (not the other way around)
[...]
void printB(byte b)
{
    System.out.println(b&0xFF);
}
Comment 3 unai 2013-10-23 21:35:42 UTC
Forgot to mention that NB 7.3.1 didn't have this bug
Comment 4 Svata Dedic 2013-10-24 14:56:07 UTC
Re. comment #1 & #2: not a duplicate.

Re. comment #2: I probably do not understand your examples; 

short s=(short)1234; // the warning is _correct_ here
byte b=(byte)255; // warning should not be here

According to JLS7, par. 5.2. "Assignment Conversion", if a value is a compile-time constant of type int (the presented literals are):
"A narrowing primitive conversion may be used if the type of the variable is byte, short, or char, and the value of the constant expression is representable in the type of the variable."

While 1234 is representable as short, 255 is not representable as byte (-128..127).

I need more context for:
(byte)0xA1
(short)32000

in a simple assignment, these do not generate the warning. If the literal (casted) is used in method invocation as a parameter, the warning also does not appear.
Comment 5 unai 2013-10-24 19:18:42 UTC
I don't think there should be any warning here:

I want to assign a *short literal* to my short variable, so

short s=(short)1234;//should be fine without any hint whatsoever.

Because if I wrote

short s=1234;

we have an *int* there on the right, that the compiler has to implicitly cast to short in order to store it into s.

Any literal, if not explicitly casted, is of type int, if you want bytes or shorts you should use the '(byte)123' and '(short)1234 syntax.

With

short s=1234;
NB7.3 used to warn us of possible loss of precision, and he was kinda right, you should be warned against inserting a value of one primitive type right into a variable of *another* type (no matter if he can statically see that 1234 [remember, it's an int literal] would fit nonetheless.

I have always seen '(byte)123' as a *byte literal*, and NOT as a int-casted-to-byte. 
OR, if you look at it the other way, Being that literals made of simple digits (with no modifiers such as 'L', '.', etc) are always of type int, it doesn't matter if such integer is over/below 127 or 32767, it's still an int that wants to go where a byte/short is requested, so you have to convert that literal to the right type.
*BUT* you got me with that JLS quote, so I won't say anything more, for that is the holy book :)
And JLS is pretty clear stating that there is no such thing as byte or short literals, but just ints, implicitly casted to the smaller type if their literal value is below the max value of the expected type (one too many glasses of wine when writing that paragraph of the spec!).

The issue here is that we can't be warned in BOTH cases, i.e. there is no way to get rid of the warnings, and the suggested refactorings bring you in a situation with new warnings.

(NB 7.3.1 behaviour used to be flawless though.)

Having

void foo(byte b)
{}

and calling

foo(0);//Note that the literal 0 is of type int
This is wrong, passing in the wrong type (smaller, "possible loss of precision, well even if at compile time we know 0 is ok -- but I could change it with an int variable tomorrow... So only then I should ad the extra (byte) cast?
In fact, NB 7.4 itself gives you a "loss of precision" warning in foo(0);
But it ALSO gives you a warning if you have foo((byte)0) so my point is that you have a warning in any case!

Best behaviour was NB7.3.1's (and from at least NB 5 as far as I can recall) where it was consistent with the notion that:
0 -1 200 100000 <- these are in literals
(byte)0 (byte)1 (byte)200 (byte)100000 <- these are byte literals (note that (byte)200 takes only the lower 8 bits, so you get an unsigned 200 [if you think of it as an unsigned byte or if you later read it back to int bitwise-anding it with 0xFF] or a "purposefully-overflowed" -56). But this might be not compliant with JLS7 (therefore NB7.3 wasn't JLS compliant).
Same with shorts...

"I need more context for:
(byte)0xA1
(short)32000"

Here you go: basically you get a warning wherever the two literals above appear in the code. E.g.:

static void foo(byte b){}
static void bar(short s){}
public static void main(String[] args)
{
    foo(0);
//ERROR on foo(0);: incompatible types: possible lossy conversion from int to byte
    bar(0);
//ERROR on bar(0);: incompatible types: possible lossy conversion from int to short
    foo(0xA1);
//WARNING just on 0xA1: incompatible types: possible lossy conversion from int to byte
    bar(32000);
//ERROR on bar(32000): incompatible types: possible lossy conversion from int to short
    bar(33333);
//WARNING just on 33333: incompatible types: possible lossy conversion from int to short
    foo((byte) 1);
//HINT on the byte keyword: unnecessary cast to byte
    bar((short) 1);
//HINT on the short keyword: unnecessary cast to byte
    foo((byte) 0xA1);//OK
    bar((short) 0xA1);
//HINT on the short keyword: unnecessary cast to byte
    bar((short) 33333);//OK
}

A total mess...
There are no refactoring suggested for the errors, but there are for the warning and the hints, so here's what happens if we apply such lightbulb-click refactorings.

From:
    foo((byte) 1);
//HINT on the byte keyword: unnecessary cast to byte
To (applying the suggested "remove redundant cast to byte" refactoring:
    foo(1);
//ERROR on foo(1);: incompatible types: possible loss of precision from int to byte

From:
    bar((short) 1);
//HINT on the short keyword: unnecessary cast to byte
To (applying the suggested "remove redundant cast to short" refactoring:
    bar(1);
//ERROR on bar(1);: incompatible types: possible loss of precision from int to short

From:
    bar((short) 0xA1);
//HINT on the short keyword: unnecessary cast to byte
To (applying the suggested "remove redundant cast to short" refactoring:
    bar(0xA1);
//ERROR on bar(0xA1);: incompatible types: possible loss of precision from int to short

See? You get from an ok (yet annoying, for all the wrong hints&warnings) situation to a KO one where you have actual syntax errors.

Smoothest behaviour was when NB 7.3 required you to mark with (byte) or (short) your shorter-than-int numeric literals: no errors, no warnings, no hints and, most of all, a consistent behaviour disregarding the actual numeric value.
If you are working with unsigned bytes and unsigned shorts, having to differentiate
byte b=127;
and
byte b=(byte)128;
enormously takes legibility away, and anyway for you they are still an unsigned 127 and an unsigned 128 (because you always treat them as such [and if ever feeding them to a function accepting ints, you &0xFF them])...

This bug is immensely annoying, I have a project (and many people might too) with tons work un byte (and short) arrays, and now the NB editor is overwhelmed by highlightnings, red lightbulbs, etc. I'm forced to go back to NB 7.3 to get my work done, before deciding what to do (refactor the whole big project, change IDE, or whatnot).


(Please see attached screenshots)
Comment 6 unai 2013-10-24 19:19:44 UTC
Created attachment 141534 [details]
Screenshot taken before applying the lightbulb-suggested refactorings
Comment 7 unai 2013-10-24 19:20:08 UTC
Created attachment 141535 [details]
Screenshot taken after applying the lightbulb-suggested refactorings
Comment 8 unai 2013-10-24 19:34:03 UTC
Created attachment 141536 [details]
Screenshot about the (short)32000, taken before refactoring
Comment 9 unai 2013-10-24 19:34:41 UTC
Created attachment 141538 [details]
Screenshot about the (short)32000, taken after refactoring
Comment 10 unai 2013-10-24 19:35:53 UTC
Sorry, forgot to show the (short)32000 case: attached screens illustrating the issue.
Comment 11 Svata Dedic 2013-10-24 20:28:48 UTC
OK; so to get things straight.

comment #2 talks about
short s=(short)1234;//warning

which is NOT a problem. It compiles OK, and does not bring any precision loss (since it assigns a compile-time constant). Warning about unnecessary cast should appear, when simplified to short s = 1234; no compiler error or warning occurs.

Even (short)1234 can be seen as a short literal, no such thing exists. The typecast operation is not (strictly) needed neither for JDK7 or JDK8ea, so it is redundant. We may argue that there could be an option for such coding style. This is out of scope of this issue however; in case you want to track this, file a separate ENHANCEMENT request... or feel free to even implement it

The problem is 

foo((byte) 1);

that produces a false warning + fix that actually breaks the code. This IS a problem. For some weird reason I didn't understood yet, the specification does not include primitive narrowing conversion as a part of method invocation conversion, even though a compile-time constant is present. The javac behaves exactly according to the spec, so NB must be fixed.

A sidenote: it is not considered too polite to change priority from the one evaluated by Dev without a justification. IMNSHO, according to http://wiki.netbeans.org/BugPriorityGuidelines, this defect qualifies as P3; a viable workaround exists (do not apply the fix, disable the misbehaving hint).
Comment 12 unai 2013-10-24 20:55:01 UTC
Thank you for your comment Svata.

Concerning your side note, I apologize: mistakenly felt, driven by frustration, that this regression should've been rolled back with P2 priority (Prevents users from completing work without significant or inefficient legwork). But yes I was wrong with that, will be more conservative in the future.
Comment 13 Svata Dedic 2013-11-04 13:02:21 UTC
Copied from issue #229951 - additional operator/arithmetic issues:

lolo_101 2013-09-02 15:44:47 UTC (comment #3)

This also happens with simple types.
Consider the following simple exemple:

int a=2, b=5;
double x = a/b;            // x = 0.0
double y = ((double) a)/b; // y = 0.4

The editor displays the hint "unnecessary cast to double" for the calculation of 'y' whereas the cast obviously changes the result.

fsttesla 2013-10-22 10:03:55 UTC (comment #4)

Another case of the same problem.

Let c a char variable. Consider an expression like
"Character " + c + " has ASCII code " + (int)c
                                         ---

Generally speaking, the "unnecessary cast" hint does not take into account if the (up)cast is relevant in the expression where it occurs, either to select the right method/operator, or to suppress a warning, etc.
Comment 14 Svata Dedic 2013-11-04 13:20:59 UTC
Incidentally the example with _calling a method_ given in comment #2 is solved as part of issue #229955.

The warning on byte b = (byte)255; could not be reproduced, but would fall into the same category as examples copied in comment #13
Comment 15 Svata Dedic 2013-11-05 17:15:32 UTC
Fixed in http://hg.netbeans.org/jet-main/rev/a643ab8b8c9c
Comment 16 Jiri Prox 2014-01-06 08:38:14 UTC
*** Bug 239984 has been marked as a duplicate of this bug. ***
Comment 17 Jiri Prox 2014-01-20 09:37:59 UTC
*** Bug 240646 has been marked as a duplicate of this bug. ***
Comment 18 josephcz 2014-03-06 16:20:56 UTC
Under build 201401141042 this is still not completely fixed...

Although this now works:
    int a=2, b=5;
    double x = a/b;            // x = 0.0
    double y = ((double) a)/b; // y = 0.4


This does not (warns on cast to int, although the cast is being used to truncate the value):
    double a=2.1, b=5;
    double x = a/b;          // x = 0.42
    double y = ((int)a)/(b); // y = 0.4