New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullReferenceException - Completely devoid of details #25

Open
vongillern opened this Issue Feb 3, 2015 · 32 comments

Comments

@vongillern

vongillern commented Feb 3, 2015

I don't know if this necessarily the right place for this, but the payoff is big enough to warrant a possible failed attempt.

NullReferenceExceptions are the most common exceptions your average developer is going to encounter, and yet the only information that is included in the exception is the message that something was null and you may or may not get a line number in the stacktrace.

Take the following example:

_orderService.CreateOrder(items.ToList(), customer.CustomerId);

If I get a NullReferenceException on this line, there are three different objects that could have been null and caused the error (_orderService, items and customer). Knowing that it was specifically customer that was null would be incredibly helpful. Extending this concept further, having a small dump of all locally scoped variables when the exception was thrown would be incredibly useful, but I'll go with baby steps and just settle for knowing which variable/expression was the root cause of the null reference exception.

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 3, 2015

It's kind of tricky because not all variable names are available when the exception is thrown. In your example it could be that only _orderService is available by name because it's a member variable (it seems). If items and customers are locals; they're names won't be known when the exception is thrown (unless the PDB is there--but if you have that you can debug it). I can't say for sure, but only providing the names some of the time might have been the impetus for not provide names at all.

@vongillern

This comment has been minimized.

vongillern commented Feb 3, 2015

Agreed. I'm not sure it'd be "easy", but well worth the effort. If the name was not available, including its type in the error message would make it unambiguous 95% of the time. (i.e. NullReferenceException - expression of type 'SuperOrderService' evaluated to null).

My guess though, is that if the PDB is available, it would know the character ranges of the null expression. If i turn on "break on thrown exceptions" in the exceptions dialog box, it always breaks me in at the right point and I can inspect all of my variables. I just want a dump so I don't have to be actively debugging.

@svick

This comment has been minimized.

Contributor

svick commented Feb 3, 2015

@vongillern I think you can easily make that even better by including the name of the invoked method. Something like (mostly copying the existing message):

NullReferenceException: Object reference not set to an instance of an object when calling 'SuperOrderService.CreateOrder'.

This shouldn't be that hard to do, since the callvirt instruction that causes the vast majority of NREs knows which method it was.

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 4, 2015

@svick that can be problematic too because the IL is really a representation of something like this:

var list = items.ToList();
var customerId = customer.CustomerId;
_orderService.CreateOrder(list, customerId);

And that's if the compiler(s) decided to put the retrieval of the param values next to the method call.

I expect there'd be a bit of work to associate those variables to a callvirt several instructions ahead of where the null value is actually found. And if that value got cached/reused, it would make it a bit more complex. I think "easily" is very optimistic. If were easy, I'm sure someone would have done it before now.

@svick

This comment has been minimized.

Contributor

svick commented Feb 4, 2015

@peteraritchie Yeah, I understand that associating callvirt with a variable may not be easy, but that's not what I'm talking about. What I meant is that the message would contain the type (as @vongillern suggested) and method that caused the NRE.

So, in the example, the message wouldn't tell you that _orderService is null, but it would tell you that the NRE happened when calling SuperOrderService.CreateOrder(), which should be sufficient to debug the issue.

Or maybe I misunderstood what you're saying?

@Alexx999

This comment has been minimized.

Contributor

Alexx999 commented Feb 4, 2015

I believe that NPE can be debugged in quite straightforward manner using crashdumps and WinDBG right now.
Adding some extra data is not that common for low-level exceptions, and NPEs are not the worst case (my personal favorite is "FileNotFoundException" when some assembly fails to resolve some dependency - you'll newer know which one actually failed).
Plus, it will surely hurt performance - exceptions are slow as it is, and NPEs are often caught.

@svick

This comment has been minimized.

Contributor

svick commented Feb 4, 2015

I believe that NPE can be debugged in quite straightforward manner using crashdumps and WinDBG right now.

I don't think that anything that involves crashdumps or WinDBG can be called straightforward. Especially if you consider that NRE is an exception that I think beginners often encounter.

NPEs are often caught

I hope that you are wrong about this. NRE pretty much always indicates a bug and catching it just hides that bug.

@stimms

This comment has been minimized.

stimms commented Feb 4, 2015

I certainly agree with @svick on this. One of the primary goals of .net should be to be as productive of an environment as possible. We should attempt to minimize the scenarios in which a developer needs to attach a debugger or examine a crash dump as they trend to be very time consuming operations.

With hundreds of thousands of developers encountering NPEs it doesn't take long for the time savings to really add up. I'm in favor of any action that gives us better error messages.

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 4, 2015

I'm not saying don't do it or that there's no value to it, just that it's not going to be that easy. As soon as you provide * some * detail, you have to provide detail in all circumstances or there will be lots of bugs logged

@Alexx999

This comment has been minimized.

Contributor

Alexx999 commented Feb 4, 2015

@svick well, I probably just got used to it. Anyway, I surely agree that exception messages are often bad.

@yaakov-h

This comment has been minimized.

yaakov-h commented Feb 4, 2015

I agree with this. I've sometimes had to resort to checking the IL offset of the exception location and compare it with a disassembled assembly just to figure out exactly which reference was null.

The more information about an error, the better.

@akoeplinger

This comment has been minimized.

Member

akoeplinger commented Feb 4, 2015

+1, anything that can be done to add more details to this exception should be investigated.

@jkotas Thanks for the link, but that UserVoice is already 4 years old and has gotten a bunch of votes without an official statement from the team. Did you investigate a solution already? Did you run into problems or other concerns? Would love to learn more details :)

