Inconsistency of IndexError vs nil for unknown capture group

[eregon]

After matching with a String, StringScanner#[:unknown_capture_group] returns nil:

$ irb -rstrscan
irb(main):001> s=StringScanner.new("abc")
=> #<StringScanner 0/3 @ "abc">
irb(main):002> s.scan("a")
=> "a"
irb(main):003> s[:foo]
=> nil

After matching with a Regexp, StringScanner#[:unknown_capture_group] raises IndexError:
(using a new process to be sure to not be affected by #135)

irb -rstrscan
irb(main):001> s=StringScanner.new("abc")
=> #<StringScanner 0/3 @ "abc">
irb(main):002> s.scan(/./)
=> "a"
irb(main):003> s[:foo]
(irb):3:in `[]': undefined group name reference: foo (IndexError)

It would be best if the behavior is consistent.

There is a test for the nil case in https://github.com/ruby/strscan/blob/v3.1.2/test/strscan/test_stringscanner.rb#L830-L840
And there are some tests for the IndexError cases in https://github.com/ruby/strscan/blob/v3.1.2/test/strscan/test_stringscanner.rb#L443-L444 and in more places.

The implementation is

strscan/ext/strscan/strscan.c

Line 1764 in 6968b11

if (!RTEST(p->regex)) return Qnil;

This is proving problematic on TruffleRuby.
We would like to upstream our pure-Ruby implementation of StringScanner, but to do so we want to avoid exposing internals as much as possible.
Things like p->regex is something I don't want to expose to code living outside the truffleruby repository.
Having to workaround by keeping extra state for whether the last match was with a String or a Regexp feels very messy and wrong semantically.

Could the behavior be unified for both of these cases?

[eregon]

Given the Regexp-matching methods predate the String ones, I think it would be best to have IndexError in both cases.
Using a String pattern should be equivalent to a Regexp in StringScanner & transparent.

Though returning nil for both cases can also be implemented easily on TruffleRuby.

The worse is the mix of both which exposes internal details of the StringScanner implementation.

BTW this inconsistency seems to have been introduced in a0ff956 which is ruby/ruby@90311f3 and https://bugs.ruby-lang.org/issues/13759

[kou]

OK. Let's use IndexError for String too.

@naitoh Do you want to work on this?

[naitoh]

Do you want to work on this?

Yes, I would like to try this issue.

[andrykonchin]

Wondering how #[name] should behave when a String pattern is used but matching failed - to raise IndexError or to return nil.

When a Regexp pattern is used #[name] returns nil:

s = StringScanner.new("abc")
s.scan(/\d/) # => nil
s[:a] # => nil

Returning nil looks consistent with handling a Regexp pattern. But is inconsistent with handling a String pattern after successful matching (when IndexError is raised).

[eregon]

I think the logic should be:

• There is always a "result of the last StringScanner match" (some kind of MatchData internally or nil) whether matched against String pattern or Regexp pattern
• If the last match failed, the last result is nil, then [] returns nil for any group
• If the last match succeeded, then use the semantics of MatchData#[] or equivalent, which raises IndexError on missing group. For a String pattern there are of course no named groups.

[kou]

Let's use the same behavior as a Regexp pattern.