Discourage usage of NotImplementedError

changed the description

@Quintasan Good call and thanks for raising this issue

I wanted to add some resources regarding this issue:

Just to add another resource I just found: https://bugs.ruby-lang.org/issues/18915 where alternatives were suggested:

NotImplementedYetError
ToBeDefinedError
AbstractMethodError

Thanks @Quintasan for creating this issue

@Quintasan could you expand on the rationale here? While I agree that in concept we are using an error class for a role that it was not originally intended (as in by original design), it feels to me that the NotImplementedError is that type of error that is not intended to be "rescued" from.

My understanding is that it was always used as a reminder for developers that "you should implement this and we will break it earlier so you know it's important".

Does that make sense?

Do you have a usecase where you had to rescue it from that is not a case similar to above?

it feels to me that the NotImplementedError is that type of error that is not intended to be "rescued" from.

Technically speaking a gem author might have to rescue from it, if for example (I'm making this up) fsync is not implemented on Rubinus on FreeBSD or something along this line. That said, I agree that this usually shouldn't happen. A brief scan shows that we have rescue_from :all in lib/api/api.rb:123 so technically we're trying to rescue all exceptions there but that's just once place out of... I don't know how many.

My understanding is that it was always used as a reminder for developers that "you should implement this and we will break it earlier so you know it's important".

I think the only "upside" of using NotImplementedError is the following scenario:

Someone creates a base class which has a a method raising NotImplementedError
Another developer subclasses it but doesn't reimplement that particular method
Somehow it makes it past:
- test suite
- review
- maintainer review
- doesn't explode when deploying
- in runtime we get NotImplementedError because no rescue block will help us here

I believe we can safely roll with NoMethodError instead of NotImplementedError here.

Thanks for raising this @Quintasan !

I agree we are possibly mis-using NotImplementedError. I noted in the internal Slack thread (https://gitlab.slack.com/archives/C02PF508L/p1646243912922679) that even https://github.com/rails/rails/search?q=NotImplementedError uses NotImplementedError in about 123 instances.

So I am okay with moving from NotImplementedError but am leaning against using NoMethodError. For me, I will still prefer some descendant of ScriptError which will not be rescued.

Also partly because we already have a lot of https://sentry.gitlab.net/gitlab/gitlabcom/?query=is%3Aunresolved+NoMethodError in Sentry that is piling up.

I am against using NoMethodError as well. It'll be very confusing to me, because it usually means we got a different object in runtime, not that we forgot to implement an abstract method. They're both bugs but in a much different way, not to mention that we can easily mis-rescue NoMethodError, which has already been overloaded.

If we really want to stop using NotImplementedError, I propose the following options:

raise ScriptError, 'this is an abstract method'
raise AbstractMethodError and define AbstractMethodError = Class.new(ScriptError)

This exception should never be rescued.

raise AbstractMethodError and define AbstractMethodError = Class.new(ScriptError)

I like this

@godfat-gitlab I think we should just extend Exception because ScriptError is for when a script can not be executed because of a LoadError, NotImplementedError or a SyntaxError and I don't think any of the cases apply here. Subclasses of Exception don't get caught by bare rescue as well IIRC. Overall I like your proposal because it would be a matter of find-and-replace in the code and writing a cop that discourages use of NotImplementedError.

@Quintasan Sure, I don't feel strongly using ScriptError or Exception. A bare rescue will only rescue StandardError so we should be good with either of them.

I'm about 90% sure there is an issue relating to this exact discussion before and I'm so sad that I can't find it but I believe the conclusion was:

"It's common practice in GitLab and gems and ruby/rails in general and it works fine so we'll keep doing it".

I also don't think I'd like to see us raise NoMethodError anywhere and just allow Rails to take care of that. I'd be fine if we see NoMethodError because the method is actually not defined but to explicitly define a method and raise NoMethodError seems like that would confuse me.

In the case of "abstract classes" I also feel like a comment would be just as good as an empty method with raise NotImplementedError because both cases will blow up when it matters.

Search helped me find 2 other times this conversation has come up in comments https://gitlab.com/search?group_id=9970&repository_ref=&scope=notes&search=NotImplementedError+fork+fsync&snippets=false . But perhaps we never created and issue before. I'm sure we had the conversation in a Slack thread before.

I think the main points in favour of NotImplementedError is:

It's a well named exception
It's in popular use in Ruby/Rails in this way
It's expensive to change because we've got it in heaps of places
It doesn't really conflict with anything because in practice we never see the NotImplementedError exception that Ruby uses as it's a low level thing that doesn't happen on our computers

So I think the reasoning for keeping is that there is no good reason not to use it and the Ruby/Rails community has turned this into a convention even if it started by accident.

It doesn't really conflict with anything because in practice we never see the NotImplementedError exception that Ruby uses as it's a low level thing that doesn't happen on our computers

I don't remember if I've ever seen this error on production but it's actually possible to receive this type of error from the VM level(e.g. rb_file_s_birthtime).

I see that the documentation about the NotImplementedError is quite clear and it's not inherited from StandardError therefore requires special effort to rescue but even in Ruby's source, there are some cases where they use it to mark the template methods which contradicts with the docs(e.g. Delegator#__getobj__, Integer#<=>) So yes, it seems like Ruby is also using it in a way we use.

I see that the documentation about the NotImplementedError is quite clear and it's not inherited from StandardError therefore requires special effort to rescue

I'm struggling to think of a good reason why'd you'd ever want to rescue this error at runtime but evidently we have a few examples:

$ git grep 'rescue NotImplementedError'
app/models/concerns/integrations/has_data_fields.rb:      rescue NotImplementedError
app/serializers/detailed_status_entity.rb:  rescue NotImplementedError
ee/app/controllers/admin/geo/replicables_controller.rb:  rescue NotImplementedError
ee/app/models/geo_node_status.rb:      rescue NotImplementedError => e
ee/app/services/geo/file_registry_removal_service.rb:      rescue NotImplementedError
qa/qa/resource/base.rb:        rescue NotImplementedError
qa/qa/resource/sandbox.rb:        rescue NotImplementedError
spec/support/shared_examples/models/integrations/chat_message_shared_examples.rb:      rescue NotImplementedError
spec/support/shared_examples/models/integrations/chat_message_shared_examples.rb:      rescue NotImplementedError

When I think about this templating approach I think of it as an attempt to recreate Java abstract classes which fail at compile time and in general feel there isn't really a valid reason to ship code that could actually ever raise NotImplementedError.

When I think about this templating approach I think of it as an attempt to recreate Java abstract classes which fail at compile time and in general feel there isn't really a valid reason to ship code that could actually ever raise NotImplementedError.

Yes, but in Ruby, there is no way to spot NotImplementedErrors before actually running the logic. This is why I was suggesting the macro as after loading all the ruby files we can check if all the template methods are implemented as we will know all of them(the ones created by the macro).

It's a well named exception

It's in popular use in Ruby/Rails in this way

I feel like these aren't great arguments - it just happens to be share a name with a condition we're attempting to raise for.. and something being popular doesn't make it correct.

It's expensive to change because we've got it in heaps of places

What is stopping it from being a fairly simply find-and-replace process, once a new, more correct exception is implemented?

@kerrizor sure you're right. I agree they're not very good arguments.

@Quintasan, please can you add a type label to this issue to help with issue discovery in issue reports.

added auto updated label

@Quintasan, please add labels to your issue. Labels help us find and categorize issues.

Thanks for your help!

(improve this comment?)

What do you think about defining a macro to define template methods?

module TemplateMethodUtility
  class InterfaceError < ScriptError
    def initialize(subject, method_name, template_location)
      call_location = caller_locations[2].to_s
      message = <<~MSG
        `#{subject}##{method_name}` is a template method which is not implemented!

        See #{template_location}.
        Call location: #{call_location}
      MSG

      super(message)
    end
  end

  def template_method(method_name)
    undef_method method_name
    template_location = caller_locations.first

    define_method(method_name) do
      raise InterfaceError.new(self.class, method_name, template_location)
    end
  end
end

Module.prepend(TemplateMethodUtility)

class Foo
  template_method def zoo; end
  # or we can omit `def` and just use `template_method :zoo`
end

class Bar < Foo; end

Bar.new.zoo
# Traceback (most recent call last):
# 	1: from template_method.rb:34:in `<main>'
# template_method.rb:21:in `block in template_method': `Bar#zoo` is a template method which is not implemented! (TemplateMethodUtility::InterfaceError)
# 
# See template_method.rb:29:in `<class:Foo>'.
# Call location: template_method.rb:34:in `<main>'

This way, we can also check if all the template methods are implemented after eager loading the application as we will have a list of template methods.

I'm not sure what problem this solves. I feel like this is like trying to extend the programming language for a minor syntactic gain. Doing this may reduce some boilerplate code but it adds a cognitive overhead to understanding code and in aggregate I'd probably prefer to keep to the boilerplate code for now.

I'm not sure what problem this solves. I feel like this is like trying to extend the programming language for a minor syntactic gain.

Cleaner error message explaining which class does not implement the method and where the template method is defined.

# template_method.rb:32:in `zoo': NotImplementedError (NotImplementedError)

#### vs ####

# template_method.rb:21:in `block in template_method': `Bar#zoo` is a template method which is not implemented! (TemplateMethodUtility::InterfaceError)

# See template_method.rb:29:in `<class:Foo>'.
# Call location: template_method.rb:34:in `<main>'

Clean and unique way to define template method which makes it easier to spot.
Being able to check all the template methods after eager loading the application logic(Yes, this summons the Javas Interfaces).

But I agree that it just introduces yet another macro to keep in mind.

Cleaner error message explaining which class does not implement the method and where the template method is defined

Did somebody get confused about what NotImplementedError meant?

Being able to check all the template methods after eager loading the application logic

Did we accidentally ship some code where we forgot to define a method?

I'm still skeptical here and this feels like we're trying to build upon the programming language to be more like another language. It feels clever but likely to lead to less productivity in my opinion but I must admit I'm very biased against metaprogramming cleverness in general and I'm often wrong.

Cleaner error message explaining which class does not implement the method and where the template method is defined

Did somebody get confused about what NotImplementedError meant?

Sometimes it's a bit hard to locate which class from the ancestor list defines the template method. This is why I usually avoid using the template method pattern in Ruby.

Being able to check all the template methods after eager loading the application logic

Did we accidentally ship some code where we forgot to define a method?

Good question. I haven't witnessed any case at GitLab yet but by just checking Sentry, there are some issues: https://sentry.gitlab.net/gitlab/gitlabcom/?query=NotImplementedError.

I am not strongly suggesting that we need a macro, I was just trying to highlight yet another option to see if someone sees any benefit of using it. But taking one step back and discussing "what problem we are trying to solve" is a better approach.

Could we simply raise 'implementation is missing' in the template method?

These should be considered build-time errors, and not be rescued from, so the simplest solution is to just raise a string IMO.

I don't think that template methods are "Java-esque" or specific to statically typed languages. It is a very useful pattern in OOP and gives code structure and communicates intention without the need to use code comments.

The argument that we need to run the application first to detect the error is not a great one, since that is true for anything you do in a dynamically typed language, and it is why we seek to have such high automated test coverage.

I think I meant to post this here: #354314 (comment 862350989)

These should be considered build-time errors, and not be rescued from, so the simplest solution is to just raise a string IMO.

@mkaeppler - But raising a string actually raises a RuntimeError which inherits from StandardError and it is the default rescued error class.

What do you mean by; These should be considered build-time errors, and not be rescued from?

"Let it crash" on runtime
fail after bytecode compilation?

That's a good point.

Re build-time errors: I mean on (pre-)CI, for instance, by running an automated test.

Re build-time errors: I mean on (pre-)CI, for instance, by running an automated test.

I agree with this but sometimes it's possible to miss these errors with unit tests and coverage reports. That's why I was suggesting that we implement a macro and check all the template methods on CI after the application boots.

Seems like there are plenty of errors on Sentry related to missing template method implementations(not sure about their severities though).

mentioned in merge request !80578 (merged)

mentioned in merge request !81982 (merged)

mentioned in merge request !82673 (merged)

mentioned in merge request !90122 (merged)

mentioned in merge request !91076 (closed)

@gitlab-org/maintainers/rails-backend sorry for the second ping but I created an MR for this – Draft: Discourage use of NotImplementedError (!91076 - closed)

By re-reading the discussion I think we didn't reach a positive conclusion. The NotImplementedError is widely used by gems. Do we have a new/different reason?

Please consider #354314 (comment 862354649).

@brodock

The only argument that holds IMO is:

4. It doesn't really conflict with anything because in practice we never see the NotImplementedError exception that Ruby uses as it's a low level thing that doesn't happen on our computers

But I still think that the fact, that it doesn't conflict with anything because we don't see it in practice automatically makes it correct.

added maintenanceworkflow typemaintenance labels

changed the description

mentioned in merge request !97240 (merged)

mentioned in merge request !107584 (merged)

mentioned in merge request !120481 (merged)

mentioned in merge request !127639 (merged)

Here's another possibility from the school of 'the best code is no code'. We don't implement anything in the superclass.

In the simplest of cases, here's the error message we get from the current pattern we're using.

class Animal
  def make_noise
    puts noise
  end

  def noise
    raise NotImplementedError
  end
end

class Duck < Animal
  def noise
    "quack"
  end
end

class Goose < Animal
end

Duck.new.make_noise
Goose.new.make_noise

$ ruby animal.rb 
quack
animal.rb:7:in `noise': NotImplementedError (NotImplementedError)
	from animal.rb:3:in `make_noise'
	from animal.rb:21:in `<main>'

Note the lack of information in this error.

Now with the superclass method removed.

class Animal
  def make_noise
    puts noise
  end
end

class Duck < Animal
  def noise
    "quack"
  end
end

class Goose < Animal
end

Duck.new.make_noise
Goose.new.make_noise

$ ruby animal.rb 
quack
animal.rb:3:in `make_noise': undefined local variable or method `noise' for #<Goose:0x00007f54863deaf0> (NameError)

    puts noise
         ^^^^^
	from animal.rb:17:in `<main>'

IMO the latter has the following advantages:

The error is intrinsically more informative. The exception text itself has useful information, it even tells us which class we're dealing with.
We remove a method that by design is intended to be unreachable. At the time of writing we have > 500 of these methods in our code base.

If we call the missing method directly we get an equally informative NoMethodError.

Goose.new.noise
animal.rb:17:in `<main>': undefined method `noise' for #<Goose:0x00007f92ebbe18d0> (NoMethodError)

Goose.new.noise
         ^^^^^^

This is not a bad idea. However, I personally find that having a method in the abstract/base class that throws NotImplementedError gives a certain level of documentation in the class. Looking at the class, it's easy to see what I have to implement when it's subclassed.

I think I come down in favor of leaving it as is for the reasons in #354314 (comment 862354649).

Given this approach came from the Liskov substitution principle from the SOLID principles. alternative is just to use a string to raise the exception

class Animal
  def make_noise
    puts noise
  end

  def noise
    raise "Not implemented"
  end
end

class Duck < Animal
  def noise
    "quack"
  end
end

class Goose < Animal
end

Duck.new.make_noise
Goose.new.make_noise

While the usability of this error message is questionable as @mallocke pointed out, it might help defining the parent class better.

mentioned in merge request !137829 (merged)

mentioned in merge request !149556

mentioned in merge request gitlab-development-kit!4360 (merged)

FYI this article also gives good reasoning why not to use it: https://nithinbekal.com/posts/abstract-methods-notimplementederror-ruby/

I am not convinced because:

Provide a default implementation

Then it's no longer an abstract method

Explicitly raise with a message

Then it can be accidentally rescued by implicit rescue, which I think it's bad. Calling an abstract method is a script error, which should not be rescued.

Create a custom exception

I think this is fine, but if it's for an abstract method, I think it shouldn't inherit from StandardError. Of course, this highly depends on the context. I just think if it's inherited from StandardError, then the author is probably not thinking about having an abstract method (which makes the class an abstract class or interface)

Raise NoMethodError

I probably have the same comment as a custom exception, except that this is overloading the meaning of NoMethodError, which might or might not be what we want, depending on the context.

I agree with myself 2 years ago: #354314 (comment 865110293)

mentioned in merge request !177783 (merged)

mentioned in merge request !185960 (merged)

Discourage usage of NotImplementedError

Proposal

Rationale

Solutions

Designs

Child items ...

Activity