[RFC] Annotating methods that raise `NotImplementedError`

Crystal Contrib
1

What do you think about improving compile-time information about methods that are not implemented on some platforms?

NotImplementedError

Some APIs in stdlib are not implemented on all targets. We try to minimize these instances in order to ensure portability. But sometimes there’s just no other way.

Still such platform-specific APIs are usually defined for all targets so they do compile. But when there’s no suitable fallback, the implementation is missing, which is signaled by NotImplementedError at runtime.
The reasoning for not making this a compile time error is to allow a program to compile even if some code path could end up calling a non-implemented method which might never happen in practice.
And this would often be quite cumbersome to work around.

Build time information

Yet with the current mechanism, we’ll only know whether a program can potentially reach a non-implemented method if it actually calls it.
Considering that in most cases, non-implemented methods are very clearly derived from the build target, this is quite poor.
A developer might want to know about potential pitfalls in case they end up calling into a potentially-unimplemented method unintentionally and maybe want to handle this differently.

But we currently only have runtime indicators for this problem.

I think it could be helpful to have a means for statically declaring a method as not-implemented, to have that information at build time.

Annotation

A very similar tool is the @[Deprecated] annotation. In fact, as a crude
solution could add @[Deprecated] to all methods that raise NotImplementedError with an appropriate message and get the desired effect.

Of course a dedicated annotation would be more appropriate and allow specific features for this context.

Portable Documentation

If the annotation is only defined on the target for which the method is not
implemented, only API docs built on that target will show it. Other targets won’t.
To fix this we would need to add the annotation for all targets, even when the method actually is implemented. But maybe the annotation could also define the conditions on which a method is not implemented? Then the compiler could show warnings only on affected targets.

The doc generator could list the conditions.

Example

@[NotImplemented(flags: %w[!win32])]
def WinError.value : self
  {% if flag?(:win32) %}
    WinError.new LibC.GetLastError
  {% else %}
    raise NotImplementedError.new("WinError.value")
  {% end %}
end

Extra

With some additional magic, it could even directly inject the runtime exception, making explicit raise NotImplementedError.new unnecessary and keeping everything succinct.
But that might be too much and certainly it’s above a MVP.

Related issue: Helper macro for `NotImplementedError` · Issue #13078 · crystal-lang/crystal · GitHub

8 Likes
3

The previously suggested @[NotImplemented] annotation requires duplicated conditions, which may get out of sync.

Assuming the method body would largely differ based on the different targets, maybe we could move the target selection into an annotation?

@[Target(flags: %w[win32])]
def WinError.value : self
  WinError.new LibC.GetLastError
end

# This method is not implemented on non-Windows targets.
@[Target(flags: %w[!win32])]
def WinError.value : self
  raise NotImplementedError.new("WinError.value")
end

The compiler would only define the method with matching flags.

If multiple defs with the same signature would match, the compiler should probably error.

This concept is similar to how rust manages conditional compilation with the cfg attribute:

That’s arguably not much different from the original version. Instead of duplicating the condition, we’ve now duplicated the signature.
But the important part is that the target conditions are now encoded in an annotation. That makes it visible to the doc generator, and we can expose information about target-specific implementations in the API docs.

2 Likes
4

I can’t speak for Rust, but Golang’s build tags work similarly, except that build tags operate on the entire file by including/excluding it from the build. The solution you’ve shown here is far more elegant and flexible!

:100:

5

In stdlib, this is mostly related to targets, and we bury these in system specific implementations under the undocumented src/crystal, mostly under system but also event_loop where the compile flag conditions wouldn’t be needed, nor the @[Target] annotation —that is still interesting to have :eyes:

There are ~140 raises of NotImplementedError inside src/crystal yet only ~40 in src otherwise, and most of them are related to unavailable iconv, openssl or zlib features where @[Target] wouldn’t help. For the few other cases, they’re indeed target related… but it might denote that there should be a system abstraction :thinking:

We should look at community shards and see the actual usage of NotImplementedError in the wild.

6

It’s a lot: :person_shrugging: context:global lang:Crys… - Sourcegraph

7

There are also cases like File#close_on_exec(value) which only raise NotImplementedError if value is true on Windows. This of course is impossible to encode statically. And the raise is even nested in the system implementation, not directly in the public method.
We still need a mechanism to document that with a prose explanation. Ideally with a specified format.

At that point however, we can treat NotImplementedError just as any other exception type. Though we’re also missing a structured format to describe what exceptions a method can throw.
For now, just a textual form should work:

# Sets `close_on_exec` behaviour.
#
# Raises `NotImplementedError` for `value: true` on Windows.
def close_on_exec=(value : Bool) : Bool
  self.system_close_on_exec = value
end
8

Many of those uses of NotImplementedError in the community shards are not related to target-specific APIs, for example a read-only IO might raise it inside #write, solely to implement an abstract def. This came from Ruby.

9

Perhaps it would be worth suggesting to users that returning an error as part of a type union is occasionally a better choice than reaching for raise NotImplementedError for these special occasions? This forces the caller to explicitly handle the condition, whether called from the base class or the child class.

User’s don’t need to give up raise forever or even get used to it, but this pattern can be useful where its desirable to force the caller to explicitly handle the exception and you don’t want the error to bubble up to the runtime undetected if missed in testing. All the pieces are right here in Crystal already, this pattern keeps the TODO style error inside of a tight boundary as well as enforcing the actual return type for the method.

# Use your own MethodNotImplemented error, not the system one.
class MethodNotImplemented < Exception
end

# explicit runtime error messages in support of the logic
alias RecoverableError = MethodNotImplemented | MyRuntimeException

class MyAbstractThinger
  # force consumers to deal with the error
  def risky_method() : Tuple{Int32, Bytes} | RecoverableError
    return MethodNotImplemented.new
  end
end

class ConcreteThinger1 < MyAbstractThinger
  # doesn’t implement it.
end

# consumer code

result = risky_method()
# Crystal’s case already does the type narrowing for us!
case result 
when {Int32, Bytes}
# success case - result is the tuple

when MethodNotImplemented
  # handle method not implemented

when MyRuntimeException
  # handle runtime error
end
10

I suppose this could be a viable option. But this kind of enforced error handling at compile time is pretty close to the other alternative: Do not implement the method at all.
This requires similar explicit handling, but is overall a bit simpler.

if thinger.responds_to?(:risky_method)
  thinger.risky_method
else
  # handle method not implemented
end

Passing an error type has some benefits of course, for example it could have some limited support for bubbling up through intermediate methods before eventually being resolved. But not a great deal of difference.

1 Like
11

oh interesting - I didn’t realise Crystal supported responds_to? like Ruby. Slick!
Assuming this is evaluated at compile time, that is indeed a much better way to handle it and doesn’t require polluting the error handling.

And this technique has no effect on error management, either. risky_method’s error shape, whether returned by the method or inside the logic with raise, remains independent of detecting if the method is there. Your example is free of any side-effects; we don’t even need the else branch at all. It does mean that error handling is now nested inside of an if block, but that’s hardly an inconvenience.

Outside of the original RFC case, addressing the use of this in Abstract Classes, it appears that Crystallers have borrowed a Ruby pattern using NotImplementedError when the language already has a compile time feature just for this. In Ruby, handling errors tends to be much faster than metaprogramming so I don’t blame anyone for trucking the luggage around - it’s just habit.