Improve MatchData#dup

[chrisseaton]

Authored by @XrXr.

Shopify#1

[eregon]

Is there anything that relies on MatchData.allocate "working"?
If not, it could be tempting to just not allow MatchData.allocate and raise (as it was).

[eregon]

Could you check getSource(matchData) != null here?
getRegexp() can already return multiple kinds of values (see comment).

[eregon]

@Cached should be on its own line (jt checkstyle checks that)

[XrXr]

Is there anything that relies on MatchData.allocate "working"?

The goal for this was to get MatchData#dup to work. MRI just uses Kernel#dup for that, which requires allocate to work. This PR also makes MatchData#clone work, which also requires allocate. Alternatively, we could implement each specifically for MatchData but I think we will end up with the same amount of complexity.

Don't really have a strong opinion on this either way, just following MRI here. One thing to note is that there is no specs to make sure dup and clone work correctly w.r.t. copying instance variable and freeze state and things like that. It would probably be a good idea to have those tests going whichever implementation we end up going with.

[eregon]

The goal for this was to get MatchData#dup to work. MRI just uses Kernel#dup for that, which requires allocate to work. This PR also makes MatchData#clone work, which also requires allocate.

Indeed, that's how MRI does it, and it's generally good to follow MRI behavior for maximum compatibility.
Do you know what uses MatchData#dup? I can't think of any use case, since the internal MatchData fields are basically all immutable.

Alternatively, we could implement each specifically for MatchData but I think we will end up with the same amount of complexity.

I think it would make the changes more localized, by just having to implement MatchData#dup and MatchData#clone.

• We could also come up with our own protocol for duping with Kernel#dup without needing to mutate the object's internal fields (e.g., klass.allocate_copy(original)), but that seems quite a bit of complication just for this case (there might be other classes benefiting from such an approach though). The main disadvantage there would be it's probably hard to make it fully compatible.
• I think MatchData should actually be frozen, and then dup/clone would just return self. But that's not how it is in MRI currently, and that would likely be less compatible with user code.

Don't really have a strong opinion on this either way, just following MRI here. One thing to note is that there is no specs to make sure dup and clone work correctly w.r.t. copying instance variable and freeze state and things like that. It would probably be a good idea to have those tests going whichever implementation we end up going with.

I think those specs are less necessary with your approach, there we can rely on Kernel#{dup,clone} specs.

I'll merge this approach, it's straightforward and aims for maximum compatibility with MRI.
Thank you for the fix and the specs!

[eregon]

MRI issue to discuss whether we can make MatchData frozen:
https://bugs.ruby-lang.org/issues/16294
I'm tempted by that approach, it sounds a lot cleaner.

[eregon]

Nobu made MatchData#allocate undefined, so I think I'll go for a similar approach in TruffleRuby:
https://bugs.ruby-lang.org/projects/ruby-trunk/repository/git/revisions/aa94245a09887f95bc0cd353b3462108d76d13ed/diff

[eregon]

What we can do here is leave __allocate__ defined as it is to allow the normal copy mechanism, but remove the user-visible allocate to make it clear we don't support half-initialized MatchData.

[eregon]

I did that in 32791da.