REPL code review

[Jan Lahoda]

Hi Maurizio,

I've tried to tweak the code according to the comments below. Links inline.

On 11.9.2015 19:01, Jan Lahoda wrote:
> 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.

http://hg.openjdk.java.net/kulla/dev/langtools/rev/1fdd34dd23bc

I kept smartTypeFilter and smartFilter, but the only the first one is 
doing the actual type check now - the second one adds the kind check (to 
get a full 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)

http://hg.openjdk.java.net/kulla/dev/langtools/rev/946ca38ccc49

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

http://hg.openjdk.java.net/kulla/dev/langtools/rev/ea1fd645628d

It first tries to find the type in the AST, and if there is no usable 
type, it resorts to the targetType machinery that already exists. I hope 
this will work reasonably, I think.

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

http://hg.openjdk.java.net/kulla/dev/langtools/rev/214544e27c9a

Pressing <tab> should now cycle between the "smart" and ordinary 
completion. (I was looking if I could improve typing detection - so that 
the completion state would reset when the user would e.g. type and then 
delete something, but so far, I didn't find a reasonable way to do that.)


How do these look?

Thanks!
     Jan

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