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

[ShinyaYoshida]

Hi Robert,
Thank you for your review!

2015-12-28 11:05 GMT+09:00 Robert Field <robert.field at oracle.com>:

>
>
>
> But "at" will not be effectively final when I re-assign at.
>
>
> That's annoying, isn't it!
>
Yes, it is!!!

I've changed to following:

+            AnalyzeTask cat;+            if (ins.stream().anyMatch(u
-> u.corralIfNeeded(ins))) {
                 // if any were corralled, re-analyze everything-
          AnalyzeTask cat = state.taskFactory.new AnalyzeTask(ins);+
             cat = state.taskFactory.new AnalyzeTask(ins);
                 ins.stream().forEach(u ->
u.setCorralledDiagnostics(cat));+            } else {+
cat = at;
             }-            ins.stream().forEach(u -> u.setStatus());+
          ins.stream().forEach(u -> u.setStatus(cat));



> So I make "at" as array to hack.
> What do you think about this hack?
>
>
> I think you have correctly characterized it -- a hack.
>
> A somewhat less hacky approach would be to assign it to a new effectively
> final AnalyzeTask variable just before the forEach.
>
>
>
>>
>> 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.
>
>
> Clean!
>
> I note though that createBySnippet will only find it if happens to be the
> first class in the CU -- it should find it anywhere.
>
> Maybe instead of computeFirstClass(), you could have a utility method that
> returns a stream of the classes in a CU would fill the needs of both
> create*() factory methods?
>

Certainly!!
I've changed so.

The new webrev is here:
http://cr.openjdk.java.net/~shinyafox/kulla/8145239/webrev.03/

Could you push it?

Best Regards,
shinyafox(Shinya Yoshida)


>
>
>
>
>>
>> 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.
>
>
> Thanks.
>
>
> New webrev is here:
> http://cr.openjdk.java.net/~shinyafox/kulla/8145239/webrev.02/
>
> Regards and have a merry christmas,
> Shinya Yoshida
>
>
> Thanks!
>
> I'll be working over the holidays so I can push when you have it ready.
>
> Much appreciated,
> Robert
>
>
>
>
>>
>> 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)
>>>>
>>>
>>>
>>
>>
>
>