@jkotas

This comment has been minimized.

Member

jkotas commented Feb 4, 2015

The project management team looks at top uservoice requests regularly and works on getting them addressed. It does not matter that the UserVoice is 4 years old - it just means that it did not gather enough votes to be in the top list yet.

For example, UserVoice votes were one of the reasons for adding SIMD support to CLR - http://visualstudio.uservoice.com/forums/121579-visual-studio/suggestions/2212443-c-and-simd

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 4, 2015

@jkotas is there someone on the team(s) that have looked at anything like this comment on this thread? Having some detail on what has been researched so far in terms of what would need to change in the code would give the community something to hit the ground running with for a potential PR.

@vongillern

This comment has been minimized.

vongillern commented Feb 4, 2015

+1 @peteraritchie, a comment from the team would be helpful. I spent a little time looking at it and the project is obviously so massive, I had a hard time knowing where I could even start.

@jkotas

This comment has been minimized.

Member

jkotas commented Feb 4, 2015

@CarolEidt would you like to comment on this?

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 4, 2015

@vongillern Right, it may be easy for the person on the CLR team that has spent the most time in that code; but that doesn't mean that person has the time to perform the change. (if that were the case, this may have been done already). For the community to do it, however acceptable that is to do, is likely not a quick task.

@CarolEidt

This comment has been minimized.

Member

CarolEidt commented Feb 4, 2015

@jkotas although I could imagine that the JIT could participate in a solution (through improved debug/unwind info if there are imprecisions), the main work would lie in getting System.Environment.GetStackTrace to report column numbers (which I think would give the desired granularity). @vongillern - System.Environment.GetStackTrace is in clr\src\mscorlib\src\System\Exception.cs. That might be a good place to start, though debug info (beyond the IL offsets that the JIT reports) is not really my area of expertise.

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 4, 2015

👍

@mikedn

This comment has been minimized.

Contributor

mikedn commented Feb 4, 2015

The column number can be obtained by passing the exception object to the stack trace class, that's trivial. But there's no useful column number that you can get from a PDB, the sequence points generated by the C# (and likely any other) compiler usually track entire statements. One would need to first make the compiler emit sequence points for sub-expressions and that's problematic partly because the debug information would become larger, partly because sub-expressions can overlap and inevitably leads to overlapping sequence points. This may end up requiring changes to the debug format.

The JIT compiler too seems to track IL offsets at statement level (or an approximation of a statement).

To me this approach sounds like a non-starter. A lot of work for questionable benefit. And it's likely that it will only work properly with debug binaries.

Another approach might be to have the runtime figure out what was variable was stored in the base/index register of the memory operand that caused the access violation. The debugger can usually map a variable to a register to display its value so maybe doing the opposite wouldn't be too much trouble. But then again, this will probably work correctly only with debug binaries.

@jkotas

This comment has been minimized.

Member

jkotas commented Feb 4, 2015

Correct, the JIT and PDB do not have enough precision in debug info today to reliably identify the source of NullReferenceException. For example, the following C# statement:

        foo(a.ToString(), b.ToString(), c.ToString());

has just one sequence point, and so it is not possible to identify from just the NullReferenceException location and debug info which one of a, b or c was null. If you would like to see the good error message even with JIT optimizations on, it is even harder because of the debug info tracking is not guaranteed to be preserved by the JIT optimizations today.

