RFR 8131027: JShell API/tool: suggest imports for a class
Robert Field
robert.field at oracle.com
Mon Feb 15 16:32:28 UTC 2016
On 02/15/16 05:28, Jan Lahoda wrote:
> 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?
You are right. For one, firstCuTree() is ...iterator().next() which
will throw an exception rather than return null. And that exception
should never be thrown since the parser will return a Compilation Unit
tree (not null) in all cases. Retract that. Sorry.
So, thumbs up!
-Robert
>
> 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