Feature #14278: Ambiguous Exception for OpenSSL::HMAC.digest

• Status: Open
• Assignee: -
• Target version: -

[KINGSABRI (KING SABRI)]

The OpenSSL::HMAC.digest shows unclear and ambiguous exception when key is nil.

require 'openssl'

key = nil
OpenSSL::HMAC.digest(OpenSSL::Digest.new('sha256'), key, 'RubyRuby')
TypeError: no implicit conversion of nil into String
`digest'

Expected Behavior
So clear and understandable issue, such:

key argument cannot be nil for OpenSSL::HMAC.digest

[Hanmac (Hans Mackowiak)]

the Error Message is common for all other stuff, check for example File.read(nil) has same error:

TypeError: no implicit conversion of nil into String

implicit conversion means that while an object might has a to_s method like nil, it doesn't has a to_str method
(difference between explicit and implicit conversion)

the moment when digest does try to convert nil into string, it doesn't know the variable name for this

---

similar error:

3 * "w"
TypeError: String can't be coerced into Integer

this one means that it failed the coerce between Numeric types

[KINGSABRI (KING SABRI)]

Hanmac (Hans Mackowiak) wrote:

the Error Message is common for all other stuff, check for example File.read(nil) has same error:

TypeError: no implicit conversion of nil into String

implicit conversion means that while an object might has a to_s method like nil, it doesn't has a to_str method
(difference between explicit and implicit conversion)

the moment when digest does try to convert nil into string, it doesn't know the variable name for this

---

similar error:

3 * "w"
TypeError: String can't be coerced into Integer

this one means that it failed the coerce between Numeric types

Thanks for your reply, and yeah that's the point. Errors must be more specific to reduce debugging and troubleshooting time.

so instead of going to look into the object code to understand the class/module/method code itself, it should be some clear error that helps me to fix my code.

[shevegen (Robert A. Heiler)]

If I understand it correctly, the change you propose is, from e. g.:

TypeError: no implicit conversion of nil into String

Towards:

key argument cannot be nil for OpenSSL::HMAC.digest

I am not at all against a more verbose/descriptive description at all, mind
you. So I am not against the general gist of your idea - for example, you
add in your example which method is the one that has a problem. And this
is a perfectly decent proposal.

However had, at the same time, I am not sure if you are aware of not, your
proposal above actually also implicitly gets rid of the TypeError error.
So ruby hackers would no longer be able to easily know which error they
can expect to intercept e. g. via a "rescue TypeError" clause. So I am
against that particular part of the proposal, because I would lose that
functionality. Again - I am not at all against the added information as to
which method has that problem, so that part is fine.

The other part of your proposed change, I think it is a question of style
e. g. "cannot be nil" versus "no implicit conversion of nil into".

I also think that we lose some information here, aka that ruby does not
want to do an implicit (automatic/automagic) conversion.

In the past, ruby has had a very short and often not too terribly useful
error reporting system, e. g. in ruby 1.8.x.

At a later time, things became slightly better and better; the did-you-mean
gem, but also different backtrace showing (the error is now shown first,
rather than having people scroll upwards in the terminal if the backtrace
is very long). Then there is the whole warning-system in ruby, also $SAFE -
I am sure the ruby core team will eventually improve on all of that, with
matz approving changes. :)

I myself have, I think, suggested at several times to be able to more easily
"fine tune" error/warning messages in general, and also be able to tell ruby
which errors/warnings to report and which to ignore. A bit like rubocop
where you can, in principle, modify rubocop's behaviour to report exactly
what you want/need (and to also enforce most of it, via the autocorrect option,
actually).

Anyway, I don't want to digress too much, so back on topic.

Two suggestions - let me know what you think about them.

(1) First, trying to put things into one line, again the current way, your
proposal, and my attempt to unify it a bit.

TypeError: no implicit conversion of nil into String

key argument cannot be nil for OpenSSL::HMAC.digest

->

TypeError: no implicit conversion of nil into String (for OpenSSL::HMAC.digest)

or

TypeError: no implicit conversion of nil into String (key argument cannot be nil for OpenSSL::HMAC.digest)

The last one is a bit long. I am fine with showing the name of the method, but
I really do not want to lose the extra information TypeError.

(2) Alternatively, perhaps we could have a way for ruby to provide more verbose
warnings. This could be handled and set internally perhaps, in some module ...
I think this may have been suggested in the past; and Jeremy Evans also proposed
something related to all of that (or perhaps it was just $SAFE, my memory is so
bad...). Some method that allows us to specify how ruby internally treats warnings,
e. g. verbosity level, via a method; or if it has to be, via a global, such as
$VERBOSE - but I think a method may be more elegant. This is a side issue though,
the more important thing is IMO, to be able to control the warning or verbosity
situation.

And then, following your suggestion, we could also additionally have a way to
control the behaviour of ruby via an environment setting, such as
"RUBY_VERBOSE_WARNINGS: 1" or something like that.

That way, we could report errors in a much more descriptive way, if the user has
set it so (default is 0; note that I here refer to the environment variable ...
we have to set it to some value; within ruby itself, we could use true/false of
course).

IF that verbose warning is set to true, no matter via environment variable or
internally, the behaviour of ruby reporting errors could then be a lot more
verbose AND helpful.

Keep in mind that people are different. I personally would prefer the shorter
variant. But I understand everyone who wants a more descriptive and verbose
error message / warning message.

If verbose warnings are enabled, then we could have multiple lines of feedback
or something like that. In colours too, to pick up nobu's old april joke
suggestion! It was a good joke because it can not be distinguished from a
real suggestion ... :)

Anyway, with verbosity enabled, it could then be several lines such as:

"The key argument cannot be nil for the method OpenSSL::HMAC.digest()."
"This causes ruby to raise a TypeError error."

^^^^ and then perhaps one more line, similar to the spirit of did-you-mean gem,
to give people a SHORT indicative of how they could solve this error.

Something like:

"Consider passing a variable/object other than nil to this method, in"
"order to fix this error."

Or something like that.

Obviously this also needs some more changes in general, so matz has to decide
whether this would be really useful or not; if the ruby core team needs
descriptions, I am sure that other ruby hackers can try to help what the
"best" message may be for people to fix certain errors.

Let's always remember that while the status quo may not be perfect, many people
are fine with the way things are too, even if the error message is not ideal
either - but it is short/succinct. And not all alternative suggestions are
"created equal" ...

[KINGSABRI (KING SABRI)]

thanks @shevegen (Robert A. Heiler) for your reply.
and yeah I believe we completely agree on everything here. The error should be more description at the same time not losing the known error class in our case (TypeError). the backtrace or the message that the one should be enhanced and be more specific.

Appreciate your fruitful discussion

[jeremyevans0 (Jeremy Evans)]

• Tracker changed from Bug to Feature
• ruby -v deleted (ruby 2.4.2p198 (2017-09-14 revision 59899) [x86_64-linux-gnu])
• Backport deleted (2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: UNKNOWN)

I do not think the current error message is a bug, and changing the message would be considered a feature. In general, Ruby TypeError messages are not completely descriptive as to the argument and method names. That is not specific to OpenSSL::HMAC. Example:

"" + nil
# TypeError (no implicit conversion of nil into String)

{}.merge(nil)
# TypeError (no implicit conversion of nil into Hash)

[] + nil
# TypeError (no implicit conversion of nil into Array)

Now, we could change all such error messages, by changing internal APIs to pass the method name, argument name, etc.. However, that is a ton of work for a minor benefit, and it doesn't really fit the dynamic nature of Ruby. The truth is, nil is an acceptable value for all of these methods, assuming you define the appropriate methods on it:

def nil.to_str; '' end
def nil.to_ary; [] end
def nil.to_hash; {} end

require 'openssl'

OpenSSL::HMAC.digest(OpenSSL::Digest.new('sha256'), nil, 'RubyRuby')
# => "\x06\e\xD0(\x99\xC8\xD9\fQ\xD7\x84\xAAZ\x9D\x9F\xF4\xFF#\x83\x83\x84[\xAD\x87\x7F\xC8\xB6\xF9\xADe\x17!"

"" + nil
# => ""

{}.merge(nil)
# => {}

[] + nil
# => []

Once you understand this, you'll probably understand why the error messages are the way they are.