I think that the first step to attack this problem would be to write a design proposal about possible approaches. The ones identified by @mikedn would be a good start. Focus on:

  • The rough sketch of extra information that would flow between the different components in the system
  • The user experience - how good and reliable would be the exception messages
  • The estimated overhead in disk footprint, RAM and CPU cycles

I know it is a lot of work...

@EmJayGee

This comment has been minimized.

EmJayGee commented Feb 4, 2015

Wouldn't this feature overall be better cast as a language/compiler feature
rather than of the runtime? The end goal is easily mapping back to the
source; debug info is designed to assist that but if we want things like
the identifier/expression text, wouldn't this best be handled by the
compiler?

I'm assuming that this kind of information being available only in debug
builds with symbols available to the runtime will be a non-starter.

Mike

On Wed, Feb 4, 2015 at 2:31 PM, Jan Kotas notifications@github.com wrote:

Correct, the JIT and PDB do not have enough precision in debug info today
to reliably identify the source of NullReferenceException. For example, the
following C# statement:

    foo(a.ToString(), b.ToString(), c.ToString());

has just one sequence point, and so it is not possible to identify from
just the NullReferenceException location and debug info which one of a, b
or c was null. If you would like to see the good error message even with
JIT optimizations on, it is even harder because of the debug info tracking
is not guaranteed to be preserved by the JIT optimizations today.

I think that the first step to attack this problem would be to write a
design proposal about possible approaches. The ones identified by @mikedn
https://github.com/mikedn would be a good start. Focus on:

  • The rough sketch of extra information that would flow between the
    different components in the system
  • The user experience - how good and reliable would be the exception
    messages
  • The estimated overhead in disk footprint, RAM and CPU cycles

I know it is a lot of work...


Reply to this email directly or view it on GitHub
#25 (comment).

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 4, 2015

@EmJayGee Starting to sound a lot like contracts, or the very least code weaving. I think if were approached form the compiler-side it may never get done. Keep in mind there's 2-3 compilers in play for any piece of code: C#/VB, JIT 32-bit, and JIT 64-bit.

@OtherCrashOverride

This comment has been minimized.

OtherCrashOverride commented Feb 5, 2015

Exceptions should be exceptional. If you think something may be null, you should check that condition in code to ensure proper operation. I think this is more an issue of coding practice than a feature for the runtime. The Common Language Runtime is, as the name implies, common among many languages so it would likely require strategies for what makes sense for each language to produce a more useful message targeted at the developer using that language.

Therefore, I suggest the proper place for this is the IDE where both source code and language are known. NullReferenceException should be a rare occurrence if good coding practices are observed.

(For an example, see F#'s use of null versus C#)

@rafabertholdo

This comment has been minimized.

rafabertholdo commented Feb 11, 2015

In my opinion, null.something should return null instead of raising an exception.
http://visualstudio.uservoice.com/forums/121579-visual-studio/suggestions/5728581-add-and-assembly-attribute-for-the-compiler-to-byp

Fortunately, C# 6.0 brings an operator to make it happen, but I think the code will be dirty. This should be a compiler attribute.

@peteraritchie

This comment has been minimized.

peteraritchie commented Feb 11, 2015

@rafabertholdo Wouldn't that just defer a null exception to higher-up? That seems like it would make the root cause of a null reference exception even harder to find.

@yaakov-h

This comment has been minimized.

yaakov-h commented Feb 13, 2015

Propagating null instead of erroring out (non-explicitly) is a bad idea, IMO. The collective experience of almost every single Objective-C developer stands against that behaviour.

@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Mar 14, 2015

Issue ping. Is there a concrete action here?

@jkotas

This comment has been minimized.

Member

jkotas commented Mar 14, 2015

A number of issues in this repo track hard problems with unclear solutions. This is one of them. Our intent is to keep them open to facilitate discussion about solutions. I have created "hard problem" label to tag them.

cc @richlander

@dsplaisted

This comment has been minimized.

Member

dsplaisted commented Sep 27, 2017

It looks like VS 2017 will show the information you want when targeting .NET Framework 4.6.2 or higher: https://blogs.msdn.microsoft.com/devops/2016/03/31/using-the-new-exception-helper-in-visual-studio-15-preview/

Null reference debug helper

I'm not sure whether it's enabled in .NET Core

@lt72 lt72 added the enhancement label Nov 30, 2017

@alexsorokoletov

This comment has been minimized.

alexsorokoletov commented Jan 5, 2018

This feature is indeed available in VS 2017, article says it is not available for .NET Core/UWP, only available for .NET 4.6.2.

Would be really great to have that not in the VS but in .NET Core/Mono so that we can have that information in the crash reports rather than when debugging in VS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment