Type::Tiny::Enum - change ::XS encoding to quote strings

[aeruder]

If we have something like Enum[qw(a b)] we would communicate with
Type::Tiny::XS via a string like:

'Enum[a, b]'

We've already been hitting issues with this (see perl rt #129729) on hyphens
but now also hitting it on spaces (see perl rt #133141). This commit
changes the encoding for enums for Type::Tiny::XS to:

'EnumQ["a", "b"]'

Since we use B::perlstring() to encode this, we should also handle any
arbitrary string.

Why change Enum -> EnumQ? We've introduced a weird API change,
Type::Tiny::XS < 0.20 won't know how to parse an Enum[] with quoted
strings, but if we pass it an EnumQ[] it'll just silently move past
it (and not be able to optimize it). Then we can release a
Type::Tiny::XS that supports EnumQ and get all the SPEED back.

[aeruder]

This along with a corresponding change over in the p5-type-tiny-xs repo is one proposed fix to the Enum encoding issues.

Also could fix 133141 by just adding another character to the "can't xsify" regex (along with the existing hyphen) or put something together where the interface between Type::Tiny and Type::Tiny::XS isn't via strings

[codecov]

Codecov Report

Merging #59 into master will decrease coverage by 0.23%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   88.80%   88.57%   -0.24%     
==========================================
  Files          36       36              
  Lines        3583     3597      +14     
  Branches      984      991       +7     
==========================================
+ Hits         3182     3186       +4     
- Misses         62       64       +2     
- Partials      339      347       +8     

Impacted Files	Coverage Δ
lib/Type/Tiny/Enum.pm	75.90% <71.42%> (-9.61%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a06a45...9072674. Read the comment docs.

[coveralls]

Coverage decreased (-0.05%) to 98.552% when pulling 9072674 on aeruder:xs-enum-encoding-bump into acd6114 on tobyink:master.

[tobyink]

I admire your commitment to sticking with the established API, but feel like at this point it might be easier to just do:

my $xsub = Type::Tiny::XS::_parameterize_Enum_for( $opts{unique_values} );

Yes, _parameterize_Enum_for isn't part of Type::Tiny::XS's officially supported API, but meh, Type::Tiny and Type::Tiny::XS can be friends with benefits.

[aeruder]

Hmmm, I’m pretty sure that would allow a Type::Tiny only change. I can take a stab at that.

…

Sent from my iPhone

On Aug 12, 2020, at 3:34 AM, Toby Inkster ***@***.***> wrote:  I admire your commitment to sticking with the established API, but feel like at this point it might be easier to just do: my $xsub = Type::Tiny::XS::_parameterize_Enum_for( $opts{unique_values} ); Yes, _parameterize_Enum_for isn't part of Type::Tiny::XS's officially supported API, but meh, Type::Tiny and Type::Tiny::XS can be friends with benefits. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[aeruder]

Yea, that's a pretty involved change too that might trigger changing the interface to Type::Tiny::XS entirely.

For Tuple[Enum[...]] to inline, we need to register the callback with Type::Tiny::XS (so that Type::Tiny::XS::is_known works) and it still needs to return something parse-able by Type::Parser (so we'll still want the B::perlstring encoding). The first part specifically means we'd probably also have to drag the AUTO::TC%d named subname logic into Type::Tiny::Enum for the inline_check to work out AND also attempt to register the sub to Type::Tiny::XS::_know. This is all assuming the intent here is not require changes in both Type::Tiny and Type::Tiny::XS for this.

Another option I could pursue is not renaming the encoding to EnumQ[], leaving it Enum[] (now with perlstring encoding) and then add some lame check for " in the contents to understand if Type::Tiny::XS is being called from a old Type::Tiny or new Type::Tiny. I'm pretty sure that would work out, but might unintentionally break someone using " in a string in an old Type::Tiny Enum (although that would have exploded on that person if they then tried to wrap that Enum in a Tuple[] and triggered the Type::Parser error).

[tobyink]

Hmm, if Type::Tiny::XS encounters Enum[ and the following character is a quote character (either " or ') then treat the strings as all quoted, and if the following character is not a quote character, treat the strings as barewords and use split.

Meanwhile Type::Tiny::Enum can do a version check on Type::Tiny::XS and pass the strings quoted if it's version 0.020+. If it's older Type::Tiny::XS, bail out from using XS if the strings don't all match /^\w+$/, and pass the strings unquoted if they do all match /^\w+$/.

[aeruder]

That sounds reasonable to me and pretty sure it'll work. I'll update the pull request in a bit.

[aeruder]

Here we go (along with the Type::Tiny::XS pull request which I've updated too). Pretty much what you suggested, version-check Type::Tiny::XS, if we can use the new encoding, otherwise use the old one (but avoid it for these broken space-including strings). This makes the assumption that the version of ::XS that includes the fix will be 0.020.

[aeruder]

This appears to be passing all tests now (with old Type::Tiny::XS and new Type::Tiny::XS [manually patched to 0.020 since the version check in here for the new API is 0.020+]). Was able to distinkt-dist it as well.

[aeruder]

Built a local Type-Tiny-1.010004 and Type-Tiny-XS-v0.020 and have confirmed that this gets at least the first half of our internal testsuite at ziprecruiter through the Type::Tiny upgrade. 🎉

[tobyink]

Released Type::Tiny::XS 0.020. Going to wait a day or so for CPAN testers to do their stuff with that before releasing a new Type::Tiny, though I don't anticipate any problems with it.