Pattern matching on primitive types mxied with instanceof is a footgun

[forax at univ-mlv.fr]

> From: "Kevin Bourrillion" <kevinb9n at gmail.com>
> To: "Maurizio Cimadamore" <maurizio.cimadamore at oracle.com>
> Cc: "Tagir Valeev" <amaembo at gmail.com>, "Brian Goetz" <brian.goetz at oracle.com>,
> "Remi Forax" <forax at univ-mlv.fr>, "amber-spec-experts"
> <amber-spec-experts at openjdk.java.net>
> Sent: Tuesday, February 4, 2025 6:14:53 PM
> Subject: Re: Pattern matching on primitive types mxied with instanceof is a
> footgun

> On Tue, Feb 4, 2025 at 8:55 AM Maurizio Cimadamore < [
> mailto:maurizio.cimadamore at oracle.com | maurizio.cimadamore at oracle.com ] >
> wrote:

>> This seems consistent. So the question is: what is the "mistake" we're talking
>> about here? The user thinking it was a _unconditional_ match where in reality
>> that's not the case? While in the case of instanceof unconditionality is harder
>> to spot, if this was a switch, the compiler would have immediately asked the
>> user to provide a default?

>> It seems to me that the reasoning here implies that developers will use "if" +
>> "instanceof" _without an "else" just for the purpose of extract some data, and
>> then complain when the thing doesn't match (because they have no "else", and so
>> their code will fail). It seems like our best weapon in this case is education,
>> rather that introducing arbitrary restrictions and asymmetries in the language?

>> (Or, perhaps, in the future have a way to perform an _unconditional_ extraction
>> without an "if", in which case the compiler will bark if the pattern is not
>> unconditional.)

> In some cases this will be relevant, but in this particular example we're
> looping over Spaceship and then checking for AlienSpaceship, so, no help.

> I think it is fair to acknowledge that this is a weakness of the feature design.
> How to mitigate it? We can try to evangelize always preferring `var` for nested
> type patterns that are expected to be unconditional. That's an unfortunate
> readability trade-off, though. And a static analyzer can't know whether you
> expected it to be unconditional or not, so there might not be much it can do to
> help.

> I am not even sure what language change could be considered here. Note that this
> problem is not specific to primitive types, it's just that primitive types are
> currently the only ones that support "value conversion" so we're encountering
> this issue with them first. Post-Valhalla, other types might support value
> conversion too.

For me it's more about the conversion being implicit vs explicit. 
Here, we have an implicit conversion while the conversion does not always succeed. 

I believe such conversions, the one that may not always succeed, should be explicit not implicit. 

regards, 
Rémi 

@Brian, i'm more worry about a TA with more than 10 years of experience having trouble to find a bug in a student project written in Java. 
@Tagir, i'm not sure usin floating point values is the issue here, you can have the same kind of bug between a character and an int. 

>> On 04/02/2025 16:00, Tagir Valeev wrote:

>>> Hello!

>>> Well, I would say that Remi has a point. This is a silent mistake that can be
>>> made. You don't see the declaration, as it's in another file so you may forget
>>> about original types and assume that they are ints. The code compiles and
>>> doesn't even throw an exception, it just works incorrectly. It would be
>>> justified if such code pattern (converting from double to int only when double
>>> fits the int) was common, so conditional narrowing could be useful in many
>>> other places. But the fact is that it's highly uncommon. Nobody does this. Many
>>> other pattern matching features are nice. E.g., I often see the code which can
>>> be migrated to pattern switch (we are still on Java 17) and I really want to
>>> use it. However, I never see the code which can be migrated to a narrowing
>>> primitive pattern.

>>> I don't agree that we should drop primitive patterns completely, but we may
>>> reconsider floating point <-> integral conversions and disable them in patterns
>>> for a while (enabling them back later will be always possible).

>>> With best regards,
>>> Tagir Valeev

>>> On Tue, Feb 4, 2025 at 12:45 PM Brian Goetz < [ mailto:brian.goetz at oracle.com |
>>> brian.goetz at oracle.com ] > wrote:

>>>> Wow, that was pretty amazing. You went from “one student, who was never taught
>>>> about how this works, was confused by it” to “ let’s pull the whole feature “
>>>> in one breath.

>>>>> On Feb 4, 2025, at 11:33 AM, Remi Forax < [ mailto:forax at univ-mlv.fr |
>>>> > forax at univ-mlv.fr ] > wrote:

>>>> > Last week,
>>>> > one of the TA for the course "OOP using Java" was grading student projects,
>>>>> the subject of the project can be summarize to "you are alone in space and angry
>>>> > aliens want you to die".

>>>> > One of the project had a weird bug when displaying aliens.
>>>>> Spoiling the result of bug hunting, this is due to students using the pattern
>>>> > matching on primitive without them realizing it.

>>>> > Here is a reproducer of the bug:

>>>> > // in AlienSpaceship.java
>>>> > record AlienSpaceship(double x, double y) implements Spaceship {
>>>> > ...
>>>> > }

>>>> > ...

>>>> > // in a method in another class
>>>> > ...
>>>> > for(Spaceship spacehip : ...) {
>>>> > ...
>>>> > if (spaceship instanceof AlienSpaceship(int x, int y) {
>>>> > // display the alien space ship at (x, y)
>>>> > }
>>>> > ...
>>>> > }

>>>>> The TA said she was about to give up when she found the issue and that the Java
>>>> > spec should not allow such pattern.
>>>>> I agree with here, pattern matching on primitive types is not a feature people
>>>> > ask for and mixed with instanceof it's a footgun.

>>>>> This feature can be re-introduced later using method patterns and it will be
>>>> > easier to understand the code,
>>>> > by example with an hyphotethical method Double.asInt

>>>>> if (spaceship instanceof AlienSpaceship(Double.asInt(int x), Double.asInt(int
>>>> > y)) {
>>>> > ...

>>>> > regards,
>>>> > Rémi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/amber-spec-experts/attachments/20250204/a412829c/attachment.htm>