Bug #18632: Struct.new wrongly treats a positional Hash as keyword arguments

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
• Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN

[Eregon (Benoit Daloze)]

$ 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.

[jeremyevans0 (Jeremy Evans)]

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.

[Eregon (Benoit Daloze)]

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

[Eregon (Benoit Daloze)]

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.

[jeremyevans0 (Jeremy Evans)]

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.

[nobu (Nobuyoshi Nakada)]

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;
         }

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset git|e660b934b98943826f888f2b73f773c6411cd199.

---

A positional Hash is not keyword arguments [Bug #18632]