RFR 8145239: JShell: throws AssertionError when replace classes with some methods which depends on these classes

[ShinyaYoshida]

Hi Robert,
Thank you for your review and comments.

I've updated my patch to take your advice:
http://cr.openjdk.java.net/~shinyafox/kulla/8145239/webrev.01/

In my patch:
- rename BaseTask#cuTree to firstCuTree
- add "cuTrees" method which return all cu tree to BaseTask
- add "findClassByName" method to TreeDissector, and use it in typeOfMethod
with current snippet's class name
- change method signatures of TreeDissector#first* to receive the parent
Tree to find first

Please review it again.

Regards,
shinyafox(Shinya Yoshida)


2015-12-15 4:29 GMT+09:00 Robert Field <robert.field at oracle.com>:

> Another good catch, Shinya!
>
> The general fix idea is good, that the analyze for parameter types cannot
> be done with stale info -- it needs to the active
> trial Unit set as context.
>
> I'll do this review from low-level detail to global -- which means many
> comments will make the previous ones moot.
>
> * Stylistically, in AnalyzeTask, adding a constructor for a prepended
> main, when that has no inherent meaning for AnalyzeTask
> seems misplaced, I'd put the prepend in the caller who knows (though it is
> convenient for streamification).  And as a result a
> lot of delicate things, like the flag settings are duplicated.
>
> * The main Unit is a member of ins -- so prepending it means a duplicate
> class, since this patch works, I guess this happens to
> work, but this doesn't seem something we should depend on.
>
> * The patch constructs the main Unit using the two argument constructor,
> which is reserved for dropped snippets and sets it
> as a dropped snippet, that is, probably, never exposed, but it is scary.
>
> * I wrote TreeDissector with an assumption, based on assuming there is
> only one class being analyzed, that the first one is the
> one we care about.  Again, since this patch works, it must
> usually/frequently/always/in-this-case/who-knows map the first input
> as the first output tree.  But I see no documentation saying that will
> happen.
>
> * In this case, TreeDissector could do something cleaner than it did
> before (picking the first), namely: it knows the class name it
> is interested in based on the class name of the outer wrap -- it can grab
> that tree.
>
> * That would mean there is no need to do the prepend trick.  That, in
> turn, would mean the same input to analyze in each case.
> Thus, no reason to reanalyze with each Unit in ins.  but, looking out a
> couple layers, that analyze has already been done in Eval.
> So, rather than passing ins into setStatus(), it could pass the
> AnalyzeTask -- saving one or many compiles.  More efficient,
> potentially much more efficient and cleaner than my original code.
>
> The last two bullets make the previous ones moot.
>
> Thanks,
> Robert
>
> On 12/12/15 02:17, ShinyaYoshida wrote:
>
>> Hi Robert,
>> Could you review my patch for issue 8145239?
>> bugs: https://bugs.openjdk.java.net/browse/JDK-8145239
>> webreve: http://cr.openjdk.java.net/~shinyafox/kulla/8145239/webrev.00/ <
>> http://cr.openjdk.java.net/%7Eshinyafox/kulla/8145239/webrev.00/>
>>
>> Regards,
>> shinyafox(Shinya Yoshida)
>>
>
>