RFR: JDK-8189778: Jshell crash on tab for StringBuilder.append(

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8189778: Jshell crash on tab for StringBuilder.append(

Jan Lahoda
Hi,

Typing:
jshell> StringBuilder sb = new StringBuilder();
jshell> sb.append(<tab><tab>

Will lead to an exception:
jshell> sb.append(Exception in thread "main"
java.lang.StringIndexOutOfBoundsException: start -59, end -59, length 238
[snip]
jdk.compiler/jdk.internal.shellsupport.doc.JavadocHelper$OnDemandJavadocHelper.getResolvedDocComment(JavadocHelper.java:481)

The reason is that the javadoc for StringBuilder.append(CharSequence,
int, int) is:
     /**
      * @throws     IndexOutOfBoundsException {@inheritDoc}
      */

The JavadocHelper tries to fill in the missing javadoc entries, but that
fails because it tries to insert the preceding entries at an
uninitialized place.

The proposed patch:
-fixes the above
-adds a test that runs the JavadocHelper on all
methods/fields/constructors of top-level types in all exported packages
(this runs for a considerable time, and we may need to disable this test
if it proves to be too heavyweight)
-adds a few more test cases for problems found by the above test
-fixes the javadoc resolution to treat missing javadoc body as
{@inheritDoc} (so that the overridden method's javadoc text is used if
missing in this method)

Bug: https://bugs.openjdk.java.net/browse/JDK-8189778
Webrev: http://cr.openjdk.java.net/~jlahoda/8189778/webrev.00/

Feedback is welcome.

Thanks,
     Jan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189778: Jshell crash on tab for StringBuilder.append(

Jan Lahoda
I've updated to patch to have some comments at places that seemed important:
http://cr.openjdk.java.net/~jlahoda/8189778/webrev.01/

Jan

On 26.10.2017 11:35, Jan Lahoda wrote:

> Hi,
>
> Typing:
> jshell> StringBuilder sb = new StringBuilder();
> jshell> sb.append(<tab><tab>
>
> Will lead to an exception:
> jshell> sb.append(Exception in thread "main"
> java.lang.StringIndexOutOfBoundsException: start -59, end -59, length 238
> [snip]
> jdk.compiler/jdk.internal.shellsupport.doc.JavadocHelper$OnDemandJavadocHelper.getResolvedDocComment(JavadocHelper.java:481)
>
>
> The reason is that the javadoc for StringBuilder.append(CharSequence,
> int, int) is:
>      /**
>       * @throws     IndexOutOfBoundsException {@inheritDoc}
>       */
>
> The JavadocHelper tries to fill in the missing javadoc entries, but that
> fails because it tries to insert the preceding entries at an
> uninitialized place.
>
> The proposed patch:
> -fixes the above
> -adds a test that runs the JavadocHelper on all
> methods/fields/constructors of top-level types in all exported packages
> (this runs for a considerable time, and we may need to disable this test
> if it proves to be too heavyweight)
> -adds a few more test cases for problems found by the above test
> -fixes the javadoc resolution to treat missing javadoc body as
> {@inheritDoc} (so that the overridden method's javadoc text is used if
> missing in this method)
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189778
> Webrev: http://cr.openjdk.java.net/~jlahoda/8189778/webrev.00/
>
> Feedback is welcome.
>
> Thanks,
>      Jan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189778: Jshell crash on tab for StringBuilder.append(

Kumar Srinivasan

Hi Jan,

Generally it looks fine to me, a few nits...

JavadocHelper.java

1.

-                        //which will be resolve in visitInheritDoc:
suggest....
+                        //which will be resolved in visitInheritDoc:
   

2. Suggest adding a space after /** or /* makes it a little easier
to read the comments.

3.

- //insertPos (as future missing elements should be inserted behind
+ //insertPos (as future missing elements should be inserted before

4.
-  //if there is a newline immediatelly after this tree, insert after
+   //if there is a newline immediately after this tree, insert after

5. Several private methods have have javadoc comments /**, do you
run javadoc -private on jshell sources ?

You may want to get Jon's review on this, he is much more familiar
with these things, than I.

Thanks
Kumar

> I've updated to patch to have some comments at places that seemed
> important:
> http://cr.openjdk.java.net/~jlahoda/8189778/webrev.01/
>
> Jan
>
> On 26.10.2017 11:35, Jan Lahoda wrote:
>> Hi,
>>
>> Typing:
>> jshell> StringBuilder sb = new StringBuilder();
>> jshell> sb.append(<tab><tab>
>>
>> Will lead to an exception:
>> jshell> sb.append(Exception in thread "main"
>> java.lang.StringIndexOutOfBoundsException: start -59, end -59, length
>> 238
>> [snip]
>> jdk.compiler/jdk.internal.shellsupport.doc.JavadocHelper$OnDemandJavadocHelper.getResolvedDocComment(JavadocHelper.java:481)
>>
>>
>>
>> The reason is that the javadoc for StringBuilder.append(CharSequence,
>> int, int) is:
>>      /**
>>       * @throws     IndexOutOfBoundsException {@inheritDoc}
>>       */
>>
>> The JavadocHelper tries to fill in the missing javadoc entries, but that
>> fails because it tries to insert the preceding entries at an
>> uninitialized place.
>>
>> The proposed patch:
>> -fixes the above
>> -adds a test that runs the JavadocHelper on all
>> methods/fields/constructors of top-level types in all exported packages
>> (this runs for a considerable time, and we may need to disable this test
>> if it proves to be too heavyweight)
>> -adds a few more test cases for problems found by the above test
>> -fixes the javadoc resolution to treat missing javadoc body as
>> {@inheritDoc} (so that the overridden method's javadoc text is used if
>> missing in this method)
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189778
>> Webrev: http://cr.openjdk.java.net/~jlahoda/8189778/webrev.00/
>>
>> Feedback is welcome.
>>
>> Thanks,
>>      Jan

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189778: Jshell crash on tab for StringBuilder.append(

Jan Lahoda
Hi Kumar,

Thanks for the comments.

On 2.11.2017 13:35, Kumar Srinivasan wrote:

>
> Hi Jan,
>
> Generally it looks fine to me, a few nits...
>
> JavadocHelper.java
>
> 1.
>
> -                        //which will be resolve in visitInheritDoc:
> suggest....
> +                        //which will be resolved in visitInheritDoc:
>
> 2. Suggest adding a space after /** or /* makes it a little easier
> to read the comments.
>
> 3.
>
> - //insertPos (as future missing elements should be inserted behind
> + //insertPos (as future missing elements should be inserted before
>
> 4.
> -  //if there is a newline immediatelly after this tree, insert after
> +   //if there is a newline immediately after this tree, insert after

Will do the above.

>
> 5. Several private methods have have javadoc comments /**, do you
> run javadoc -private on jshell sources ?

No, but a lot javac code uses /** comments to comment methods (even if
they are not a full javadoc comment, like Attr.isType), so I simply kept
using the same strategy. (IDEs may also be able to show the javadoc
comments in code completion.)

Thanks,
     Jan

>
> You may want to get Jon's review on this, he is much more familiar
> with these things, than I.
>
> Thanks
> Kumar
>
>> I've updated to patch to have some comments at places that seemed
>> important:
>> http://cr.openjdk.java.net/~jlahoda/8189778/webrev.01/
>>
>> Jan
>>
>> On 26.10.2017 11:35, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Typing:
>>> jshell> StringBuilder sb = new StringBuilder();
>>> jshell> sb.append(<tab><tab>
>>>
>>> Will lead to an exception:
>>> jshell> sb.append(Exception in thread "main"
>>> java.lang.StringIndexOutOfBoundsException: start -59, end -59, length
>>> 238
>>> [snip]
>>> jdk.compiler/jdk.internal.shellsupport.doc.JavadocHelper$OnDemandJavadocHelper.getResolvedDocComment(JavadocHelper.java:481)
>>>
>>>
>>>
>>> The reason is that the javadoc for StringBuilder.append(CharSequence,
>>> int, int) is:
>>>      /**
>>>       * @throws     IndexOutOfBoundsException {@inheritDoc}
>>>       */
>>>
>>> The JavadocHelper tries to fill in the missing javadoc entries, but that
>>> fails because it tries to insert the preceding entries at an
>>> uninitialized place.
>>>
>>> The proposed patch:
>>> -fixes the above
>>> -adds a test that runs the JavadocHelper on all
>>> methods/fields/constructors of top-level types in all exported packages
>>> (this runs for a considerable time, and we may need to disable this test
>>> if it proves to be too heavyweight)
>>> -adds a few more test cases for problems found by the above test
>>> -fixes the javadoc resolution to treat missing javadoc body as
>>> {@inheritDoc} (so that the overridden method's javadoc text is used if
>>> missing in this method)
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189778
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8189778/webrev.00/
>>>
>>> Feedback is welcome.
>>>
>>> Thanks,
>>>      Jan
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189778: Jshell crash on tab for StringBuilder.append(

Jan Lahoda
In reply to this post by Kumar Srinivasan
Hi,

A webrev updated to the current state and with some further fixes is here:
http://cr.openjdk.java.net/~jlahoda/8189778/webrev.02/

On 2.11.2017 13:35, Kumar Srinivasan wrote:

>
> Hi Jan,
>
> Generally it looks fine to me, a few nits...
>
> JavadocHelper.java
>
> 1.
>
> -                        //which will be resolve in visitInheritDoc:
> suggest....
> +                        //which will be resolved in visitInheritDoc:

Done.

>
> 2. Suggest adding a space after /** or /* makes it a little easier
> to read the comments.

Done.

>
> 3.
>
> - //insertPos (as future missing elements should be inserted behind
> + //insertPos (as future missing elements should be inserted before

The missing elements are inserted behind the tree that was visited by
the method.

>
> 4.
> -  //if there is a newline immediatelly after this tree, insert after
> +   //if there is a newline immediately after this tree, insert after

Done. Also changed "after" to "behind".

>
> 5. Several private methods have have javadoc comments /**, do you
> run javadoc -private on jshell sources ?

Changed to comments to /*.

Any feedback is welcome!

Thanks,
    Jan

>
> You may want to get Jon's review on this, he is much more familiar
> with these things, than I.
>
> Thanks
> Kumar
>
>> I've updated to patch to have some comments at places that seemed
>> important:
>> http://cr.openjdk.java.net/~jlahoda/8189778/webrev.01/
>>
>> Jan
>>
>> On 26.10.2017 11:35, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Typing:
>>> jshell> StringBuilder sb = new StringBuilder();
>>> jshell> sb.append(<tab><tab>
>>>
>>> Will lead to an exception:
>>> jshell> sb.append(Exception in thread "main"
>>> java.lang.StringIndexOutOfBoundsException: start -59, end -59, length
>>> 238
>>> [snip]
>>> jdk.compiler/jdk.internal.shellsupport.doc.JavadocHelper$OnDemandJavadocHelper.getResolvedDocComment(JavadocHelper.java:481)
>>>
>>>
>>>
>>> The reason is that the javadoc for StringBuilder.append(CharSequence,
>>> int, int) is:
>>>      /**
>>>       * @throws     IndexOutOfBoundsException {@inheritDoc}
>>>       */
>>>
>>> The JavadocHelper tries to fill in the missing javadoc entries, but that
>>> fails because it tries to insert the preceding entries at an
>>> uninitialized place.
>>>
>>> The proposed patch:
>>> -fixes the above
>>> -adds a test that runs the JavadocHelper on all
>>> methods/fields/constructors of top-level types in all exported packages
>>> (this runs for a considerable time, and we may need to disable this test
>>> if it proves to be too heavyweight)
>>> -adds a few more test cases for problems found by the above test
>>> -fixes the javadoc resolution to treat missing javadoc body as
>>> {@inheritDoc} (so that the overridden method's javadoc text is used if
>>> missing in this method)
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189778
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8189778/webrev.00/
>>>
>>> Feedback is welcome.
>>>
>>> Thanks,
>>>      Jan
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189778: Jshell crash on tab for StringBuilder.append(

Jonathan Gibbons
OK.

Long term, we should have a utility class/method somewhere, either in DocTrees, or in the jdk.javadoc module, to provide "official" support for obtaining a doc comment with `{@inheritDoc}` resolved.  One gotcha in that sort of code that we are becoming more aware of, is the need to handle inline references using `<a href="link">`. At least for you in this Shell console world, that is not such an issue.

-- Jon

On 12/06/2017 01:11 PM, Jan Lahoda wrote:
Hi,

A webrev updated to the current state and with some further fixes is here:
http://cr.openjdk.java.net/~jlahoda/8189778/webrev.02/

On 2.11.2017 13:35, Kumar Srinivasan wrote:

Hi Jan,

Generally it looks fine to me, a few nits...

JavadocHelper.java

1.

-                        //which will be resolve in visitInheritDoc:
suggest....
+                        //which will be resolved in visitInheritDoc:

Done.


2. Suggest adding a space after /** or /* makes it a little easier
to read the comments.

Done.


3.

- //insertPos (as future missing elements should be inserted behind
+ //insertPos (as future missing elements should be inserted before

The missing elements are inserted behind the tree that was visited by the method.


4.
-  //if there is a newline immediatelly after this tree, insert after
+   //if there is a newline immediately after this tree, insert after

Done. Also changed "after" to "behind".


5. Several private methods have have javadoc comments /**, do you
run javadoc -private on jshell sources ?

Changed to comments to /*.

Any feedback is welcome!

Thanks,
   Jan


You may want to get Jon's review on this, he is much more familiar
with these things, than I.

Thanks
Kumar

I've updated to patch to have some comments at places that seemed
important:
http://cr.openjdk.java.net/~jlahoda/8189778/webrev.01/

Jan

On 26.10.2017 11:35, Jan Lahoda wrote:
Hi,

Typing:
jshell> StringBuilder sb = new StringBuilder();
jshell> sb.append(<tab><tab>

Will lead to an exception:
jshell> sb.append(Exception in thread "main"
java.lang.StringIndexOutOfBoundsException: start -59, end -59, length
238
[snip]
jdk.compiler/jdk.internal.shellsupport.doc.JavadocHelper$OnDemandJavadocHelper.getResolvedDocComment(JavadocHelper.java:481)



The reason is that the javadoc for StringBuilder.append(CharSequence,
int, int) is:
     /**
      * @throws     IndexOutOfBoundsException {@inheritDoc}
      */

The JavadocHelper tries to fill in the missing javadoc entries, but that
fails because it tries to insert the preceding entries at an
uninitialized place.

The proposed patch:
-fixes the above
-adds a test that runs the JavadocHelper on all
methods/fields/constructors of top-level types in all exported packages
(this runs for a considerable time, and we may need to disable this test
if it proves to be too heavyweight)
-adds a few more test cases for problems found by the above test
-fixes the javadoc resolution to treat missing javadoc body as
{@inheritDoc} (so that the overridden method's javadoc text is used if
missing in this method)

Bug: https://bugs.openjdk.java.net/browse/JDK-8189778
Webrev: http://cr.openjdk.java.net/~jlahoda/8189778/webrev.00/

Feedback is welcome.

Thanks,
     Jan