RFR: 8200145: Conditional expression mistakenly treated as standalone

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8200145: Conditional expression mistakenly treated as standalone

Guoxiong Li-2
Hi all,

If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.

This patch fixes it and adds a test case.
Thank you for taking the time to review.

Best Regards.
-- xiong

-------------

Commit messages:
 - 8200145: Conditional expression mistakenly treated as standalone

Changes: https://git.openjdk.java.net/jdk/pull/2324/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2324&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8200145
  Stats: 46 lines in 3 files changed: 45 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2324.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2324/head:pull/2324

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone

Aleksey Shipilev-5
On Sat, 30 Jan 2021 08:50:53 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.
>
> This patch fixes it and adds a test case.
> Thank you for taking the time to review.
>
> Best Regards.
> -- xiong

I think we want to ping @lahodaj to see if this makes sense.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone

Maurizio Cimadamore-2
In reply to this post by Guoxiong Li-2
On Sat, 30 Jan 2021 08:50:53 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.
>
> This patch fixes it and adds a test case.
> Thank you for taking the time to review.
>
> Best Regards.
> -- xiong

The fix looks good - it's a good "quality of life" fix, which improves the conditional classification by making it more resilient to erroneous types. I wonder if the fix can be made more minimal (see code comment).

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 4326:

> 4324:      */
> 4325:     public Type unboxedType(Type t) {
> 4326:         if (t.hasTag(ERROR))

This is not necessary, right? After all, you check for errors upfront, in the conditional code. Reason I'm asking is that it's not clear to me as to whether we should just return the error type, or no type. Also, this might affect code which is unrelated to the one you are fixing.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1959:

> 1957:         //where
> 1958:             boolean primitiveOrBoxed(Type t) {
> 1959:                 return (!t.hasTag(TYPEVAR) && !t.hasTag(ERROR) && types.unboxedTypeOrType(t).isPrimitive());

Good catch - you can also use `!t.isErroneous()` here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone

Jan Lahoda-3
On Fri, 16 Apr 2021 15:53:34 GMT, Maurizio Cimadamore <[hidden email]> wrote:

>> Hi all,
>>
>> If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.
>>
>> This patch fixes it and adds a test case.
>> Thank you for taking the time to review.
>>
>> Best Regards.
>> -- xiong
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 4326:
>
>> 4324:      */
>> 4325:     public Type unboxedType(Type t) {
>> 4326:         if (t.hasTag(ERROR))
>
> This is not necessary, right? After all, you check for errors upfront, in the conditional code. Reason I'm asking is that it's not clear to me as to whether we should just return the error type, or no type. Also, this might affect code which is unrelated to the one you are fixing.

(I am not an expert on types, so I may be wrong.)

This method seems to return either the primitive type for which `t` is a box, or `noType`. So returning a `noType` for erroneous type seems reasonable to me. Basically, it would mean that an erroneous type is not a box for any type, which seems sensible. (Currently, every erroneous type is seen as the box for the first primitive type in the sequence, which I think is `byte`.)

I agree only one of these two changes is needed, although I'd personally probably try to go with the one here, in `unboxedType`, as it seems like a generally desirable behavior to me. I agree it can have effects on other parts of the code, though, so it has more potential to break something.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone

Maurizio Cimadamore-2
In reply to this post by Guoxiong Li-2
On Sat, 30 Jan 2021 08:50:53 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.
>
> This patch fixes it and adds a test case.
> Thank you for taking the time to review.
>
> Best Regards.
> -- xiong

Marked as reviewed by mcimadamore (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone

Maurizio Cimadamore-2
In reply to this post by Jan Lahoda-3
On Fri, 16 Apr 2021 16:55:56 GMT, Jan Lahoda <[hidden email]> wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 4326:
>>
>>> 4324:      */
>>> 4325:     public Type unboxedType(Type t) {
>>> 4326:         if (t.hasTag(ERROR))
>>
>> This is not necessary, right? After all, you check for errors upfront, in the conditional code. Reason I'm asking is that it's not clear to me as to whether we should just return the error type, or no type. Also, this might affect code which is unrelated to the one you are fixing.
>
> (I am not an expert on types, so I may be wrong.)
>
> This method seems to return either the primitive type for which `t` is a box, or `noType`. So returning a `noType` for erroneous type seems reasonable to me. Basically, it would mean that an erroneous type is not a box for any type, which seems sensible. (Currently, every erroneous type is seen as the box for the first primitive type in the sequence, which I think is `byte`.)
>
> I agree only one of these two changes is needed, although I'd personally probably try to go with the one here, in `unboxedType`, as it seems like a generally desirable behavior to me. I agree it can have effects on other parts of the code, though, so it has more potential to break something.

I'm ok with going for the proposed fix, as long as tests are ok. Approving.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone

Guoxiong Li-2
On Fri, 16 Apr 2021 17:26:24 GMT, Maurizio Cimadamore <[hidden email]> wrote:

>> (I am not an expert on types, so I may be wrong.)
>>
>> This method seems to return either the primitive type for which `t` is a box, or `noType`. So returning a `noType` for erroneous type seems reasonable to me. Basically, it would mean that an erroneous type is not a box for any type, which seems sensible. (Currently, every erroneous type is seen as the box for the first primitive type in the sequence, which I think is `byte`.)
>>
>> I agree only one of these two changes is needed, although I'd personally probably try to go with the one here, in `unboxedType`, as it seems like a generally desirable behavior to me. I agree it can have effects on other parts of the code, though, so it has more potential to break something.
>
> I'm ok with going for the proposed fix, as long as tests are ok. Approving.

Yes, this is not necessary if we only solve this bug. But it is useful logically. And all the related tests passed locally and passed at the `Pre-submit tests`. So I think it is good to remain it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone [v2]

Guoxiong Li-2
In reply to this post by Guoxiong Li-2
> Hi all,
>
> If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.
>
> This patch fixes it and adds a test case.
> Thank you for taking the time to review.
>
> Best Regards.
> -- xiong

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

  Use method isErroneous

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2324/files
  - new: https://git.openjdk.java.net/jdk/pull/2324/files/bf5e88f1..2fe7665d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2324&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2324&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2324.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2324/head:pull/2324

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone [v2]

Guoxiong Li-2
In reply to this post by Maurizio Cimadamore-2
On Fri, 16 Apr 2021 15:54:05 GMT, Maurizio Cimadamore <[hidden email]> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Use method isErroneous
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1959:
>
>> 1957:         //where
>> 1958:             boolean primitiveOrBoxed(Type t) {
>> 1959:                 return (!t.hasTag(TYPEVAR) && !t.hasTag(ERROR) && types.unboxedTypeOrType(t).isPrimitive());
>
> Good catch - you can also use `!t.isErroneous()` here.

Fixed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone [v3]

Guoxiong Li-2
In reply to this post by Guoxiong Li-2
> Hi all,
>
> If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.
>
> This patch fixes it and adds a test case.
> Thank you for taking the time to review.
>
> Best Regards.
> -- xiong

Guoxiong Li has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

 -  Merge branch 'master' into JDK-8200145
 - Use method isErroneous
 - 8200145: Conditional expression mistakenly treated as standalone

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2324/files
  - new: https://git.openjdk.java.net/jdk/pull/2324/files/2fe7665d..ec4c6402

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2324&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2324&range=01-02

  Stats: 238854 lines in 6772 files changed: 141667 ins; 67989 del; 29198 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2324.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2324/head:pull/2324

PR: https://git.openjdk.java.net/jdk/pull/2324
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8200145: Conditional expression mistakenly treated as standalone [v3]

Guoxiong Li-2
On Sat, 17 Apr 2021 15:16:13 GMT, Guoxiong Li <[hidden email]> wrote:

>> Hi all,
>>
>> If the argument `Type t` of the method `Types.unboxedType` is an `ErrorType`, the `Types.unboxedType` may return the wrong result. And in this case, `Attr.isBooleanOrNumeric` and `Attr.isBooleanOrNumeric` return the wrong result, too.
>>
>> This patch fixes it and adds a test case.
>> Thank you for taking the time to review.
>>
>> Best Regards.
>> -- xiong
>
> Guoxiong Li has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
>  -  Merge branch 'master' into JDK-8200145
>  - Use method isErroneous
>  - 8200145: Conditional expression mistakenly treated as standalone

I updated the code according to the comment. Thank you for reviewing again and sponsoring.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2324