Fix fallback argument for Psych.load

[stomar]

[WIP: depends on a solution to #340, see also below]

@hsbt I think this should be solved in the upcoming 3.0 release.

There is an error in the implementation of the fallback argument for Psych.load which causes an exception; it only works with Psych.load_file currently.

Before the fix:

$LOAD_PATH.unshift("./lib")

require "tempfile"
require "yaml"

Tempfile.create("empty") do |f|
  Psych.load_file(f)      # => false
end

Tempfile.create("empty") do |f|
  Psych.load_file(f, 42)  # => 42
end

Psych.load("")            # => false
Psych.load("", nil, 42)   # => NoMethodError: undefined method `to_ruby' for 42:Integer\nDid you mean?  to_r

However, it shouldn't make a difference whether the loaded yaml is read from a file or a string, so load and load_file should behave in the same way and both (or none) accept a fallback argument.

After the fix:

# ...

Psych.load("")            # => false
Psych.load("", nil, 42)   # => 42

Problem: This will not work for hashes currently, because of the (newly introduced) symbolize_names keyword argument for Psych.load. The keyword argument does not play well with the optional arguments. The provided hash is not parsed as the value for the optional fallback argument, unless the keyword argument is provided, too (see #340).

Psych.load("", nil, {})   # => false (!!!)
Psych.load("", nil, {}, symbolize_names: true)   # => {}

(This is the reason for the current test fails.)

BTW. @tenderlove mentioned in #149 and #264 that a default return value of nil might make more sense, but this could only be changed with a major release. 3.0?

[k0kubun]

Note that my change was backward compatible unlike this change. This interface change (wrapping Psych::FALLBACK implicitly) is breaking.

If you really want this kind of breaking change (probably acceptable as major upgrade if we yank 3.0.0), my suggestion is using keyword arguments for fallback too instead. It would be scalable for future extension.

[stomar]

I thought about this some more, and am even more convinced that the way the new option was implemented is wrong.

It should not make any difference whether a yaml file is loaded using load_file or by first reading it into a string and then using load. Both methods should behave in the exact same way, and provide the same options. (That's how load_file was implemented in the first place, by simply wrapping load in File.open.) It dosen't make any sense that load blows up when provided with a fallback value, and certainly is not an intended behaviour that's meant to be relied upon. It's a bug (the fallback is wrapped with FALLBACK at the wrong place).

This PR does not break backwards compatibility, it fixes this bug.

On the other hand, keeping the new option (symbolize_names or symbolize_keys) the way it is would effectively cement this wrong behaviour and prevent fixing this bug for a long time, and additionally increase the differences between the two methods even further (both methods should have---or not have---that option).

[k0kubun]

How about changing "fallback = false" to "fallback: false"? With that change, users can easily notice fallback's behavior is fixed/changed.

[k0kubun]

Or another idea is having "legacy_fallback = false, fallback: false" and keeping legacy_fallback's behavior for migration, deprecating legacy_fallback usage.

[stomar]

fallback: false sounds good to me, if the compatibility break is acceptable and if this doesn't cause problems in any of the other methods that possibly should be considered, too (safe_load, load_stream, and so on).

legacy_fallback doesn't convince me, I would guess it has the exact same problem with hash arguments; also I think fallback is not widely used currently.

[k0kubun]

fallback: false sounds good to me, if the compatibility break is acceptable and if this doesn't cause problems in any of the other methods that possibly should be considered, too (safe_load, load_stream, and so on).

I agree with it. That should depend on maintainer's decision anyway.

[stomar]

Since the fallback argument was in the meantime changed to a keyword argument, closing in favour of

• Add more test cases for fallback of Psych.load_file #345: (uncontroversial) documentation fix and adding of more test cases for the current (unchanged) behaviour of Psych.load_file
• Fix fallback keyword argument of Psych.load #346: possibly controversial bug fix / breaking change of Psych.load's fallback keyword argument as discussed above