Updated patterns-in-switch doc

[Tagir Valeev]

Hello!

Found a couple of minutes to read this. Below are some of my thoughts.

#1 Number of not covered primitive types.

> with the possible temporary exception of the three primitive types not currently permitted

I believe, there are four of them: boolean, long, double, and float

Similarly, this part should be refined to mention long:

> is limited to the integral numeric primitive types; this includes char but leaves out float, double and boolean.

#2 Disallowing switch expressions inside guards

> And worse if we allow switch expressions inside guards, which we shouldn't do.

Hm... sounds like this may heavily complicate the grammar if we really
want to prohibit switch expressions anywhere inside the guard. E.g.:
switch (obj) {
case Foo(int x)
  __where process(switch(x) {case 1 -> 10;case 2 -> 20;default -> 0;}) -> ...
}
Should this be allowed? If yes, then there's no point to disallow
top-level switch expressions inside guards, as nested ones are
confusing to the same level.
If yes, then the grammar should be updated to include tons of
productions like ExpressionNoSwitch, MethodInvocationNoSwitch,
ArgumentListNoSwitch, and so on.

To me, adding any restrictions to expressions inside guards looks an
arbitrary decision. If users really want to write confusing code, let
them allow using expression switches in guards.

#3 Total patterns in instanceof

> It is sensible because x instanceof <total pattern> is in some sense a silly question, in that it will always be true and there's a simpler way (local variable assignment) to express the same thing.

It should be noted that local variable assignment requires the
variable declaration which cannot be done inside the expression.
However, the total pattern allows declaring temporary variables inside
the expression. E.g. assuming

class X {
  Foo doExpensiveCalculation();
}

we can write today
if (x != null && x.doExpensiveCalculation() != null &&
x.doExpensiveCalculation().isValidResult()) {
  use(x.doExpensiveCalculation());
}

Using total instanceof we can declare new variable without explicit statement:

if (x != null && x.doExpensiveCalculation() instanceof var foo && foo
!= null && foo.isValidResult()) {
  use(foo);
}

If this is prohibited, we will need to split `if` to two separate statements:

if (x != null) {
  var foo = x.doExpensiveCalculation();
  if (foo != null && foo.isValidResult()) {
    use(foo);
  }
}

Well, we may split declaration and assignment and put the assignment
inside the condition:

Foo foo;
if (x != null && (foo = x.doExpensiveCalculation()) != null &&
foo.isValidResult()) {
  use(foo);
}

But this has at least two drawbacks: necessity to specify explicit Foo
type (var doesn't work anymore) and broadening the scope of `foo` more
than necessary (it pollutes the namespace after `if`). In general, it
looks asymmetrical to me that the condition can introduce variables
only if we can express the condition on that variable with a pattern,
but we cannot do this if we have a guard-like condition.

If the guard is considered as a part of the pattern (rather than a
part of switch-case label grammar) then we could use the following
syntax:

if (x != null && x.doExpensiveCalculation() instanceof var foo __where
foo != null && foo.isValidResult()) {
  use(foo);
}

In this case, the pattern is guarded, thus non-total, thus we can use
it in instanceof. This also reads quite naturally to me, as __where
explicitly says that the following condition refines the pattern, not
just some unrelated, so it could be convenient to use guards on
non-total patterns as well.

To conclude, I think if we disallow total patterns in instanceof we
should allow guards there. Otherwise, we are losing the convenience of
introducing variables inside the expressions.

#4 null-hostility of String/primitive box/enum switches only

> For a switch on String, a primitive box type, or an enum type, if there is no explicit case null, we insert an implicit case null at the beginning of the switch that throws NPE.

This looks a little bit suspicious to me, but probably we can preview
and play with this rule. On one hand, looking only at switch selector
expression, we cannot say without resolve whether it's an enum or a
sealed type. And sometimes sealed types could be very similar to
enums. Especially, taking into account that enhanced enums weren't
implemented so we cannot have generic enum, but now we can emulate it
with a generic sealed type whose subclasses are all singletons. So one
may think of a specific type as of enum, but it's not actually an enum
(thus the switch will unexpectedly tolerate null).

On the other hand, if we disallow constant patterns, it looks like we
can judge whether it's a enum/string/box-switch or pattern-switch
checking looking at one of the case labels: labels for enum-switch and
pattern-switch should look visually distinct (two identifier tokens
for type pattern but only one token for enum/string/box constant). If
we can be sure that patterns always differ visually from constant
labels, then I think it's an ok idea.

To conclude, given all our previous discussions about nullity and all
the disadvantages of alternative designs, I think the newly proposed
nullity design is good to be implemented as a preview feature. We can
refine it in later preview rounds if it appears to be problematic.

With best regards,
Tagir Valeev

On Tue, Sep 8, 2020 at 11:45 PM Brian Goetz <brian.goetz at oracle.com> wrote:
>
> I have updated
>
>     https://github.com/openjdk/amber-docs/blob/master/site/design-notes/type-patterns-in-switch.md
>
> based on our discussions.