Skip to content
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

Interface implementations require explicit 'inheritdoc' to propagate exceptions #9

Closed
NoTuxNoBux opened this issue Jul 7, 2023 · 4 comments

Comments

@NoTuxNoBux
Copy link

NoTuxNoBux commented Jul 7, 2023

When using interfaces and concrete implementations, announcing them in the interface method doesn't suffice; you also need to use <inheritdoc/> explicitly on the implementation.

To start with a correctly reported error and build from there:

public interface IFoo
{
    public void Bar();
}

public class Foo : IFoo
{
    public void Bar()
    {
        throw new Exception("Oops");
    }
}

The above complains that the Exception is not announced, which is correct.

public interface IFoo
{
    public void Bar();
}

public class Foo : IFoo
{
    /// <exception cref="Exception"></exception>
    public void Bar()
    {
        throw new Exception("Oops");
    }
}

The above complains that the exceptions thrown by the implementation are wider than the ones of the interface, which is also correct.

public interface IFoo
{
    /// <exception cref="Exception"></exception>
    public void Bar();
}

public class Foo : IFoo
{
    public void Bar()
    {
        throw new Exception("Oops");
    }
}

The above goes back to the first error and complains that the implementation throws exceptions it shouldn't, which is incorrect.

public interface IFoo
{
    /// <exception cref="Exception"></exception>
    public void Bar();
}

public class Foo : IFoo
{
    /// <inheritdoc/>
    public void Bar()
    {
        throw new Exception("Oops");
    }
}

The above solves all problems.

Not a big problem in itself, but (I think?) docblocks should automatically be inherited, at least this is the behaviour also exposed by e.g. OmniSharp, as shown by its tooltips:

afbeelding

@carlreinke
Copy link
Member

Visual Studio also shows documentation comments for implementing/overriding methods that don't provide their own. This seems okay when the documentation comment is being shown to a human, but for a code analyzer, I think it's maybe not the right choice. There's an easy and clear mechanism for opting in to inheritance, as you noted, and I think it's preferable to be intentional about inheritance.

@carlreinke carlreinke closed this as not planned Won't fix, can't repro, duplicate, stale Jul 11, 2023
@NoTuxNoBux
Copy link
Author

I respect your decision and am fine with adding inheritdoc as workaround 👍 .

I do think that it's the right choice to implement the same behaviour here, though. Not necessarily just because this is also what other tools do, but mainly because the checked exceptions you indicate are part of the interface as well (as in Java) - in other words there really is no choice for the developer but to have the concrete implementation method diverge from the interface method regarding its thrown exceptions. What I mean is: doesn't this pretty much force you to always add inheritdoc in all of these cases since this is always what you want? (And if that is the case, should it not be the default 😅 ?)

@carlreinke
Copy link
Member

The problem is that it's not necessarily a violation of the contract to diverge from the interface. In many cases, it's completely valid to throw only a subset of the exceptions that the interface specifies. And the empty set, which is what you get when the documentation comment is omitted, is an important subset.

@NoTuxNoBux
Copy link
Author

NoTuxNoBux commented Jul 12, 2023

Ah, I see, it's that you want to be able to indicate that your concrete implementation only throws a subset of exceptions, which may also be important to e.g. callers directly talking to it rather than going through the interface.

I can see an argument for doing that, and don't feel strongly either way - in my head I saw it as the exceptions ideally being part of the API in the programming language syntax itself; in which case having a subset is of course possible in the implementation itself, but the signature of the method cannot diverge from the interface implementation at all (so every caller, even not going through the interface, should take into account that this method may throw all, not just a subset, of all interfaces from the interface method). Then again, you can also narrow down the return type whilst not violating the interface, so this indeed makes sense in a similar way.

Thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants