Skip to content

Add specs for rb_interned_str and rb_interned_str_cstr#1327

Merged
eregon merged 6 commits intoruby:masterfrom
herwinw:rb_interned_str
Jan 18, 2026
Merged

Add specs for rb_interned_str and rb_interned_str_cstr#1327
eregon merged 6 commits intoruby:masterfrom
herwinw:rb_interned_str

Conversation

@herwinw
Copy link
Copy Markdown
Member

@herwinw herwinw commented Jan 16, 2026

Keeping this as a draft for now. The comment in the MRI source for rb_interned_str reads as follows:

/**
 * Identical to rb_str_new(), except it returns an infamous "f"string.  What is
 * a  fstring?  Well  it is  a special  subkind of  strings that  is immutable,
 * deduped globally, and managed by our GC.   It is much like a Symbol (in fact
 * Symbols  are dynamic  these days  and are  backended using  fstrings).  This
 * concept has been  silently introduced at some point in  2.x era.  Since then
 * it  gained  wider acceptance  in  the  core.   Starting from  3.x  extension
 * libraries can also generate ones.
 *
 * @param[in]  ptr           A memory region of `len` bytes length.
 * @param[in]  len           Length  of  `ptr`,  in bytes,  not  including  the
 *                           terminating NUL character.
 * @exception  rb_eArgError  `len` is negative.
 * @return     A  found or  created instance  of ::rb_cString,  of `len`  bytes
 *             length, of  "binary" encoding,  whose contents are  identical to
 *             that of `ptr`.
 * @pre        At  least  `len` bytes  of  continuous  memory region  shall  be
 *             accessible via `ptr`.
 */
VALUE rb_interned_str(const char *ptr, long len);

But the encoding of the result is always Encoding::US_ASCII, which leads to the following spec:

it "support binary strings that are invalid in ASCII encoding" do
  str = "foo\x81bar\x82baz".b
  result = @s.rb_interned_str(str, str.bytesize)
  result.encoding.should == Encoding::US_ASCII
  result.should == str.dup.force_encoding(Encoding::US_ASCII)
  result.should_not.valid_encoding?
end

I will create an issue for Ruby to get clarification about the desired behaviour.

@herwinw
Copy link
Copy Markdown
Member Author

herwinw commented Jan 16, 2026

Upstream: https://bugs.ruby-lang.org/issues/21842

@herwinw herwinw mentioned this pull request Jan 16, 2026
Closed
@herwinw
Update specs for rb_interned_str
5f23192 
Updated them to the current status of the upstream bug report, which is
all strings are binary. This might change to either ASCII-7BIT or
BINARY.

The current specs pass with the latest upstream version of Ruby 4.1
(commit 3e13b7d4ef)
@herwinw
Copy link
Copy Markdown
Member Author

herwinw commented Jan 17, 2026

Updated the specs to match the behaviour of ruby/ruby#15888.

This might not be the final version, ruby/ruby#15894 has a proposed change to return US-ASCII if everything is ASCII compatible, and BINARY otherwise.

@herwinw
Copy link
Copy Markdown
Member Author

herwinw commented Jan 17, 2026

Updated again to match the new behaviour of ruby/ruby#15894

@herwinw
Update specs to new behaviour
f737668 
It the whole string is valid ASCII, it returns US-ASCII encoding.
Otherwise, it is BINARY.

The current specs pass with the latest upstream version of Ruby 4.1
(commit 78b7646bdb)
eregon
eregon reviewed Jan 17, 2026
View reviewed changes
optional/capi/string_spec.rb Outdated
end
end

it "returns the same frozen strings for different encodings" do
Copy link
Copy Markdown
Member

@eregon eregon Jan 17, 2026

Choose a reason for hiding this comment

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

This description/test is confusing because the encoding is ignored by that function, it only receives char* and length.

eregon
eregon reviewed Jan 17, 2026
View reviewed changes
optional/capi/string_spec.rb Outdated
end
end

it "returns the same frozen strings for different encodings" do
Copy link
Copy Markdown
Member

@eregon eregon Jan 17, 2026

Choose a reason for hiding this comment

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

Same

@eregon eregon marked this pull request as ready for review January 17, 2026 09:30
eregon
eregon reviewed Jan 17, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks great, just 2 comments

@herwinw
Update specs for rb_interned_str(_cstr)
7266f1b 
Add full checks for the behaviour around US_ASCII/BINARY encoding
choice. Remove some of the checks that depend on the encoding of the
source string, since this function uses C char* without any encoding
information.
@herwinw
Add additional test for encoding of rb_str_to_interned_str
e1c587d 
The documentation in the MRI source refers to rb_interned_str as being
similar, this test is just to accentuate the difference between these
two funcitons.
@herwinw
Copy link
Copy Markdown
Member Author

herwinw commented Jan 18, 2026

I've removed both tests for encoded strings, and added two additional tests:

  • Specific tests for the decision on the returned encoding (US-ASCII or BINARY)
  • A test for rb_str_to_interned_str to actually validate the encoding of the returned value. The documentation in the MRI header file describe this function as "Identical to rb_interned_str, except ...", for this one it is important to preserve the encoding.

eregon
eregon reviewed Jan 18, 2026
View reviewed changes
@eregon eregon merged commit ddcd042 into ruby:master Jan 18, 2026
16 checks passed
@herwinw herwinw deleted the rb_interned_str branch January 18, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eregon eregon eregon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@herwinw @eregon