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

[FR] NotImplementedError, to communicate user-extendable but not-yet-implemented API #50196

Open
Seelengrab opened this issue Jun 16, 2023 · 7 comments
Labels
domain:error messages Better, more actionable error messages kind:feature Indicates new feature / enhancement requests

Comments

@Seelengrab
Copy link
Contributor

Seelengrab commented Jun 16, 2023

Currently, defining some form of API like so:

abstract type Foo end

"""
    bar(::Foo)

Implements `bar` for subtypes of `Foo`. Needs to be implemented to fulfill the interface expected of `Foo`.
"""
function bar end

always throws a MethodError. From a user-perspective, it's hard to find out whether bar was supposed to be implemented by a package they're using or not - it could very well just be a bug, since all MethodError communicates is "There is no method matching this". Since this can bubble up from deep in some library, it'd be better for users to be able to say "I got this NotImplementedError", which also makes it clear to package maintainers that they forgot to implement some method that is required of the interface they claim to implement.

This feature request/proposal is aimed at solving this issue, by giving package authors & Base an option to communicate to users "There is no fallback definition, since your type should implement this". This is done with a new error type, NotImplementedError, which is defined like so:

struct NotImplementedError <: Exception
    interface::String
    func::String
end

function Base.showerror(io::IO, nie::NotImplementedError)
    printstyled(io, "NotImplementedError: "; color=:red)
    print(io, "The called method is part of a fallback definition for the `", nie.interface, "` Interface.\n",
              "Please implement `", nie.func, "` for your type T.")
end

and used like so:

julia> abstract type Foo end

julia> bar(::Foo, ::Int) = throw(NotImplementedError("Foo", "bar(::T, ::Int)"))

julia> struct Baz <: Foo end

julia> bar(Baz(), 1)
ERROR: NotImplementedError: The called method is part of a fallback definition for the `Foo` Interface.
Please implement `bar(::T, ::Int)` for your type T.
Stacktrace:
 [1] bar(::Baz, ::Int64)
   @ Main ./REPL[4]:1
 [2] top-level scope
   @ REPL[6]:1

image

This allows the distinction between intended-to-be-extended API (bar(::Foo, ::Int)) and this-is-supposed-to-error (any other signature on bar, which throws a MethodError).

To be discussed

The details of what exactly NotImplementedError should contain, since this version with just two strings is IMO too bare-bones, as it requires discipline/good grasp of the intended API to be able to create the string directly.

Why is this needed?

This error and variations on it are widespread throughout the ecosystem and I think Base could benefit from having some fallback definitions like the one above as well, to give much more informative error messages when subtyping e.g. <: AbstractArray or other abstract types in Base.

@Seelengrab
Copy link
Contributor Author

I have implemented this concept as part of a package now, you can find it here: RequiredInterfaces.jl. It works pretty much exactly as described in this issue, while also allowing implementors of an interface to check that they've at least implemented the correct methods of an interface. Please do check out the documentation if this is of interest to you.

There are some aspects that (sadly) don't work, mostly due to limitations in Julia, which is expanded on in the docs.

@brenhinkeller brenhinkeller added domain:error messages Better, more actionable error messages kind:feature Indicates new feature / enhancement requests labels Aug 4, 2023
@BioTurboNick
Copy link
Contributor

BioTurboNick commented Oct 12, 2023

