Shopify
[Discussion] Disable Style/RaiseArgs cop
This cop makes it more cumbersome to use custom exception classes that do not use a message as only argument to its constructor, e.g.
Class CommandFailure
def initialize(command)
@command = command
super("`#{command}` (pid: #{command.pid}) #{termination_status}")
end
end
raise CommandFailure.new(command_that_failed)
According to the cop, I should change the raise call to
raise CommandFailure, command_that_failed
While that works (accidentally), that goes against how raise is documented in Ruby:
With no arguments, raises the exception in $! or raises a RuntimeError if $! is nil. With a single String argument, raises a RuntimeError with the string as a message. Otherwise, the first parameter should be the name of an Exception class (or an object that returns an Exception object when sent an exception message). The optional second parameter sets the message associated with the exception, and the third parameter is an array of callback information.
(The reason why you can use exception instance is an exception instance returns itself when you call #exception on it.)
There are several ways to work around this and appease Rubocop:
- Use a keyword argument rather than a positional argument:
CommandFailure.new(command: command_that_failed) - Introduce a second argument (for which I have no reason):
CommandFailure.new(command, nil) - Assign the exception to a variable first, and then raise the variable:
raise command_failure -
# rubocop:disable Style/RaiseArgs
When we force people to use a workaround like this, maybe Rubocop is not the best tool to enforce this. However, we can also decide that custom exceptions with a single argument constructor are rare enough that a workaround is acceptable.
How about using the compact style? If we always require a exception instance we would be able to be consistent and avoid making devs have to decide if they should create an instance or not.
@gmalette was saying something around raise ExceptionClass, "message" being more performant than raise ExceptionClass.new("message") but I am not sure if I remember it correctly.
I don't know if there is a practical difference in MRI, but VMs will typically be able to optimize the raise Class, msg more easily
I don't think we should stop enforcing this rule (or reverse it into the "compact" style) because I would expect any subclass of Exception to be able to receive new with an optional error message, and therefore the raise ErrorClass, "message" style should work for all those.
Having children of Exception getting initialized with values other than the message sounds like a Liskov violation to me, and that's a smell. I would not encourage that in the Style guide.
I definitely see cases when you want to raise an object that is already initialized, or was initialized in another fashion. For those I would suggest calling raise with the instance at hand in a local variable, for example.
I would expect any subclass of Exception to be able to receive new with an optional error message
That's not an expectation that the Ruby's core lib fulfills; some of the core exceptions don't accept strings as the first positional argument.
Having children of
Exceptiongetting initialized with values other than the message sounds like a Liskov violation to me
While raise FooException, not_a_string is a Liskov violation, raise FooException.new(not_a_string) is not.
in a local variable
... and that's not a smell?
VMs will typically be able to optimize the
raise Class, msgmore easily
@chrisseaton do you know if that is the case for TruffleRuby and JRuby?
TruffleRuby implements this logic in Ruby:
def raise(exc=undefined, msg=undefined, ctx=nil)
internal_raise exc, msg, ctx, false
end
module_function :raise
def internal_raise(exc, msg, ctx, internal)
skip = false
if Primitive.undefined? exc
exc = $!
if exc
skip = true
else
exc = RuntimeError.new ''
end
elsif exc.respond_to? :exception
if Primitive.undefined? msg
exc = exc.exception
else
exc = exc.exception msg
end
raise TypeError, 'exception class/object expected' unless exc.kind_of?(Exception)
elsif exc.kind_of? String
exc = RuntimeError.exception exc
else
raise TypeError, 'exception class/object expected'
end
unless skip
exc.set_context ctx if ctx
exc.capture_backtrace!(2) unless exc.backtrace?
Primitive.exception_set_cause exc, $! unless exc.equal?($!)
end
if $DEBUG
STDERR.puts "Exception: `#{exc.class}' #{caller(2, 1)[0]} - #{exc.message}\n"
end
Primitive.vm_raise_exception exc, internal
end
I think the key for this discussion is this bit:
elsif exc.respond_to? :exception
if Primitive.undefined? msg
exc = exc.exception
else
exc = exc.exception msg
end
raise TypeError, 'exception class/object expected' unless exc.kind_of?(Exception)
elsif exc.kind_of? String
exc = RuntimeError.exception exc
else
So we always check if it respond_to? :exception - if it does, we just call it, either with the message or not, if it doesn't we check if it's a string instead, etc. JRuby works in a similar way to this code here, but it's implemented in Java.
if (optionalMessage == null) {
exception = obj.callMethod(context, "exception");
} else {
exception = obj.callMethod(context, "exception", optionalMessage);
}
To me, raise FooException.new(message) is more explicit and better. raise FooException, message isn't a syntax replicated anywhere else in Ruby and continues to look strange to me.
I think raise FooException.new(message) is better for optimising VMs, but the reason why is quite complicated - it puts the call-site for the construction of the exception lower down the call stack and so in a place where it is more likely to get an independent inline cache, which will optimise better. This does apply to both JRuby and TruffleRuby in theory. I think you might struggle to prove it in a benchmark though, due to it being quite subtle in practice.
I would say that raise FooException.new(message) is cheaper for all of MRI, JRuby, and TruffleRuby to run, but it's not a strong difference.
In practice, I see most people write raise FooException, message and sometimes people ask me what I'm doing when I write raise FooException.new(message) instead.
