Proposal: Automatic documentation for potential exceptions

[alrz]

Currently you can carelessly throw exceptions without any documentation for the client unless you manually write <exception> doc tags above the method. I suggest to automate this so that all potential exceptions would be added to method's doc automatically and Quick Info can show them.

/// <exception cref="ExceptionA"></exception>
void F() => throw new ExceptionA();
/// <exception cref="ExceptionB"></exception>
void G() => throw new ExceptionB();
/// <exception cref="ExceptionA"></exception>
/// <exception cref="ExceptionB"></exception>
void H() { F(); G(); }

->

void F() => throw new ExceptionA();
void G() => throw new ExceptionB();
void H() { F(); G(); }

If the method is outside of the assembly boundary, we can infer exception tags from documentation of the used methods.

It would be also helpful if a code fix add those tags to include the descriptions of the reasons why these exceptions could be thrown. I think this can be an alternative to checked exception (a la Java) to encourage people to document exceptions without forcing them to write try catch.

[svick]

How do you figure out which exceptions can be thrown in the presence of arbitrarily complex catch clauses and rethrow statements?

Consider e.g.:

void H()
{
    try
    {
        F();
        G();
    }
    catch (Exception ex)
    {
        if (AnotherMethod(ex))
        {
            throw;
        }
    }
}

Here, the compiler wouldn't be able to figure out which exception can actually be thrown by the method and it will either report exceptions that won't be thrown by the method or not report exceptions that can be thrown (or both).

I'm not sure how useful would an imperfect implementation like that be.

[alrz]

@svick This will work on static type of exceptions, so your method will likely throw an exception of type Exception plus whatever AnotherMethod throws, if any. I think that's enough for developer to know that's a potential failure. If you want more specific exception types, you should wrap, not rethrow.

[jnm2]

Statically, if you know what exception types F and G and AnotherMethod can throw, even catch (Exception) { throw; } will only ever throw one of those known types.

[alrz]

@jnm2 Sorry, human mistake. :D That's why I like this to be generated by the compiler.

[jnm2]

I like this proposal. It would be great to have a warning for missing exception documentation and a quick fix to insert all individually known exceptions, just like how <params> works today.
I'm using this which might interest you: https://github.com/CSharpAnalyzers/ExceptionalReSharper
But if it's implemented officially and without the ReSharper dependency, all the better.

[DavidArno]

I'm confused how you would automate this and where the code fix comes into it. Your code example implies one could remove these comments and have them auto-generated somehow so that they appeared in Quick Info. But a code fix implies they'd be added to the source if missing.

Could you clarify, please?

[alrz]

@DavidArno The explicit exception documentation (which can be added by the code fix) is only required if you want to write descriptions of the reasons why these exceptions could be thrown (inside <exception> tag). Otherwise, it should report potential exception types in Quick Info without any xml doc tag (meaning that the exception tags will be emitted by the compiler under the hood just as if they were written with an empty body).

[jnm2]

As a consumer I'd like this automated feature because I'd like to know what the API might do, but the problem could be that I'd rely too heavily on it having been generated. As a consumer, I think it would be better if the static analysis was done by the IDE and presented in addition to the doc comments so that you don't have to rely on the author's generation process (and version of compiler).

As an author, I want to write meaningful explanations of why each exception might occur. Adding empty nodes under the covers doesn't help me achieve that goal, but an analyzer warning would be fantastic with a code fix that inserts the node in my source code and positions my cursor to tab through and type descriptions.

[alrz]

@jnm2 Most of the time just exception types suffice as one can have an idea of what exceptions could be thrown — in fact, to this day, it's not supported to show exception reasons in Quick Info (#1515). Besides, in my opinion, writing documentation is a far more involved process and developers are often too lazy to do that! So, having the compiler figure out the exception types would be a big step to start. This doesn't necessarily discourage people to write meaningful explanations. I'd agree that an optional warning for missing exception docs can be helpful if you take that as a convention for your codebase.

[HaloFour]

This sounds like something that could be accomplished via an analyzer, albeit a potentially complicated one. I've not done much work with analyzers. Can they access the XML documentation of either referenced libraries or annotated methods within the current project?

[alrz]

@HaloFour Sure, but at best they can suggest to add xml doc comments. As a language feature, it would be totally transparent to the programmer. Otherwise you need to manually run the fix to update xml docs every time code changes.

[HaloFour]

@alrz

You'd need to enable XML docs for any of this to work as described anyway. Seems like a poor-man's implementation of checked exceptions. If we're going to wade in that direction I'd rather see it "done right", although if I recall the team expressed no desire to implement checked exceptions.

[alrz]

Since this is just an informative analysis and there would be no guarantee in the same sense of checked exceptions, I'd agree that it should not be built into the compiler. I don't know if Roslyn provides any mechanism to do this transparently along the compilation process.

[MgSam]

I think a problem with this is that any such list of exceptions that Roslyn might generate would either be noisy or preclude important cases. NREs, StackOverflowException, OverflowException can potentially happen anywhere.

Another problem is exceptions that are re-used for different things for different purposes- such as InvalidOperationException. The IDE can tell you that this might be thrown but in absence of documentation you have no way of knowing if this is from the .Single() extension method and never will fail or is from a source that you need to catch and potentially handle.

[HaloFour]

@MgSam

