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

[ShinyaYoshida]

Hi Robert,
I'll reply inline.

2015-12-22 10:38 GMT+09:00 Robert Field <robert.field at oracle.com>:

> Hi Shinya,
>
> Cool.
> Some comments below.
>
> Eval.java --
>   Changes are name changes, code clean-up, and, passing the parameter.
> That all looks good.
>   In the case that some methods are corralled, a reanalysis is done.  This
> reanalysis will not have failures for the corralled
>   methods, and possibly other dependent methods -- it thus would be a
> better basis for determining the parameter types
>   which is what the "at" parameter is used for.  Since compileAndLoad only
> uses "at" for for setting diagnostics, and now
>   this use, there is no reason a separate "cat" (corralled "at") variable
> is needed, the corralled case can just reset "at".
>
I see. I'll reuse the re-analyzed "at" when any is corralled.
But "at" will not be effectively final when I re-assign at.
So I make "at" as array to hack.
What do you think about this hack?


> SourceCodeAnalysisImpl.java --
>   Changes are name changes.  Those are good.
>
> TaskFactory.java --
>   Changes are to store a list of CompilationUnitTree, instead of just the
> first.  Name changes accordingly. That is good.
>
>   AnalyzeTask constructor: Nice stream use.  The line above converts
> Iterable to Iterator, only to be converted back in the
>   argument to Util.stream.  One option would be to leave cuTrees as a List
> (just eliminating the conversions).  It appears
>   only to be used in two places (getting the first element, or to create a
> stream). So, leaving it an Iterable (no conversion)
>   seems another good option.  BTW. Using "cuts" as the name of the local
> variable and the field should be avoided, esp,
>   as in this case, where they are different.
>

>   ParseTask constructor: While currently only one compilation unit is
> returned by parse as it is currently used and as the
>   JShell parse code is implemented.  That may change in the future, and
> even without that knowledge it would be best not
>   base code on that unwritten assumption.  So, basically, whatever
> treatment is picked for the AnalyzeTask constructor
>   should also be applied to this.
>
>   Nit: 383 should be empty line between these two declarations.
>
I've changed the return type of BaseTask#cuTrees to Iterable<...> and
AnalyzeTask and ParseTask  keeps the return value of analyze() and parse()
without conversion.

In ParseTask, I've changed collecting import trees or class trees from all
of the parsed compilation units.


>

>
> TreeDissector.java --
>   Changes are to move away from the assumption that the desired code is
> always in the first compilation unit and to
>   add functionality for searching all compilation units for the desired
> class. It does indeed move in that direction with some
>   embedded assumptions of which uses first and which searches; And that
> would be OK given that the class is a bit ad hoc
>   anyway.  But getStartPosition or getEndPosition, used with
> findClassByName would give incorrect results (which would be
>   hard to trace).  Note that those two are not currently used together.
> Given that all uses of TreeDissector require a target
>   class, it would be much safer and clearer if the target class and
> corresponding compilation unit were immutable and
>   established at the instantiation of TreeDissector.  So, basically two
> constructors: one with the just BaseTask as a parameter
>   that uses the first compilation unit and first class, and the other with
> the BaseTask and class name as a parameters, where
>   compilation unit and class are found by search.
>
I've added targetClass and targetCompilationUnit and changed using it
instead of firstClass and cuTree.
And I've created static factory methods, createByFirstClass
and createBySnippet instead of constructors.


>
> Unit.java ---
>   Changes are to add AnalyzeTask parameter, with some nice code clean-up.
> Looks good.
>   One important comment was lost with the change to conditional at 416.
> Maybe reverse the sense of the test to be positive
>   (paren >= 0) and preface the return-conditional with something like:
>        // Extract the parameter type string from the method signature,
>        // if method did not compile use the user-supplied parameter types
>
Oops...
I've changed as you mentioned.

New webrev is here:
http://cr.openjdk.java.net/~shinyafox/kulla/8145239/webrev.02/

Regards and have a merry christmas,
Shinya Yoshida


>
> Thanks much,
> Robert
>
>
>
>
>
>
> On 12/14/15 18:10, ShinyaYoshida wrote:
>
> 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)
>>>
>>
>>
>
>