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

Fri Feb 12 21:47:34 UTC 2016

Hi Robert,

Thanks for the comments. I've uploaded a new iteration here:
http://cr.openjdk.java.net/~jlahoda/8131027/webrev.02/

Delta webrev:
http://cr.openjdk.java.net/~jlahoda/8131027/webrev.02/delta/webrev/index.html

On 11.2.2016 08:43, Robert Field wrote:
> HI Jan,
>
> Review of version 01 ---
>
> ConsoleIOContext.java --
>
>     fixes() -- on unexpected character it just cycles, waiting for
>     another, for a user stuck where not wanted that could be confusing.
>     If a user is here by mistake and typing ahead then ignoring
>     unexpected characters increases the chance that a random character
>     will have an unexpected effect.  Some options (depending on the
>     perceived likelihood of legitimately typing a wrong char: (1) no
>     loop, if an unexpected char beep and exit  (2) loop for a small
>     number of times, three?, beeping on bad input. (3) print some help
>     "Type a number" on bad input.

I've removed the loop.

>
>     FixComputer[] -- FQN is used in user error:  "\nNo candidate FQNs
>     found to import."

Fixed.

>
>
> Eval.java --
>
>     OK
>
>
> JShell.java --
>
>     OK
>
>
> SourceCodeAnalysis.java --
>
>     analyzeType() & listQualifiedNames() -- Nice javadoc.
>
>     QualifiedNames --
>
>         The constructor is only use by us, exposing in the API is messy
>         and potentially confusing.  Luckily the solution is just to
>         remove "public" off the constructor, since package-private is
>         all we need. (while you are at it, can you tuck CompletionInfo
>         constructor in too (impl used to be in a separate package)?)

Fixed.

>
>         isResolvable() -- the methods above refer to "the simple name in
>         the original code".  This calls it "the given identifier", I
>         assume this is the same thing?  I'd stick with "the simple name
>         in the original code"

Fixed.

>
> SourceCodeAnalysisImpl.java --
>
>     analyzeType() --
>
>         It aborts with null when not complete, which seems fine.  Why
>         does it proceed (and add a semicolon) when the input in empty --
>         I'd think it should also abort.

It turned out the "empty input" section was unnecessary - 
Completeness.EMPTY has isComplete == false, so the method should 
automatically abort for the empty input.

>         Seems you would want to add the semicolon when completeness ==
>         COMPLETE_WITH_SEMI.

Done.

>         The TODO comment says "comment handling" -- this is what
>         MaskCommentsAndModifiers.java does -- just plug into that -- see
>         Eval.eval().

The empty input section has been removed.

>         IMPORT and METHOD are rejected, but I'd think CLASS, ENUM,
>         ANNOTATION_TYPE, INTERFACE, and VARIABLE would be as well.

Yes, thanks; done.

>
> ... to be continued ...
>
> -------
>
> On mi Fedora (KDE-spin) machine I could get neither Alt-f1 or Alt-Enter
> to work.
>
> Send me a sketch of what user docs would say and I'll incorporate it in
> /help

I've added the help text into the review.

>
On 11.2.2016 20:50, Robert Field wrote:> Continuing (see below for first 
part) ...
>
> SourceCodeAnalysisImpl.java --
>
>     Both listQualifiedNames() and analyzeType() -- at.firstCuTree() can
>     be null, are you safe if that happens?

The code/wrap that is being passed to AnalyzeTask should ensure there is 
always a CompilationUnit, I think. Or do I miss some case where that 
wouldn't happen? (I've found a few bugs and added some more tests while 
looking at this.)

Any comments are welcome.

Thanks!

Jan

>
>
> TreeDissector.java --
>
>     Looks good
>
>
> Tests --
>
>     Look good.
>
>
> Thanks,
> Robert
>
>
>> On Feb 9, 2016, at 5:03 AM, Jan Lahoda <jan.lahoda at oracle.com
>> <mailto:jan.lahoda at oracle.com>> wrote:
>>
>> 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
>