Was just exploring the many uses of MethodErrors in Base/Stdlibs (from #51673) and I think your proposal here could be a better solution for them.

In Julia, another common pattern is to have a fallback method with generic types that may attempt to convert to a more specific type. But if the more specific method hasn't been defined for whatever reason, you can end up with a stack overflow - so a MethodError is thrown instead. Or the fallback method exists to resolve ambiguity, and isn't really intended to be defined at all. The show/display stack makes heavy use of MethodErrors which should probably be NotImplementedErrors

A NotImplementedError would be a good way to communicate that a more specific method is expected by the API design, but the design is broken because of the absence of the method. And/or to communicate if the method is intentionally left unimplemented.

I think it might be helpful for the constructor to accept the same arguments as a MethodError so it can be switched easily. I don't know if the interface part is generic enough? Might be better to leave that as an addition for if/when interfaces are mature enough to be added to Julia. An optional message argument for more detailed explanation when necessary would be useful.

Perhaps:

NotImplementedError(msg::AbstractString)
ERROR: NotImplementedError: <msg here>

NotImplementedError(f, args)
ERROR: NotImplementedError: no implementation has been provided matching f(::Type1, ::Type2).

NotImplementedError(f, args, msg::AbstractString)
ERROR: NotImplementedError: no implementation has been provided matching f(::Type1, ::Type2). <msg here>

A usability catch here is possibly ensuring people don't overwrite the fallback method while attempting to fix the error.

EDIT: Perhaps if we want to indicate an abstract type as an interface, could also do:

NotImplementedError(f, args, type::Type)
ERROR: NotImplementedError: no implementation has been provided matching f(::Type1, ::Type2), expected as part of the interface for <type here>.

NotImplementedError(f, args, type::Type, msg::AbstractString)
ERROR: NotImplementedError: no implementation has been provided matching f(::Type1, ::Type2), expected as part of the interface for <type here>. <msg here>

@mikmoore
Copy link
Contributor

I don't necessarily see NotImplementedError as intrinsically different (or less arcane) than a MethodError. The distinction appears to be subtle. I think a challenge would be to decide whether/where to apply a NotImplementedError rather than leave the default MethodError. Should Base.:+ have NotImplementedError fallbacks or should it continue to throw the current MethodError? Should it be NotImplemetedError for <:Number arguments and MethodError in general?

If packages want to make their own NotImplementedError variants, they have their specific reasons and we couldn't stop them if we wanted to. But I don't see this as fundamentally necessary within Base. Would we use it widely within Base/stdlibs?

I'm afraid the rest of my comment might be going a little off-base from your intent and proposal, but I already wrote it so here it is:

Some users already "abuse" MethodError by manually defining them in cases they maybe shouldn't. Really, I take issue with most cases where a method is defined for an otherwise-useful function (i.e., one that has other methods that return without throwing) with the exclusive purpose of throwing, as this is already the purview of MethodError. I see NotImplementedError as a more tempting version of a manual MethodError, but with the same footgun capacity to inadvertently block usable methods with clumsy over-typed signatures. An experienced user is quite good at understanding and resolving a MethodError, so long as they aren't needing to simultaneously untangle someone's misguided attempt to be helpful, and I wouldn't want to risk losing the useful information it can provide.

If we can arrive at a clear and agreeable convention as to when a NotImplementedError is distinct and appropriate, fine. Absent that, or in addition, I might prefer a solution that attempts to improve MethodError to be more helpful. If we can't improve the generic MethodError text, then perhaps there is a solution where we make a new function methoderrorhint(::typeof(f)) = hint::AbstractString, which augments a thrown MethodError(f,args) by including the hint as part of the error message. I vaguely recall some other error-hinting functionality already existing (or at least being discussed) somewhere?

Again, maybe this last part is talking past this proposal and isn't relevant.

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Oct 13, 2023

Should it be NotImplemetedError for <:Number arguments and MethodError in general?

Yes, that is the idea, as it is also implemented in RequiredInterfaces.jl. That's also exactly the semantic difference - MethodError is about "there is no expectation for this to work" and NotImplementedError is "this is supposed to work, but someone somewhere forgot something". The package doesn't provide such fallback definitions for things from Base because of piracy (though I guess I could register the appropriate interfaces, as has been requested before Seelengrab/RequiredInterfaces.jl#9).

If we can't improve the generic MethodError text, then perhaps there is a solution where we make a new function methoderrorhint(::typeof(f)) = hint::AbstractString, which augments a thrown MethodError(f,args) by including the hint as part of the error message. I vaguely recall some other error-hinting functionality already existing (or at least being discussed) somewhere?

The functionality you're thinking of is Experimental.register_errorhint, but that approach has several downsides: It works purely on the exception type, requiring either a type parameter & spurious specialization of error paths to dispatch on for various users of that error type; or some form of additional registration mechanism/hook that would be called on some condition. That's very complicated, both for package users as well as implementors. In contrast, throwing a bespoke NotImplementedError has a clear meaning: for the last method in the stacktrace, someone hasn't implemented the interface correctly.

I agree that the misuse of throw(MethodError(..)) is bad, and is in part what drove me to propose this. However, I disagree that an experienced user is good at resolving these - NOT throwing MethodError when the package author has all information required to know that the user of their package did something wrong is obviously a better choice, and makes it easier for both experienced and especially newcomers to know what they have to do to resolve the error. Adding that kind of communication to every kind of MethodError that could possibly be thrown for a function call clearly doesn't scale.

@BioTurboNick
Copy link
Contributor

For some context, C#/.NET has NotImplementedException, and Python has NotImplementedError. Their primary purpose is to provide noisy placeholders when building out an implementation of an interface/abstract class.

The proposal here is fairly similar, except that because of how Julia works, the interface/type definer can throw the exception as a fallback method rather than the interface implementer having to manually throw it as boilerplate.

@halleysfifthinc
Copy link
Contributor

This is more of a comment on the inverse of the NotImplementedError kind of errors, but I think is relevant to the scope of how widely relevant a NotImplementedError would be.

Relating to dispatch/methods/composition/interfaces, there seem to be 2 main kinds of errors :

A. This method doesn't exist and shouldn't/won't/can't exist because XYZ
B. This method doesn't exist yet but should exist (to comply with interface XYZ, etc)

And MethodErrors are thrown for both cases, which is where the confusion/ambiguity lies. IMO, errors in case B, which could be solved with a NotImplementedError, are only easily dealt with for small and/or well-defined interfaces. But for interfaces like <:Number or arrays, you have the challenges raised by @mikmoore about when/where NotImplementedError methods should be defined, and a big problem there is that it making it compose well is hard (eg overly broad fallback methods preventing composition that could've worked).

The appeal of "abusing" MethodErrors is that sometimes there are methods which are known by the author to not work (case A). A different approach would be some kind of a WontBeImplementedError to disambiguate the errors in this case. The benefits there are that methods which won't/can't ever work are likely to be more clearly defined (theoretically, eg will have more tightly scoped methods than the converse NotImplementedYetError which would need to be broad/generic fallbacks). If a more specific error of this type was generally adopted, then MethodErrors could slowly become more and more likely to be a case B and better fill this NotImplementedError case.

Ultimately I think these are subtly orthogonal issues, and WontBeImplemented and NotYetImplemented errors aren't mutually exclusive, but the existence of a WontBeImplementedError could affect how broadly a NotImplementedError would be relevant.

@Seelengrab
Copy link
Contributor Author

But for interfaces like <:Number or arrays, you have the challenges raised by @mikmoore about when/where NotImplementedError methods should be defined

The issue with Number is less that it's unclear where NotImplementedError should be placed, but rather that it's unclear what the interface even is - that's orthogonal to NotImplementedError being useful for those interfaces we do know exactly already (and, just maybe, would provide a gentle push to better define these fuzzy interfaces as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:error messages Better, more actionable error messages kind:feature Indicates new feature / enhancement requests
Projects
None yet
Development

No branches or pull requests

5 participants