Bug in the record pattern translation / record pattern spec

[Remi Forax]

> From: "Brian Goetz" <brian.goetz at oracle.com>
> To: "amber-spec-experts" <amber-spec-experts at openjdk.java.net>
> Sent: Monday, June 27, 2022 9:38:00 PM
> Subject: Re: Bug in the record pattern translation / record pattern spec

> [ dropping amber-dev ]

> Thanks for this concrete example, its helpful.

> So far, we can put this in the category of "there exists at least one case where
> this handling of exceptions makes it harder to understand what is going on."
> This isn't necessarily dispositive, in that in order to make a fair
> determination, we'd have to consider both the frequency (this is kind of a
> corner case) and also the complement: the cases where it helps.

> Let me explain (again) why we went this way. We're doing record patterns *now*,
> but it is entirely clear where this is going: record patterns are not special,
> records will eventually acquire an implicit canonical deconstruction pattern,
> and users will be able to explicitly declare deconstruction patterns as well.
> And (barring separate compilation anomalies), the two should not really be
> distinguishable; whether `javac` generates a synthetic member for a dtor
> pattern, or whether it simulates the behavior of that synthetic member on the
> client side, should be an implementation detail. So accessors are not "special"
> here.

> Having a deconstruction pattern throw is a logic error, of the same sort that
> causes NPE or AIOOBE. Pattern matching may, as part of doing its thing, invoke
> the imperative logic associated with a deconstruction pattern; it expects these
> not to fail. Telling users "pattern matching might fail with an arbitrary
> error" only encourages users to try and catch these errors, which is misguided
> and which might well catch up other unrelated errors in the same net. "There
> was a violated invariant in a pattern-driven construct" is a reasonable error
> model to present to users when we are invoking unspecific behavior at
> unspecific times on the user's behalf.
yes, that's why i said that for me the issue is closer to an implementation problem, the idea is fine but the current implementation is not. 

We can notice that the static block has exactly the same issue, in both case you do not want users to try to recover if an exception occurs. 
Compared to the semantics of a static block, the key difference is that a static block throws an Error not a runtime exception. 
There are two problems with using a runtime exception, users may still think it's Ok to catch it and if a getter/deconstructor throw an Error like OutOfMemoryError/StackOverflowError, wrapping it into a runtime exception change the behavior of the others try/catchs that are present below in the stack. 

For the deconstruction pattern, i propose to throw a specific error and to not wrap the subclasses of java.lang.Error thrown by the deconstruction pattern. 

> Obviously, the code in this example is broken; what you're hoping for is a clean
> SOE (if it were wrapped once with ME, that would be fine), so users can say
> "oops, I'm a dummy" and fix it.
yes ! 

Rémi 

> On 6/27/2022 4:54 AM, Remi Forax wrote:

>> Hi all,
>> i've already reported that issue but it was theoretical because the support of
>> the record pattern was not integrated into the jdk 19 at that time and i was
>> not able to explain int clearly.  Now that the support is available, this code
>> should throw a StackOverflow error but it creates a linked list of
>> MatchException which makes it hard to debug.

>> Wrapping all exceptions into a runtime exception is not a good idea when you
>> have recursive calls which is a kind of natural when you have pattern matching,
>> for me it's a spec issue but i want to be sure.

>> BTW, the bug here lies in the fact that deconstructing Cons(int value, int size,
>> RecChain next) calls the accessor size() which itself calls RecChain::size
>> which is a nice puzzler by itself.

>> public sealed interface RecChain {
>>   default int size() {
>>     return switch (this) {
>>       case Nil __ -> 0;
>>       case Cons(int value, int size, RecChain next) -> 1 + next.size();
>>     };
>>   }

>>   record Nil() implements RecChain { }

>>   record Cons(int value, int size, RecChain next) implements RecChain {
>>     @Override
>>     public int size() {
>>       return size != -1? size: RecChain.super.size();
>>     }
>>   }

>>   public static void main(String[] args){
>>     RecChain chain = new Nil();
>>     for (var i = 0; i < 100; i++) {
>>       chain = new Cons(i, -1, chain);
>>     }
>>     System.out.println(chain.size());
>>   }
>> }

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