REPL code review

[Maurizio Cimadamore]

The changes look good; I like how you handled the diamond case by 
reusing the target-type machinery to find a suitable target-type.

The smart filter simplification is also welcome.

Thanks

Maurizio

On 14/09/15 16:06, Jan Lahoda wrote:
> 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
>>>