RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

On 3/26/19 4:57 AM, Lindenmaier, Goetz wrote:
>>   and also the cases when it requires to look at its caller frame.
> Never.  Only the method that was executed when the exception is
> thrown is needed. I only mention the backtrace because it happens to
> store the top method and the corresponding bytecode position in the
> top method.

That explains why I don't see an example of:
      getNull().m();

      public void m() {};

      static NPE getNull() {
          return null;
      }

For a compiler expert or one who study the data flow analysis paper, is 
this example very clear to them that it won't be supported?

This is related to my comment w.r.t. the data flow analysis comes from.  
I was looking for some level of information be spelled out in the JEP 
that can sufficiently give a non-compiler expert an idea but of course 
no point to replicate the paper here.

It's good to know that this proposal needs the top frame only. Please 
make it clear in
the JEP.    The JEP can take out `StackWalker` but instead talks about 
`StackFrame`
since doing it in Java means that it only needs to get back a 
`StackFrame` instance
representing the top frame stored in `Throwable::backtrace`

This will avoid the confusion on calling StackWalker in the NPE 
constructor and
constructing many StackFrame and many ResolvedMethodName.  That was part
of the code review discussion.
>   
>> "Computing either part of these messages can fail due to insufficient
>> information."
>> What are the cases that it fails to obtain the information? It'd be useful
>> to list some examples if not all.
> (a ? b : c).d
> You can not know whether the ?-expression yielded b or c, thus
> you don't know why the access .d failed.  There is a corresponding example
> in [1].

Please describe in the JEP the known cases that this proposal doesn't 
cover.   Conditional expression is one and null receiver if it's the 
return value from a caller frame (I think this would help the readers).
>> The "Different bytecode variants of a method" section
>>
>> This section raises the case when a method representing an execution stack
>> frame throwing NPE becomes obsolete, it does not have the information
>> to compute the message lazily.  So this falls into the "insufficient
>> information" category and no improved NPE message can be generated.
> Yes, note this also only addresses the method in which the exception was raised.

This should also go to the list of known cases that this proposal does 
not cover.
>> The "computation overhead" section
>>
>> I agree with the requirement to have minimal performance overhead to
>> the NPE construction cost.
>>
>> "NullPointerExceptions are thrown frequently."
>> "Fortunately, most Exceptions are discarded without looking at the message."
>>
>> This is interesting.  Do you have any statistic on the number of NPE thrown
>> and swallowed on certain application/environment that you have gathered?
> No, I can try to generate some numbers, though.  But this assumption was made by
> several others that commented on the previous review thread. For Throwable
> in general, it is also the assumption why the intermediate backtrace data
> structure is implemented instead of generating the stackFrames right away.
>
Since the JEP quotes that NullPointerExceptions are thrown frequently
and swallowed, it would help if you can generate some numbers.

This JEP will help the discussion on the proposed feature and design and 
avoid any
confusion/assumption of the implementation.

>> "If the message is printed based on the backtrace, it must be omitted
>>   if the top frame was dropped from the backtrace."
>>
>> If NPE includes the hidden frames, `Throwable::getStackTrace` and
>> `Throwable::toString` and the VM printing of NPE stack trace needs to
>> filter the hidden frames.  I think it's appropriate for this JEP to
>> cover rather than in a separate JBS issue.
> Hmm, I don't completely understand what you want to say, please
> excuse, I'm a foreign speaker :). I'll give a comprehensive answer:
> To my understanding this is a consequence of the existing JVM/class file
> implementation that calls methods that should be hidden to the user.
> The implementation drops these frames when the backtrace datastructure
> is computed. If this happens, the information needed to compute the
> message is lost, and it can not be printed.

Yes I am familiar with that.
> The fact that this happened must be remembered in the VM
> so that printing the message can be suppressed. Else a message for
> a completely wrong bytecode is printed.

Yup.
> I think the JEP should mention that this is happening.  How this is
> implemented and then pushed to the repo is not subject to a JEP,
> is it?

I think this is related to this JEP.    The question would be:
- should it include all hidden frames?
- should it include the top frame if it's a hidden frame since this 
feature only requires the top frame?

This JEP requires the hidden frame to be recorded in the backtrace. This 
has
impact to the printing of stack trace that needs to know about hidden frames
and decide if it should be filtered or included when printing.

> I made a webrev of its own for this problem, so that the code
> can be looked at isolated of other, orthogonal issues.
> http://cr.openjdk.java.net/~goetz/wr19/8221077-hidden_to_frame/

I don't need the webrev as we are discussing the proposed feature and 
design.
>> The "Message persistance" section
>>
>> I read this section like an implementation details
>   Yes, I had the feeling that I was talking too much about implementation details,
> but mostly in the "Basic algorithm" section.
> This section discusses characteristics of NPE that differ from other exceptions
> and that were proposed in the course of the review of my
> original webrev. They are visible to the Java implementor.
> So I think this is just the basic information needed in a JEP.

This is a good start that allows us to start the discussion.  This JEP 
will probably need several iterations of improvement.  I'm writing a JEP 
that I have iterated many times already and it makes the JEP and 
proposed feature better.   Just to share with you that it's part of the 
process.
>> I think this section
>> intends to call out that the message of NPE if constructed by user code
>> i.e. via public constructor, should not be altered including no-arg
>> constructor or pass a null message to 1-arg constructor.  The enhanced
>> NPE message is proposed only when NPE is thrown due to null dereference
>> when executing bytecode.
> Yes.
>   
>> I don't understand what you tried to say about "npe.getMessage() ==
>> npe.getMessage()".
>> Throwable::getMessage does not guarantee to return same String object for
>> each invocation.
> It does not guarantee so, but it's the case for most exceptions. Usually,
> one calls super(msg) in an Exception constructor.
> It is a design decision to implement it that way, my original proposal
> was different. I'm fine with this design, but it should be mentioned I think.

