RFR 8131027: JShell API/tool: suggest imports for a class

[Jan Lahoda]

Hi Robert,

Thanks for the comments - I've uploaded an updated webrev here:
http://cr.openjdk.java.net/~jlahoda/8131027/webrev.01/

A delta webrev from the previous iteration:
http://cr.openjdk.java.net/~jlahoda/8131027/webrev.01/delta/webrev/index.html

Some more comments inline.

On 28.1.2016 22:38, Robert Field wrote:
> This will be a big help -- thanks for coding this.
>
> Below is a pre-review.
>
> JShell:
>
>      Fine.
>
>
> Eval:
>
>      Looks good.
>
>
> SourceCodeAnalysis:
>
>      This is public API, so names and comments really matter...

I tried to improve the names and javadoc.

>
>      analyzeType() -- There is a description of where the expression
> begins, but not of where it ends.
>
>      getDeclaredSymbols() -- Please document what "possible" in
> "possible FQNs" means.
>
>      IndexResult --
>
>          This class needs a name that is more descriptive of what it is,
> and the name should align with whatever name and comment are finally
> given to getDeclaredSymbols().    NameLookup? ResolvedName? ...
>
>          The class comment should describe what this class is -- and it
> more than a list of FQNs, as it also deals with the simple name,
> up-to-dateness, resolvability.
>          getFqns() -- As above, and probably here rather than there, --
> what does possible mean?
>
>          getSimpleNameLength() -- Says "unresolvable", but there is a
> test (below) for it being resolvable -- probably just remove that word.
> "right left to", were you meaning "right of"?   "for which the
> candidates could be computed" if they can't be computed, how does that
> effect this method?  Wouldn't return still return the length?  Can that
> phrase just be removed?   Is the a reason that the length (rather than
> the name) is returned?
>
>          isUpToDate() -- I have no idea what this means.
>
>          isResolvable() -- Resolvable in the context of what?
>
>      Throughout class, I would avoid "FQN".  See, for example, the use
> of "fully qualified name" in java/lang.  I would spell it out in
> comments.   The java/lang uses "name" as the base for identifiers, but
> this won't work for getFqns() since it needs to be differentiated from
> simple names.  Perhaps getQualifiedNames().
>
>      Nit: Throughout the JShell code and throughout the JDK codebase the
> convention is that comments start on the line after the "/**" (not on
> the same line).
>
> SourceCodeAnalysisImpl:
>
>      getDeclaredSymbols() -- My reading of the spec (above) is that the
> cursor is at the beginning of the identifier to match, this is at the
> end.  Spec needs clarification or ...

The cursor is expected to be immediately after the identifier, I tried 
to clarify that in the javadoc.

>
>      If there is an @author tag on this file, you should certainly be
> first.
>
>      currentIndexes and two other fields are declared in the middle of
> the class.  While old conventions forbade this:
> https://web.archive.org/web/20130627193836/http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141855.html
>
>      others don't when there is good reason.  But I mention this because
> there are many forward references to all three of these -- so their
> implied locality is misleading.
>
>      I would need some code context (comments) to be able to properly
> review.

I've added comments. Please let me know where I need to add more.

>
> ConsoleIOContext:
>
>      Need more context (code comments) for me to be able to review.

I've added comments. Please let me know where I need to add more.

>
> TreeDissector:
>
>      See TypePrinter.
>
> TypePrinter:
>
>      My (unspoken) design for this was to be as stand-alone (untied to
> the JShell architecture) as possible -- esp. since it is subclassed off
> javac, which is why I had that stuff in higher levels.  It could  be
> moved up just one level (to TreeDissector)

Ok, I've moved the method to TreeDissector - TypePrinter would still 
seem to be a nice place for it, to me, though.

>
> ComputeFQNsTest:
>
>      Need tests with multiple answers.  If the other fields of
> IndexResult are interesting enough to be present they should be tested.
>
> InferTypeTest:
>
>      Looks pretty good.  Maybe some other express forms: boolean
> operators, ....

Thanks - there turned out to be a bug with binary operators, should be 
fixed now.

Any comments are welcome!

Thanks,
    Jan

>
> Thanks,
> Robert
>
>
> On 01/22/16 03:40, Jan Lahoda wrote:
>> Hello,
>>
>> I'd like to ask for a review of a patch that adds two new features:
>> a) add import: when there is an unresolvable identifier, press
>> "<base-shortcut> i" (*), and a list of known FQNs should be printed,
>> with an ability to easily import any of them.
>>
>> b) create variable: when there is an expression, press
>> "<base-shortcut> v", and a variable stub will be created, with the
>> expression as the initializer.
>>
>> The "<base-shortcut>" is either Alt-Enter or Alt-F1, depending on the
>> platform. For Windows,
>>
>> The patch is here:
>> http://cr.openjdk.java.net/~jlahoda/8131027/webrev.00/
>>
>> For Windows, an additional patch to jdk.internal.le is needed (I'll
>> send a separate review to core-libs for that):
>> http://cr.openjdk.java.net/~jlahoda/8147984/webrev.00/
>>
>> Any comments are welcome!
>>
>> Thanks,
>>    Jan
>