REPL code review -- Eval

[Maurizio Cimadamore]

On 17/09/15 21:17, Robert Field wrote:
>
> On 09/11/15 08:25, Maurizio Cimadamore wrote:
>> * Eval
>>
>>     - just curious - you seem to do a lot of String-based analysis 
>> (see example in computeQualifiedParameterTypes()) - why is it not 
>> possible to rely on the more robust Element API to do that?
>
> Not sure what you mean by "String-based analysis".  The final return 
> is a String which is stored in the Snippet for equality comparison 
> with incoming Snippets. But all the processing using compiler API 
> constructs: a compiler analysis pass is run "AnalyzeTask", 
> TreeDissector walks the tree to extract the the MethodType.  Then a 
> subclass of the compiler's SignatureGenerator converts it to a String.
>
>> I.e. couldn't a snippet corresponding to a declaration have an Element?
>
> Well, it does have an Element intermediate. It is not stored in the 
> Snippet because there is no on-going need to analyze the Snippet once 
> it is place. I use Type directly because Symbol hiding behind Element 
> does not have the functionality I need.
>
>> Or, even more generally, why not just create real AST nodes for wraps?
>
> A high-level design decision was that wraps would be source-to-source 
> this is for specification and design-docs.  Also, in some cases trees 
> do not break the source down sufficiently.
Not sure I understand what you mean here - but I understand this was a 
deliberate choice; I just note that, at the end of the day, I'm not sure 
if it buys you simplicity.
>
>>
>>    - UnresolvedExtractor - it would probably help to use 
>> DiagnosticFlag.RECOVERABLE instead of hard-coded strings to filter 
>> out resolution error (I also noted this somewhere below). However, I 
>> also note how the logic for extracting the unresolved symbol is 
>> inherently fragile and dependent on what is the shape of the javac 
>> diagnostics.
>
> Using a flag seems better indeed.  I see DiagnosticFlag.RESOLVE_ERROR 
> which seems to be what matches resolution errors (there is no doc on 
> these enum values).  Yes? What is DiagnosticFlag.RECOVERABLE?
Sorry - I messed them up - RESOLVE_ERROR is the flag associated with 
recoverable errors during annotation processing - bottom line, 
annotation processing has a very similar definition of recoverable as 
you do.
>
>> I wonder if looking at the analyzed tree might not provide a more 
>> robust solution here.
>
> I would not want to replicate javac's error detection logic, but after 
> the error is detected I'd have to use trees to find the unresolved if 
> I can't depend on the error format.
>
>>
>>     - the Range logic is clever - but, again, it seems like a big 
>> workaround for the fact that wrapping is defined in terms of strings 
>> - not trees; any reason you went down this path?
>
> See above.
>
>>
>>     - corralled - I found the Snippet hierarchy underused when it 
>> comes to corralling; the task of corralling a snippet is basically 
>> performed by Eval through some helper routines - couldn't we have 
>> each snippet define how corralling should be done? You go half-way 
>> there by having a corral() method in the Snippet base class, but it 
>> doesn't do much, other then retrieving whatever value has been set 
>> from outside.
>
> Corralling logic is in two parts.  In Eval.processMethod() the 
> corralled wrap is generated and used as one of the values to create 
> the MethodSnippet (currently only methods can be corralled).  In 
> Eval.declare(), on failing compilation, if a corralled wrap is 
> available, it attempts to compile the corralled wrap.
>
> The  *Snippet classes present as immutable values, though some 
> internal values are mutable.  This is an internal immutable value. So, 
> it could be computed after creation.  But the tree dissection, range, 
> and wrapping code has everything in common with Eval, and nothing in 
> common with *Snippet.
>
> The corralled compilation logic is independent of Snippet kind.
>
>>
>>    - the corralling logic doesn't seem to work on supertypes using FQN:
>>
>> -> class A extends B.Inner { static class Inner { } }
>> |  Error:
>> |  package B does not exist
>> |  class A extends B.Inner { static class Inner { } }
>> |                  ^-----^
>
>> I would have expected the snippet to wait for B.
>
>
> Hmmm,..
>
> It is a good thing you suggested changing those confusing names. I use 
> the old term "corralling" still in the code, which refers to 
> RECOVERABLE_DEFINED, which this isn't, but seems it should be 
> RECOVERABLE_NOT_DEFINED.
>
> The problem is that the error is not a "compiler.err.cant.resolve" 
> error but a "package does not exist" error.  It seems, except for 
> import that this error should be handled like resolution errors. The 
> change to using a flag and more general extractor (as above) should 
> address this.
If the flag is set for such errors (finger crossed) - anyway if it's not 
I guess we can make it so (but we'd need to consult the annotation 
processing gods).
>
>>
>>     - the updates of corralled snippets seems to happen at the wrong 
>> time - this could lead to chain of symbols that are not usable:
>>
>> -> class D extends E { }
>> |  Added class D, however, it cannot be referenced until class E is 
>> declared
>>
>> -> class E { D d; }
>> |  Added class E, however, it cannot be referenced until class D is 
>> declared
>>
>>     I guess what happens here is that, since the decision on whether 
>> to corral or not is based on the fact that we can compile cleanly , 
>> here we run into troubles. The first snippet obviously cannot be 
>> compiled cleanly, so it's corralled; the second one cannot compile 
>> cleanly because the first one is not 'defined' - so you get another 
>> resolution error and another corralled snippet. Unfortunately, the 
>> two snippets taken together form a legal source. Btw, I think the 
>> current behavior is preventing stuff like this:
>>
>> -> class A extends B { }
>> |  Added class A, however, it cannot be referenced until class B is 
>> declared
>>
>> -> class B extends A { }
>> |  Added class B, however, it cannot be referenced until class A is 
>> declared
>>
>> from looping forever in Eval.declare; perhaps the code could be made 
>> more robust by looking explicitly for update loops (i.e. this can be 
>> done by keeping a stack of all the updated snippets and see if we 
>> encounter the same snippet twice).
>>
>>    - on update loops - I guess it is indeed possible to have loops, 
>> even with current corralling limitations (by exploiting redefinition):
>>
>> -> class Outer { class Inner extends Foo { } }
>> |  Added class Outer, however, it cannot be referenced until class 
>> Foo is declared
>>
>> -> class Foo { }
>> |  Added class Foo
>>
>> -> class Foo extends Outer { }
>>
>> Update loop check logic is in order I think. Also, interestingly - 
>> I'm not able to stop the REPL once I get into such a loop. Actually, 
>> later I found out it doesn't loop forever - I guess eventually it 
>> runs out of stack (after several minutes) and exits saying this:
>>
>> |  Replaced class Foo
>> |    Update overwrote class Foo
>>
>>
>> Which seems bogus anyway - given there's cyclic inheritance (no error 
>> is reported!).
>>
>
> Oy!
>
> Wrapped snippets are compiled one at a time -- simple and clean, but 
> that fails in this case.  One problem is that the set of updates is 
> not known before compilation.  If a new snippet is or causes an update 
> to an existing snippet its dependencies need only be updated in the 
> case that redefinition fails.  Redefinition usually succeeds, and 
> certainly does for the false positive updates.  I think the initial 
> snippet and set of updates will need to be recompiled as full sets in 
> each iteration, at least in the case of failure.
Yeah - that seems a promising approach - bundle them up and recompile 
them together.

Maurizio
>
> Thanks,
> Robert
>