REPL code review

[Jan Lahoda]

Hi Maurizio,

Thanks for the review and comments!

On 11.9.2015 17:25, Maurizio Cimadamore wrote:
[snip]

> -SourceCodeAnalysisImpl - generally looks good, and the tab completion
> logic is quite clever:
>
>     - in scopeContent() what is the difference between the
> 'scopeIterable' and a call to Scope.getLocalElements (which is routed
> via Scope.getSymbols)?

The general intent of scopeContent is to gather all elements that are 
defined at the given place (for which the Scope was created). 
Scope.getLocalElements only returns elements that are local to the scope 
- for example for a Scope that was created for a location inside a 
method, the getLocalElements won't include the imported elements.

So, generally, it is necessary to recursively use 
Scope.getEnclosingScope() and use getLocalElements on all the Scopes 
from the given Scope up. The scopeIterable is used to do that - it 
iterates up the Scope hierarchy - on each item of the iterable the 
getLocalElements is called and the results are concatenated (using 
Stream.flatMap).

I'll work on the other comments for SourceCodeAnalysisImpl.

Thanks,
     Jan

>
>     - the logic for filtering based on target type is cool - but could
> use some cleanup - for instance there are two smart filters, and it's
> not clear why those are needed; it seems like smartTypeFilter is only
> used in imports, and the main difference is that it is allowed to
> operate on type elements, while the smartFilter can't; but the typing
> logic is the same (except for isAssignable vs. isSubtype - which
> shouldn't be important in the smartTypeFilter case anyway). I suggest
> just using smartFilter (without the class filtering logic), and the add
> the proper extra filtering logic
>     (i.e. must be a class, must be a non-class) as a separate 'kind'
> filter.
>
>     -found a bug with varargs (varargs expansion does not take place):
>
> -> class Foo { static void m(int... i) { } }
> |  Replaced class Foo
> |    Update overwrote class Foo
> -> Foo.m(Display all 395 possibilities? (y or n)
>
>     -found bugs around constructors; asMemberOf doesn't get called, and
> that leads to useless smart filters:
>
> -> class Baz<X> {
>     >> Baz(X x) { }
>     >> }
>     |  Added class Baz
>
>     -> String s = "";
>     |  Added variable s of type String with initial value ""
>
>     -> new Baz(Display all 391 possibilities? (y or n)
>
>     -> new Baz<String>(Display all 391 possibilities? (y or n)
>
>     -> Baz<String> bz = new Baz<>(Display all 391 possibilities? (y or n)
>
> The problem is that the type of the constructor is Baz(X) and no
> variable in scope can possibly have type X, so all elements in the scope
> are cleared out. In the first two cases should be fixable by simply
> adding a call to asMemberOf() on the constructor element. The third case
> is more difficult - perhaps some heuristics could be attempted, but the
> general case is hard.
>
>      -smart completion doesn't always work as expected - in particular,
> target-type-based suggestions seem to occur only once:
>
> -> fs.m(
> ls                        <press tab to see more>
>
> -> fs.m
> |  Error:
> |  cannot find symbol
> |    symbol:   variable m
> |  fs.m
> |  ^--^
>
> -> fs.m(Display all 389 possibilities? (y or n)
>
>     After first time, all members will be displayed (unless you start
> working on a different instance...)
>
> * TaskFactory:
>      - I see that cntResolve counts the recoverable errors - javac does
> have a notion of recoverable errors that is used in annotation
> processing - all recoverable diagnostics are marked with a special flag
> (DiagnosticFlag.RECOVERABLE). I wonder if it would be more robust to
> rely on that, rather than using raw diagnostic text (as the latter might
> change over time).
>
>     - I like the hierarchy of tasks - it is very easy to see which tasks
> are meant to operate on source code and which on synthetic wraps.
>
>
> Maurizio
>