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

[Robert Field]

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)