Bug in the record pattern translation / record pattern spec

[Brian Goetz]

[ 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.

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.



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-observers/attachments/20220627/67f8b1ba/attachment.htm>