Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use spaces around => in Hash#inspect for symbol keys that would generate invalid syntax #10737

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jeremyevans
Copy link
Contributor

Also use spaces around => in PP.pp for hashes with symbol keys that would generate invalid syntax.

Fixes [Bug #20433]

@jeremyevans jeremyevans requested a review from nobu May 7, 2024 18:59
@@ -3449,7 +3450,18 @@ inspect_i(VALUE key, VALUE value, VALUE str)
rb_enc_copy(str, str2);
}
rb_str_buf_append(str, str2);
rb_str_buf_cat_ascii(str, "=>");
if (RB_SYMBOL_P(key)) {
switch (*(RSTRING_PTR(str) + RSTRING_LEN(str) - 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit nitpicky, but are symbols guaranteed to be in any particular encoding? This check might incorrectly not-space a symbol that does need it?

Maybe the way to go is to just change the separator to " => " unconditionally, then we don't have to play this game.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to always having spaces, but that is a much less compatible change that probably needs approval from Matz. I'm not sure whether there are other failing cases besides the ones reported in the bug report (this one fixes the problematic cases specified in the bug report). Symbols can be in any encoding, but inspect does not try to dump the encoding information.

Copy link
Contributor

@KJTsanaktsidis KJTsanaktsidis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 should work in most cases at least and won’t do any harm in the cases where the symbol is in a strange encoding where !/?/etc don’t match ascii values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants