RFR: JDK-8231826: Implement javac changes for pattern matching for instanceof

Tue Oct 15 12:56:23 UTC 2019

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