[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-compiler-dev
Subject:    Re: RFR (14): JDK-8236670: Conflicting bindings accepted in some cases
From:       Maurizio Cimadamore <maurizio.cimadamore () oracle ! com>
Date:       2020-01-13 14:22:45
Message-ID: 51c3b363-9af3-e08a-6f17-fd5057d33957 () oracle ! com
[Download RAW message or body]

Looks good - I like the way in which you have folded binding computation 
inside Attr.

As for the changes in TransPatterns, they are a bit hard to digest, but 
I think ultimately correct. I suggest replacing bindingDeclared(Symbol) 
with declareBinding(Symbol), so that it's clearer that it's an operation 
that should take place before any access (e.g. call to "getBindingFor").

Maurizio

On 13/01/2020 12:42, Jan Lahoda wrote:
> Hi,
>
> Tagir reported a problem where javac accepts a program with 
> conflicting bindings, like (simplified):
>
> static void test(Object o1, Object o2) {
>       boolean b = o1 instanceof String s && !(o2 instanceof String s));
> }
>
> There are two problem here:
> -in the right-hand side of the conditional and, "s" is already in 
> scope, but no error reported. This is because in Check, while checking 
> clashes, clashes between binding variables are ignored, and expected 
> to be reported in MatchBindingsComputer - but here, 
> MatchBindingsComputer does not see the conflict when computing the 
> binding variables for the expression. The proposed fix is to let the 
> error be reported in Check, with some checks to avoid duplicate errors 
> from MatchBindingsComputer. As a side effect, this should provide 
> better error positions in some (although not all) cases, consider:
> static void test(Object o1, Object o2) {
>       if (o1 instanceof String s && (o2 instanceof String s));
> }
> before:
> /tmp/T.java:3: error: illegal attempt to redefine an existing match 
> binding
>       if (o1 instanceof String s && (o2 instanceof String s));
>                                  ^
> with the patch:
> /tmp/T.java:3: error: illegal attempt to redefine an existing match 
> binding
>       if (o1 instanceof String s && (o2 instanceof String s));
>                                                           ^
>
>
> -the second problem is more intricate - some bindings consistency 
> constraints are curently only checked when match bindings are 
> evaluated for a whole expression, like e.g.:
> if (!(o1 instanceof String s) && !(o2 instanceof String s)) {}
>
> this is illegal per JLS 6.3.1.1 ("It is a compile-time error if a 
> pattern variable is both introduced by a when false, and by b when 
> false.") The constraint is checked when the expression is e.g. a 
> condition for an if statement, but not when the expression is e.g. an 
> initializer to a variable:
> boolean b = !(o1 instanceof String s) && !(o2 instanceof String s);
>
> - this yields no error, as the expression is never checked by the 
> MatchBindingComputer as a whole. We could either run the 
> MatchBindingComputer on every non-conditional expression, but seems a 
> nicer way is to build the current match bindings while attributing the 
> expressions (i.e. keeping the match bindings in Attr and enhance/check 
> them as it goes up the AST).
>
> The current proposed patch is here:
> http://cr.openjdk.java.net/~jlahoda/8236670/webrev.00/
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8236670
>
> What do you think?
>
> Thanks,
>      Jan
[prev in list] [next in list] [prev in thread] [next in thread]