RFR: JDK-8231826: Implement javac changes for pattern matching for instanceof
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Tue Oct 22 16:20:14 UTC 2019
Looks good
Maurizio
On 22/10/2019 16:27, Jan Lahoda wrote:
> Hi,
>
> A new draft of the pattern matching for instanceof has been published
> here:
> https://mail.openjdk.java.net/pipermail/amber-dev/2019-October/004962.html
>
>
> A significant change in this draft is that safe(*) reifiable types are
> allowed in instanceof of. And updated version of the pattern matching
> patch for javac which supports this change is here:
> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.03/
>
> A diff from the previous revision:
> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.delta.02-03/
>
> (*) reifiable types are allowed if (for "expr instanceof type"),
> "expr" can be cast to "type" without unchecked warnings.
>
> Does this make sense?
>
> Thanks,
> Jan
>
> On 15. 10. 19 15:02, Maurizio Cimadamore wrote:
>> Yep - that would be more helpful (at least to me)
>>
>> Thanks
>> Maurizio
>>
>> On 15/10/2019 13:56, Jan Lahoda wrote:
>>> Would this be better?
>>>
>>> Full patch:
>>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.02/
>>>
>>> Diff from previous:
>>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.delta.01-02/
>>>
>>> Thanks,
>>> Jan
>>>
>>> On 15. 10. 19 13:59, Maurizio Cimadamore wrote:
>>>> Hi,
>>>> the flow changes look good - I think the TransPattern documentation
>>>> should contain less text and more code snippet examples which show
>>>> what the generated code looks like (as I've tried to do in my
>>>> email, and as you've done for visitTypeTest). In other words, what
>>>> is missing here is "the big picture" which shows what are the main
>>>> ideas behind the translation strategy.
>>>>
>>>> Specific example: decorateExpression:
>>>>
>>>> + //if there are binding variables defined and used only in this
>>>> expression,
>>>> + //which are not confined to a specific sub-expression,
>>>> + //a let expression is created which replaces the statement, and
>>>> + //the binding variables are hoisted into this let expression:
>>>>
>>>>
>>>> This kind of illustrates my point:
>>>>
>>>> * "if there are binding variables defined and used only in this
>>>> expression, which are not confined to a specific sub-expression" is
>>>> very convoluted, and will be almost meaningless when we pick up
>>>> this code again in 6 months
>>>> * "a let expression is created which replaces the statement" -
>>>> statement? Probably cut and paste error
>>>>
>>>> Thanks
>>>> Maurizio
>>>>
>>>> On 15/10/2019 10:46, Jan Lahoda wrote:
>>>>> Hi,
>>>>>
>>>>> I've updated the patch with the Flow changes and with additional
>>>>> comments in TransPatterns here:
>>>>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.01/
>>>>> diff from previous:
>>>>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.delta.00-01/
>>>>>
>>>>> An additional patch (that would apply on top of this one) which
>>>>> makes all instanceof instances to be modelled as instanceof
>>>>> <pattern>:
>>>>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.01.unify.instanceof/
>>>>>
>>>>>
>>>>> Some more comment inline.
>>>>>
>>>>> On 10. 10. 19 17:33, Maurizio Cimadamore wrote:
>>>>>> Hi Jan,
>>>>>> the code looks generally very clean, kudos.
>>>>>>
>>>>>> Some general comments:
>>>>>>
>>>>>> * looking at the spec, it seems like both "instanceof T" and
>>>>>> "instanceof T t" are cases of type test patterns. I guess I'm
>>>>>> fine with the implementation doing what it always did in terms of
>>>>>> plain "instanceof T", but I'm worried about the intersection
>>>>>> between this and e.g. the tree API - InstanceofTree::getPattern
>>>>>> returns null in cases like "instanceof T"; now, I know I know
>>>>>> that we're speaking about a JDK specific API, but I think this
>>>>>> issue reveals some modelling issues in the way we treat
>>>>>> instanceof, and I'm worried that some of these issues might pop
>>>>>> up in the future. I'd prefer to either rectify the spec so that
>>>>>> plain 'instanceof T' is not a pattern matching instanceof, or
>>>>>> rectify javac so that these tests are internally also represented
>>>>>> with patterns (at the expense of some extra allocation, perhaps).
>>>>>>
>>>>>> * If I'm not mistaken the only use for the
>>>>>> "MATCH_BINDING_TO_OUTER" flag is to be able to distinguish
>>>>>> between regular 'expression-wide' bindings, and bindings which
>>>>>> 'leaked' outside a statement (e.g. an if statement). And the
>>>>>> reason you need to distinguish between these is that you don't
>>>>>> want Check::checkUnique to flag duplicate errors between regular
>>>>>> 'expression-wide' bindings, which are reported elsewhere
>>>>>> (MatchBindingsComputer). But this is also, more crucially, used
>>>>>> in TransPattern, where the 'isPreserved' flag is used to control
>>>>>> whether a variable decl for the binding variable should be
>>>>>> 'lifted' to the enclosing statement context or not. Is my
>>>>>> understanding correct here?
>>>>>
>>>>> Yes, the primary intent is to mark variables that need to be
>>>>> hoisted to the parent of the current statement.
>>>>>
>>>>>>
>>>>>> * The idea behind TransPatterns seems to be: when we process a
>>>>>> statement, or an expression, we attempt to add all declaration
>>>>>> for the bindings that are used inside the statements/expression
>>>>>> upfront. If we are processing a statement, then we surround the
>>>>>> results in a block; e.g.
>>>>>>
>>>>>> if (obj instanceof Foo f) {
>>>>>> ...
>>>>>> }
>>>>>>
>>>>>> becomes
>>>>>>
>>>>>> {
>>>>>> Foo f$;
>>>>>> if (let Object temp = obj in (obj instanceof Foo && (f$ =
>>>>>> (Foo)temp) == temp) {
>>>>>> ...
>>>>>> }
>>>>>>
>>>>>> If we are processing an expression, we instead generate a
>>>>>> LetExpr, e.g.
>>>>>>
>>>>>> boolean b = obj instanceof Foo t && t.equals(foo);
>>>>>>
>>>>>> becomes:
>>>>>>
>>>>>> boolean b = let Foo f$ = null in ((let Object temp = obj in (obj
>>>>>> instanceof Foo && (f$ = (Foo)temp) == temp) && f$.equals(foo))
>>>>>>
>>>>>> So, sometimes the hoisted var is a real var decl in a block,
>>>>>> other times is a var decl inside a let expression. In these cases
>>>>>> we have to generate an initializer, to set the value (which might
>>>>>> be used later).
>>>>>
>>>>> The hoisted vars do not have an initializer (they used to have
>>>>> one, but it is both unnecessary and was masking out bugs, so I
>>>>> have removed it). But I see I've forgot the initializer code
>>>>> commented out in TransPatterns, removed in the updated version to
>>>>> avoid confusion.
>>>>>
>>>>>> On top of that, instanceof generates its own letExpr to cache the
>>>>>> target of the test (to avoid double computation).
>>>>>>
>>>>>> It also seems to me that the code handles cases where the binding
>>>>>> variable is not used, neither hoisted - e.g.
>>>>>>
>>>>>> boolean field = obj instanceof Foo t;
>>>>>>
>>>>>> In this case we generate a plain instanceof w/o the init (because
>>>>>> the 't' variable hasn't been hoisted anywhere).
>>>>>>
>>>>>> Finally, we use the 'isPreserved()' flag to ensure that variables
>>>>>> are hoisted correctly - for instance, if something is to be
>>>>>> preserved (and the enclosing context allows for it) we push
>>>>>> things in the enclosing context instead.
>>>>>>
>>>>>> Am I getting the correct picture here?
>>>>>
>>>>> Yes, I think it is correct.
>>>>>
>>>>>>
>>>>>> It would be nice to have more javadoc spread around to help the
>>>>>> reader understand what's the rationale and show some snippet of
>>>>>> generated code.
>>>>>>
>>>>>> * Flow, I wonder if, like you had created SnippetAliveAnalyzer,
>>>>>> creating a SnippetBreakAnalyzer would help you avoid the
>>>>>> breaksOut[0] trick (that could become a field in the child visitor)
>>>>>
>>>>> I tried to do these two in the updated patch.
>>>>>
>>>>> Thanks for the comments!
>>>>>
>>>>> Jan
>>>>>
>>>>>>
>>>>>>
>>>>>> Other than that, it looks very good.
>>>>>>
>>>>>> Maurizio
>>>>>>
>>>>>> On 10/10/2019 13:12, Jan Lahoda wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> As part of the effort to prepare JEP 305: Pattern Matching for
>>>>>>> instanceof (Preview) for Propose to Target, I would like to ask
>>>>>>> for a code review for the corresponding javac changes.
>>>>>>>
>>>>>>> The webrev is here:
>>>>>>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.00/
>>>>>>>
>>>>>>> The patch applies on top of:
>>>>>>> https://mail.openjdk.java.net/pipermail/compiler-dev/2019-October/013727.html
>>>>>>>
>>>>>>>
>>>>>>> The current spec the patch is striving to implements is here:
>>>>>>> http://cr.openjdk.java.net/~gbierman/jep305/jep305-20190918/specs/patterns-instanceof-jls.html
>>>>>>>
>>>>>>>
>>>>>>> As far as I know, there is one (significant) open issue in the
>>>>>>> spec, and that is whether non-reifiable types should be allowed
>>>>>>> in "instanceof <type-test-pattern>". Currently (AFAIK), the spec
>>>>>>> does not allow non-reifiable types in the type test pattern in
>>>>>>> instanceof, and the javac implementation should be consistent
>>>>>>> with the spec. Should the spec change, the corresponding update
>>>>>>> to the javac code should have a very limited impact.
>>>>>>>
>>>>>>> I'll be preparing a CSR for this change in the coming days.
>>>>>>>
>>>>>>> The JBS issue for this code change is:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8231826
>>>>>>>
>>>>>>> Any feedback is welcome!
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Jan
More information about the compiler-dev
mailing list