I think the JEP should be made clearer  what you intend to say.

>> When deserializing the class bytes may be of a different version, so
>> StackTraceElements
>> are serialized with its string form.  Serializing NPE with a computed message
>> seems to be the only viable solution.
> I think implementing writeReplace() is a good idea, too. But
> Roger does not like it: http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-February/058513.html
>

The JEP should record this either if this is an open issue or makes it a 
clear proposal to compute the message when NPE is serialized and what 
the client would do (that may be running in a different version)

>> The "Message content" section
>>
>> I think this section can break down into clear scenarios
>> 1. debug information is present
>> 2. debug information is absent
>> 3. the matching version of bytecode is not available
>>
>> I agree that the message should be easy for Java developers including the
>> beginners to understand the error.
> Hmm, only printing local variable names is affected by the debug information.
> So I don't think a section of its own is necessary.
>
> If the matching bytecode is not available, nothing can be printed.

See my suggestion above to list all known cases that this proposed 
feature will not cover, i.e. no improved message.
> So it does not affect the content of the message. Should I call this
> section "Message wording"?  I think this is closer to what I want to
> discuss in this paragraph.
>
>> You bring up a good question about the exception message format e.g.
>> whether with
>> quotation (none vs single quote vs double quote).  It's good to have a guideline
>> on that while this is out of scope of this JEP.  IAE was updated to include
>> module name, loader name and improve the error message for the developers
>> to understand the exception which is a good improvement.
> IAE quotes the names, which are user defined strings. It does not quote class names, method names etc.
> Here, I was asked to quote these, which is inconsistent with IAE. But there are other
> examples where method names are quoted ... So there is no clear line followed
> yet.  In the end, I don't care, but want advice how to do it. (I would not quote ...).
>

You can add it to the open issues section.

>> "Alternatives" section
>>
>> "The current proposal is to implement this in the Java runtime in C++ accessing
>>   directly the available datastructures in the metaspace."
>>
>> "Also, ASM and StackWalker do not support the full functionality needed.
>> StackWalker must be called in the NullPointerException constructor."
>> This is not true as you wrote in the subsequent sentence.  If we choose
>> the implementation to Java, StackWalker will need to be extended to
>> read Throwable's backtrace.
> The senctence above describes the status quo. The current implementation
> of StackWalker must be called in the constructor.

I just called out the confusion this section could cause.  I think it 
should be updated
or rephase.

>> Such specialized form can provide the
>> ability to fetch the bytecode in Method* if appropriate.  This is
>> the implementation details.  As discussed in the email thread, another
>> implementation choice could be a hybrid of native in the VM and Java
>> to accomplish this task (for example fetching the bytecode of the
>> current version could be done in the VM if we agree supporting
>> the redefinition case).
> Yes, obviously we can implement a lot of solutions. Instead of changing
> StackWalker, we could simply expose [2] java_lang_Throwable::get_method_and_bci()
> to Java in NPE.  After all, we only need the method which happens to be stored
> as top frame in backtrace.
>

That's easy but implementation-detail.

>> For logic that is not strictly needed to be done in the VM, doing it
>> in Java and library is a good option to consider (putting aside the
>> amount of new code you have to write).   Looks like you can't evaluate
>> the alternatives since existing library code requires work in order to
>> prototype this in Java.  So I'd say more investigation needs to be done
>> to decide on alternative implementation approaches.
>   
>> "Testing" section
>>
>> "The basic implementation is in use in SAP's internal Java virtual machine since
>> 2006."
>>
>> This is good information.  This assumes if the same implementation goes into
>> JDK.
>> So this is subject to the discussion and code review.   I think it's adequate to say
>> unit tests will need to be developed.
> There is a unit test in the webrev.  Please point out test cases you think
> are missing. [1] stems from this test.

I am not commenting on [1] but the content of this section.

Please refer to the JEP template (http://openjdk.java.net/jeps/2) about 
the Testing section.

|// What kinds of test development and execution will be required in 
order // to validate this enhancement, beyond the usual mandatory unit 
tests? // Be sure to list any special platform or hardware requirements. |

>
>>   I think this feature should not impact much of the existing VM code
>> and so running existing jtreg tests, jck tests and other existing test suites should
>> provide adequate coverage.
>>
>> I found [1] is quite useful to understand the scenarios this JEP considers.  I
>> think it
>> would be useful to include them in the JEP perhaps as examples when
>> describing
>> the data flow analysis.
> I thought I picked out the most obvious example, pointer chasing, and used it
> in the motivation and the description of the basic algorithm.
>
>>   [1] can be grouped into several categories and the JEP
>> can
>> just talk about the worthnoting groups and does not need to list all messages
>> such as
>> dereference a null receive on getfield, null array element vs null array
> Yes, I think it makes sense to put more messages into the JEP.

To be clear:

What I was suggesting is to describe the scenarios this proposed feature 
will improve the message
but *not* to cut-n-paste the messages from [1].  So this is not tied to 
what the message will be
printed from the implementation.

Writing the scenario in English can help discussing the NPE messages 
should be implemented
that will be easy for Joe developer to understand.

Mandy

> But I would
> like to do that once it is agreed on the messages, so I don't need to
> edit the JEP all the time.
> The file referenced is simply generated by the test, so it's easy to update
> and sure to reflect the implementation.
>
>> This JEP includes a few links to the C++ functions in the current webrev.  I
>> appreciate that and I assume they will be taken out at some point.
> Yes, sure, I can remove that.
>
> Best regards,
>    Goetz.
>