RFR: JDK-8231826: Implement javac changes for pattern matching for instanceof
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Nov 22 12:31:56 UTC 2019
Looks good
Maurizio
On 22/11/2019 12:21, Jan Lahoda wrote:
> 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