Bug #18632
closedStruct.new wrongly treats a positional Hash as keyword arguments
Description
$ ruby -e 'Struct.new(:a, name: "b")'
-e:1:in `new': unknown keyword: :name (ArgumentError)
from -e:1:in `<main>'
^ expected
$ ruby -e 'Struct.new(:a, { name: "b" })'
-e:1:in `new': unknown keyword: :name (ArgumentError)
from -e:1:in `<main>'
^ wrong
It shouldn't be such an error for the 2nd command since it's a positional Hash.
It should be a TypeError, like when passing e.g. nil
instead of the positional Hash.
Also:
$ ruby -e 'p Struct.new(:a, {}).members'
[:a]
But it should be an error to pass a positional Hash.
I think this is worth fixing, because it basically breaks the separation of positional and keyword arguments for this method.
Also Struct.new does take a keyword argument, keyword_init: true
.
Updated by jeremyevans0 (Jeremy Evans) over 2 years ago
A lot of C methods will still treat positional hashes as keywords. I think only those that use rb_scan_args
/rb_scan_args_kw
will error if a regular hash is passed instead of keywords. For example, you can call Kernel#raise
with a positional hash and it will treat the hash as keywords. This explains the behavior of the second Struct issue with the empty hash, which is treated as empty keywords and is ignored.
In short, this is not an issue specific to Struct.new
. If we consider this a bug, we have to modify all C functions that use option hashes/keywords and do no use rb_scan_args
/rb_scan_args_kw
, and make them raise ArgumentError if passing a positional hash and not keywords. Even that wouldn't change the behavior of C function methods defined in external gems.
Updated by Eregon (Benoit Daloze) over 2 years ago
And another maybe related issue, this time in Struct#initialize:
$ ruby -we 'p Struct.new(:name, :legs, keyword_init: true).new(name: "elefant", legs: 4)'
#<struct name="elefant", legs=4>
^ OK
$ ruby -we 'p Struct.new(:name, :legs, keyword_init: true).new(4)'
-e:1:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError)
from -e:1:in `new'
from -e:1:in `<main>'
^ fails as expected
$ ruby -we 'p Struct.new(:name, :legs, keyword_init: true).new({name: "elefant", legs: 4})'
#<struct name="elefant", legs=4>
^ should fail, it is not kwargs
And this example also show that providing keyword_init: true
can cause an inconsistency:
I'm using master here to simplify:
$ ruby -we 'p Struct.new(:name, :legs, keyword_init: true).new({name: "elefant", legs: 4})'
#<struct name="elefant", legs=4>
^ incorrect
$ ruby -we 'p Struct.new(:name, :legs).new({name: "elefant", legs: 4})'
#<struct name={:name=>"elefant", :legs=>4}, legs=nil>
^ correct
$ ruby -we 'p Struct.new(:name, :legs).new(name: "elefant", legs: 4)'
#<struct name="elefant", legs=4>
^ correct
Updated by Eregon (Benoit Daloze) over 2 years ago
For Kernel#raise
I found that it's actually important to separate positional and kwargs, because raise
does have cause:
kwargs and optional args, and the 1st or 2nd argument can somtimes be a Hash: https://github.com/oracle/truffleruby/issues/2298
You mean rb_scan_args/rb_scan_args_kw correctly separate positional & kwargs, but the rest do not?
I think there is nothing we need to do for C functions not accepting kwargs, because then anyway there is no difference.
But for C functions taking kwargs they should not mix positional & kwargs and that is worth fixing.
Updated by jeremyevans0 (Jeremy Evans) over 2 years ago
Eregon (Benoit Daloze) wrote in #note-3:
You mean rb_scan_args/rb_scan_args_kw correctly separate positional & kwargs, but the rest do not?
Correct. I think most C functions that handle kwargs use rb_scan_args
, but not all.
I recommend you add this as topic to the next developer meeting. Changes in this area will break backwards compatibility, so even if we decide to make them, we need to have an implementation plan, such as issuing deprecation warnings in 3.2 and breaking use with positional hashes in 3.3. I'm willing to do the work of auditing all core/ext methods and updating those that need changes if we decide to make this change.
Updated by nobu (Nobuyoshi Nakada) over 2 years ago
rb_check_arity
with unlimited arguments are:
array.c:2420: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
array.c:7602: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
eval.c:1138: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
eval.c:1195: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
eval.c:1649: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
eval.c:1754: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
ext/-test-/iter/yield.c:6: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
ext/io/console/console.c:1456: rb_check_arity(argc, 0, UNLIMITED_ARGUMENTS);
ext/syslog/syslog.c:304: rb_check_arity(argc, 2, UNLIMITED_ARGUMENTS);
ext/win32ole/win32ole.c:3353: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
ext/zlib/zlib.c:3225: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
file.c:374:#define apply2args(n) (rb_check_arity(argc, n, UNLIMITED_ARGUMENTS), argc-=n)
hash.c:4579: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
proc.c:2615: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
process.c:2584: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
signal.c:432: rb_check_arity(argc, 2, UNLIMITED_ARGUMENTS);
string.c:8179: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
string.c:8438: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
struct.c:580: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
struct.c:1517: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
vm_eval.c:2193: rb_check_arity(argc, 2, UNLIMITED_ARGUMENTS);
vm_method.c:2393: rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS);
Probably only these two.
diff --git i/eval.c w/eval.c
index d91440676fb..cb2b3779fe4 100644
--- i/eval.c
+++ w/eval.c
@@ -704,6 +704,9 @@ extract_raise_opts(int argc, const VALUE *argv, VALUE *opts)
if (!RHASH_EMPTY_P(opt)) {
ID keywords[1];
CONST_ID(keywords[0], "cause");
+ if (!rb_scan_args_keyword_p(RB_SCAN_ARGS_PASS_CALLED_KEYWORDS, opt)) {
+ rb_warn_deprecated_to_remove("3.3", "passing non-keywords Hash", "**");
+ }
rb_get_kwargs(opt, keywords, 0, -1-raise_max_opt, opts);
if (RHASH_EMPTY_P(opt)) --argc;
return argc;
diff --git i/struct.c w/struct.c
index 8b19266e62d..c3af0830105 100644
--- i/struct.c
+++ w/struct.c
@@ -589,11 +589,15 @@ rb_struct_s_def(int argc, VALUE *argv, VALUE klass)
if (RB_TYPE_P(argv[argc-1], T_HASH)) {
static ID keyword_ids[1];
+ VALUE opt = argv[argc-1];
if (!keyword_ids[0]) {
keyword_ids[0] = rb_intern("keyword_init");
}
- rb_get_kwargs(argv[argc-1], keyword_ids, 0, 1, &keyword_init);
+ if (!rb_scan_args_keyword_p(RB_SCAN_ARGS_PASS_CALLED_KEYWORDS, opt)) {
+ rb_warn_deprecated_to_remove("3.3", "passing non-keywords Hash", "**");
+ }
+ rb_get_kwargs(opt, keyword_ids, 0, 1, &keyword_init);
if (keyword_init == Qundef) {
keyword_init = Qnil;
}
Updated by nobu (Nobuyoshi Nakada) over 2 years ago
- Status changed from Open to Closed
Applied in changeset git|e660b934b98943826f888f2b73f773c6411cd199.
A positional Hash is not keyword arguments [Bug #18632]