I don't think that's necessarily a problem. Java checked exceptions work in the same manner. You're expected to document those exceptions that would be thrown due to a breakdown in the method logic, not those that might result due to programmer error or virtual machine failure.

[alrz]

@MgSam

NREs, StackOverflowException, OverflowException can potentially happen anywhere.

I'm talking about explicitly thrown exceptions, not the ones that "potentially happen anywhere." Since .NET framework does provide this information, it would flow out of your code and will be useful for others.

list of exceptions that Roslyn might generate would either be noisy or preclude important cases

That's why I think that a code fix would not be the right mechanism to implement this. It would be only useful when you want to write full explanation of why each exception might occur. But here I just need to know what exceptions might occur. Knowing something is simply better that not knowing at all. Not all codebases provide such information. But if it was automated, one as the consumer, at least, have a clue. And I don't think all people would care enough (or able to) automate this internally.

you have no way of knowing if this is from the .Single() extension method and never will fail

If they used Single it will fail. If it's not intended to fail, they should have used FirstOrDefault. That would be of course a bug, unless it is supposed to bubble up the stack. In that case, it should've been documented. In any case, the auto generated exception tag, doesn't make it harder, if not easier, to find the potential bug! At least, it will help you to investigate the cause and update the documentation, if any.

[orthoxerox]

Have you read Joe Duffy's blog post? He argues a single [Throws] provides enough information for the consumers of your method.

[iam3yal]

@alrz I like the idea of making exceptions more discoverable but I don't like the xml comments part at all.

[alrz]

@eyalsk That's why I'm looking for a transparent solution that doesn't require developers to opt-in.

[alrz]

@orthoxerox I suppose throws in Swift follows the same sentiment. But that would be a breaking change in C# because currently there is nothing to show that if the method throws or not.

[jnm2]

@alrz

But here I just need to know what exceptions might occur. Knowing something is simply better that not knowing at all. Not all codebases provide such information. But if it was automated, one as the consumer, at least, have a clue.

I think that's a good goal but I also think XML doc comments is the wrong place to do it. Possible exceptions should be analyzed by Visual Studio along with IntelliSense and other metadata at the point where you are consuming the API. That is the only way you can get the guarantee you're looking for.

[alrz]

@jnm2 As I said in the comment above, if we take this out of the compiler, it will no longer depend on the compiler version. xml docs would just be the underlying mechanism to persist this information.

[jnm2]

@alrz The reason why I think xml docs are not a good underlying mechanism to persist this information is that there is no guarantee that the exception information is reliable. Maybe I didn't use the latest version of Visual Studio and the exception information is misleading if you were expecting it to be generated in general.
It would be much higher fidelity to calculate all this and augment the XML docs on the consumer side automatically in the IDE along with IntelliSense.

[gulshan]

@orthoxerox @alrz What if warnings are used instead of error following non-nullable objects proposal? Then we shall get a lenient/optional checked exception based error model in C#.

[alrz]

@Pilchie @CyrusNajmabadi Do you want this as a refactoring or analyzer? I guess this should be an analyzer to suggest fix-all, but it feels like it needs a heavy analysis, performance-wise. What are your thoughts?

[CyrusNajmabadi]

Seems like something an analyzer could do. But i'm not sure we would take this even if it were written. The problem seems like the scenario would be 'incomplete'. i.e. we could help you add these tags, but not help you maintain them.

It's also going to be problematic if we don't add 'exception' tags for things that do get thrown, with people getting upset and saying "hey your fixer didn't add all the tags it shoudl have. it's broken!".

[yoavfr]

Currently you can carelessly throw exceptions without any documentation for the client unless you manually write <exception> doc tags above the method. I suggest to automate this so that all potential exceptions would be added to method's doc automatically and Quick Info can show them.

/// <exception cref="ExceptionA"></exception>
void F() => throw new ExceptionA();
/// <exception cref="ExceptionB"></exception>
void G() => throw new ExceptionB();
/// <exception cref="ExceptionA"></exception>
/// <exception cref="ExceptionB"></exception>
void H() { F(); G(); }

->

void F() => throw new ExceptionA();
void G() => throw new ExceptionB();
void H() { F(); G(); }

If the method is outside of the assembly boundary, we can infer exception tags from documentation of the used methods.

It would be also helpful if a code fix add those tags to include the descriptions of the reasons why these exceptions could be thrown. I think this can be an alternative to checked exception (a la Java) to encourage people to document exceptions without forcing them to write try catch.

Here you go:
https://marketplace.visualstudio.com/items?itemName=YoavFrandzel.CheckedExceptions
I'm sure there are cases that are not completely covered, but I think it does what you want in 90% of the cases.

[xmarshal]

@yoavfr

I'm sure there are cases that are not completely covered, but I think it does what you want in 90% of the cases.

this highlights exceptions even inside the try catch block

[robloo]

Visual Studio 2022 with C# now auto-completes the summary comments with any exceptions. Adding this analyzer would go great with that feature to cleanup old code.

[MoazAlkharfan]

Visual Studio 2022 with C# now auto-completes the summary comments with any exceptions. Adding this analyzer would go great with that feature to cleanup old code.

This doesn't work if used with for example ArgumentNullException.ThrowIfNull(obj)
It overall doesn't seem to work if there is no throw keyword in the method

[alrz]

Not sure if docs can be auto generated using a generator? But it could definitely special case ThrowIfNull and such.