Add a corresponding Debug.Assert call

[haacked]

If you call a method from within a try/catch statement, the fact that it throws ArgumentException could be swallowed and unknown to you. Same kind of thing happens in an async method call (without await of course).

But Debug.Assert will make sure that such things are visible to the user when running the application in debug by throwing a modal dialog.

And it does nothing when built in RELEASE mode.

[SimonCropp]

@Haacked can you push a breaking unit test?

[haacked]

Well a unit test would only apply in a debug build. In fact, this makes things really tricky. If you do this, then all your existing unit tests would be invalid in a DEBUG build.

One thing I've done in my code is override this behavior when running in a unit test. But that's weird code to add to this. Need to think it over a bit.

[haacked]

In fact, not sure you can write a unit test. Keep in mind this is a diagnostic feature. It throws up a MODAL DIALOG. You wouldn't want that happening during your unit test runs. ;)

So that gives me an idea, maybe there's a compilation constant we could use to create a custom build for CI and the unit test runner. The only time you want Debug.Assert is when testing a debug build of your app.

[SimonCropp]

We could ever wrap Debug.Assert in a delegate and then inject a stub at test time

On Wed, Feb 6, 2013 at 9:11 AM, Phil Haack notifications@github.com wrote:

In fact, not sure you can write a unit test. Keep in mind this is a diagnostic feature. It throws up a MODAL DIALOG. You wouldn't want that happening during your unit test runs. ;)

So that gives me an idea, maybe there's a compilation constant we could use to create a custom build for CI and the unit test runner. The only time you want Debug.Assert is when testing a debug build of your app.

— Reply to this email directly or view it on GitHubhttps://github.com/Fody/NullGuard/issues/13#issuecomment-13154945.

[SimonCropp]

there is also this

((DefaultTraceListener)Debug.Listeners[0]).AssertUiEnabled = false;

On Wed, Feb 6, 2013 at 9:51 AM, Simon Cropp simon.cropp@gmail.com wrote:

We could ever wrap Debug.Assert in a delegate and then inject a stub at test time

On Wed, Feb 6, 2013 at 9:11 AM, Phil Haack notifications@github.comwrote:

In fact, not sure you can write a unit test. Keep in mind this is a diagnostic feature. It throws up a MODAL DIALOG. You wouldn't want that happening during your unit test runs. ;)

So that gives me an idea, maybe there's a compilation constant we could use to create a custom build for CI and the unit test runner. The only time you want Debug.Assert is when testing a debug build of your app.

— Reply to this email directly or view it on GitHubhttps://github.com/Fody/NullGuard/issues/13#issuecomment-13154945.

[haacked]

Nice!

[distantcam]

Is it a problem if a try/catch catches the error from the calling method? Presumably the try/catch is there for a reason, and that the dev has put in logging, or otherwise recorded the exception themselves.

I wouldn't want the code in my try/catch to suddenly get more noisy. That's why I put you in a try/catch-log in the first place.

[haacked]

@distantcam imho, catching an ArgumentException and ArgumentNullException is indicative of a bug in the code. So I'd want to be warned about that. It often happens when someone is overreaching and catching all exceptions.

In Eric Lippert's taxonomy of exceptions, I'd put argument exceptions (and null refs) in the Boneheaded category. They're indicative of programming error, not due to exogenous circumstances.

I'd support a setting to turn off debug asserts in the attribute, but think it makes sense to turn them on by default. Keep in mind that these are compiled out of release versions so they won't affect your release application.

[haacked]

One other point. You can also get in situations where exceptions on other threads are swallowed (depending on how you create the background thread). So that'd be another case where the Debug.Assert would alert you to a problem you might not otherwise notice.

[distantcam]

Alright, I'll have to chew on this one a bit. Haven't had my morning coffee yet.

Eric's article looks interesting, but I'd say that rather than classifying the exceptions there are also reasons why you want to catch things.

Catch because you have to would catch everything except bonehead exceptions.

Catch because this app is a service and must not stop for anything is admittedly a rare occurrence, but still valid. I do have this scenario at work and Debugs would hurt that.

But, as you say, they would only appear in debug builds, and if they're optional too then you can turn them off.

[haacked]

Yeah, that wouldn't affect your service. Also, I'd argue that crashing the service might be the better thing depending on the service. For example, imagine that your service manages microtransactions and by continuing, it's making transactions off of bad data due to the exception. Now your service is now compounding that mistake into a million dollar mistake rather than crashing and getting someone to look at it immediately.

Point being, if you allow erroneous code to continue to run, you have no way to predict if the result is worse than crashing the app or not. Having the app not run gives you a known quantity. You can mitigate that. Much harder to mitigate the unknown.

But that's a conversation for another time. :)

[SimonCropp]

@Haacked when you say "Same kind of thing happens in an async method call" I assume you mean some code like this

public async Task Method(string foo)
{
    await Task.Delay(1);
}

And your concern is that the Null check will get pulled into the async code?

If so this is not the case because of how the null check is done the resultant code will look something like this

public Task Method(string foo)
{
    if (foo == null)
    {
        throw new ArgumentNullException("foo");
    }
    <Method>d__0 d__;
    d__.<>4__this = this;
    d__.foo = foo;
    d__.<>t__builder = AsyncTaskMethodBuilder.Create();
    d__.<>1__state = -1;
    d__.<>t__builder.Start<<Method>d__0>(ref d__);
    return d__.<>t__builder.Task;
}

So as you can see the null check will be performed synchronously and not be hidden by the async exception handler

Or do i misunderstand your concern?

[haacked]

That assumes you're using tasks. But you could be doing QueueUserWorkItem or BeginFoo EndFoo and

    <legacyUnhandledExceptionPolicy enabled="1" />

Is one example.

Even without the legacy attribute, a debug assert is useful while testing because an exception brings down the app domain. An assert allows you to intervene.

[haacked]

Ok, so I'm trying this out (finally) and I don't see the Debug.Assert calls. What do I need to do to enable it?

[distantcam]

The Debug.Assert calls are only built into the debug builds. Are you checking debug and not release?

[haacked]

@distantcam Yeah, I set my project to Debug.

[distantcam]

@Haacked Ok, the Debug.Asserts need the DEBUG build constant set.

NullGuard also need to be able to find a reference to System (or System.Diagnostics.Debug for WinRT).

Otherwise if you could email me your project I could take a look.

[distantcam]

Nevermind, I have been able to repo this locally. Looking into it now.

[distantcam]

@Haacked NullGuard.Fody 0.7.2 fixes this issue.

[haacked]

Awesome! Thanks!