RFR: JDK-8231826: Implement javac changes for pattern matching for instanceof
Jan Lahoda
jan.lahoda at oracle.com
Fri Nov 22 12:21:28 UTC 2019
A few additional touches to the patch:
-improvement to the javadoc for InstanceOfTree.getPattern(), as
suggested on the CSR (JDK-8231827)
-adding missing @module key in
test/langtools/tools/javac/annotations/typeAnnotations/classfile/Patterns.java
-enhancing the test/langtools/tools/javac/patterns/ReifiableOld.java
test to verify that for the most recent source level and preview
features disabled the "instanceof <non-reifiable-type>" still works as
before (i.e. non-reifiable types disallowed).
Delta webrev from last revision:
http://cr.openjdk.java.net/~jlahoda/8231826/webrev.delta.03-04/
Full webrev:
http://cr.openjdk.java.net/~jlahoda/8231826/webrev.04/
Any objections to this?
Thanks,
Jan
On 22. 10. 19 18:20, Maurizio Cimadamore wrote:
> 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