RFR 8131027: JShell API/tool: suggest imports for a class
Jan Lahoda
jan.lahoda at oracle.com
Mon Feb 15 13:28:58 UTC 2016
Hi Robert,
I am sorry to oppose here, but I couldn't find a way to get a null from
BaseTask.firstCuTree() - that would mean that the "cuts" Iterable would
need to contain null. "cuts" is filled from JavacTask.parse, and I don't
see a way that could contain null (JavacTaskImpl.parseInternal is even
dereferencing all CompilationUnitTrees in the list). The
JavacParser(/ReplParser).parseCompilationUnit should always return a
non-null value, as far as I can tell (even for an empty file, there
should be an (empty/without any ClassTrees) CompilationUnitTree instance
created). Is there a particular case where firstCuTree() returns null?
Thanks,
Jan
On 12.2.2016 23:19, Robert Field wrote:
> A sufficiently bad syntax error could mean no compilation unit. A null
> test and I'm thumbs up!
>
> -Robert
>
>
>
> On February 12, 2016 1:47:39 PM Jan Lahoda <jan.lahoda at oracle.com> wrote:
>
>> 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
>>>
>
>
More information about the kulla-dev
mailing list