REPL code review -- Eval

[Robert Field]

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.

>
>    - 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?

> 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.

>
>     - 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.

Thanks,
Robert