Bug #86127 for Class-XSAccessor: Predicates don't act like Moo/Mouse/Moose

Thu Jun 13 21:49:18 2013 perl [...] toby.ink - Ticket created

Subject:

Predicates don't act like Moo/Mouse/Moose

In Moose, predicates check: exists $object->{attribute} In Class::XSAccessor, predicates check: defined $object->{attribute} It would be nice if either this difference were eliminated, or greater attention could be drawn to it in the documentation.

Fri Jun 14 01:43:49 2013 smueller [...] cpan.org - Correspondence added

On Thu Jun 13 21:49:18 2013, TOBYINK wrote: Show quoted text

> In Moose, predicates check: > > exists $object->{attribute} > > In Class::XSAccessor, predicates check: > > defined $object->{attribute} > > It would be nice if either this difference were eliminated, or greater > attention could be drawn to it in the documentation.

I just pushed a commit that makes the hash implementation use exists instead of defined. In hindsight, it should have been that way from the start (and I can't say I need motivation from compatibility with other modules for that conclusion). I'm just worried that this is quite an incompatible change to make. :( What do others think about such a change? --Steffen PS: The documentation said "[...] for testing whether the attribute foo is defined in the object." which seems plenty clear to me. Anyway. Water under the bridge.

Fri Jun 14 01:43:49 2013 The RT System itself - Status changed from 'new' to 'open'

Fri Jun 14 06:44:20 2013 perl [...] toby.ink - Correspondence added

Based on grep.cpan.me, the following modules appear to be using Class::XSAccessor's "predicates" feature: * Moos (I'm a contributor to Moos, and speaking with my Moos hat on, this issue causes a subtle bug in Moos; changing the behaviour to match Moose would resolve the bug - though the bug could be solved within Moos instead by not using Class::XSAccessor to generate predicates.) * Asm::Preproc::Token This uses Class::XSAccessor::Array, so raises the question of how predicates should work there. * Padre::Wx::Main This might need testing. * File::PackageIndexer In test cases. I don't think it would make any difference here, but you're probably better placed to answer that one than I am. Although not showing up in the grep.cpan.me search, MooseX::XSAccessor also uses predicates, and the difference between Class::XSAccessor's predicates and Moose's predicates is listed as a CAVEAT. Moo notably does *not* use Class::XSAccessor's predicates, even though it uses the getters/setters/accessors. Perhaps because of this issue?

Sat Jun 15 11:09:59 2013 smueller [...] cpan.org - Correspondence added

On Fri Jun 14 06:44:20 2013, TOBYINK wrote: Show quoted text

> Based on grep.cpan.me, the following modules appear to be using > Class::XSAccessor's "predicates" feature: > > * Moos > > (I'm a contributor to Moos, and speaking with my Moos hat on, this > issue causes a subtle bug in Moos; changing the behaviour to match > Moose would resolve the bug - though the bug could be solved within > Moos instead by not using Class::XSAccessor to generate > predicates.) > > * Asm::Preproc::Token > > This uses Class::XSAccessor::Array, so raises the question of how > predicates should work there. > > * Padre::Wx::Main > > This might need testing. > > * File::PackageIndexer > > In test cases. I don't think it would make any difference here, but > you're probably better placed to answer that one than I am. > > Although not showing up in the grep.cpan.me search, MooseX::XSAccessor > also uses predicates, and the difference between > Class::XSAccessor's predicates and Moose's predicates is listed as > a CAVEAT. > > Moo notably does *not* use Class::XSAccessor's predicates, even though > it uses the getters/setters/accessors. Perhaps because of this > issue?

Heh, don't underestimate non-CPAN code! Nonetheless, I appreciate your help in checking. I lean towards changing behaviour. For arrays, however, it must remain a defined check. This is because otherwise, one would just be exposing an implementation detail (placeholders).

Sat Jun 15 11:24:51 2013 CHOCOLATE [...] cpan.org - Correspondence added

Would it be a lot of work (work I'm not volunteering to do :-) to provide both (for Hash)? i.e. keep "predicates" as-is and provide an option for those who want to use exists(). I'm guessing the hard part is coming up with a name. How about "checks"? use Class::XSAccessor { checks => { has_foo => 'foo', has_bar => 'bar', }, }; -- chocolateboy

Sat Jun 15 11:38:30 2013 CHOCOLATE [...] cpan.org - Correspondence added

Incidentally, from the name I would expect predicate(s) to check for true/false rather than defined/not-defined. But providing predicates for all 3 cases (exists, defined, and true/false) would involve coming up with names (preferably backwards compatible) that work as plurals. These are the best I can think of off the top of my head. They're pretty verbose, which is not ideal, but at least they have the virtue of clarity: bool_predicates (or boolean_predicates) exists_predicates defined_predicates

Sat Jun 15 11:44:26 2013 CHOCOLATE [...] cpan.org - Correspondence added

I guess there's also this: predicates => { defined => { has_foo => 'foo', }, exists => { has_bar => 'bar', }, bool => { has_baz => 'baz', }, }

Sat Jun 15 12:30:36 2013 smueller [...] cpan.org - Correspondence added

Implementing the other variants is easy enough. Sadly a bit of c&p code, but given the current code base, that's not the worst thing on the planet. Regarding the nested hash syntax, I'm not in favour of that because it would work differently from the other bits of syntax and that's just plain confusing. If I were to implement all of these, I'd go for the "FOO_predicates" idea and make "predicates" an alias for the defined check that we have now so that we don't break anybody's code. Does that sound like a plan?

Sat Jun 15 12:41:14 2013 CHOCOLATE [...] cpan.org - Correspondence added

Show quoted text

> Does that sound like a plan?

It does to me...

Sun Jun 16 08:20:34 2013 perl [...] toby.ink - Correspondence added

Sounds like a good idea to me. My main desire is that Class::XSAccessor is able to provide Moose-like predicate methods. What it calls those in the API I'm less concerned about. Bool predicates seem (from my perhaps naive perspective; I know very little about XS programming) like they'd be fairly annoying to implement (they'd need to take into account that the hash value might be an overloaded object) for very little win (getters already accomplish the same job).

Sun Jun 16 09:08:00 2013 CHOCOLATE [...] cpan.org - Correspondence added

Show quoted text

> Bool predicates seem ... like they'd be fairly annoying to implement > (they'd need to take into account that the hash value might be an > overloaded object)

This is covered by SvTRUE(sv).

Sun Jun 16 09:08:34 2013 smueller [...] cpan.org - Correspondence added

I implemented defined_p. and exists_p. in the master branch. Added basic docs and tests - all seems fine. predicates remains an alias for defined_predicates for backcompat. I didn't implement boolean predicates since indeed, the ordinary accessors fulfil that role apart from a bool cast. Review appreciated.

Sun Jun 16 09:32:44 2013 CHOCOLATE [...] cpan.org - Correspondence added

Looks good to me. I'm not sure about the "can be done already with getters" line of argument, since the same could be said of defined() predicates. If people have a use for boolean predicates, it makes sense to implement them entirely in XS, rather than requiring clients to incur the cost of an extra "cast" in Perl. That said, they're out of the scope of this ticket, and it may well be that no-one needs that optimization.

Sun Jun 16 14:27:04 2013 perl [...] toby.ink - Correspondence added

For the majority of cases where you'd use a boolean predicate, you don't even need a cast though. if ($obj->boolean_predicate) can, yes, be written with a cast, as if (!! $obj->getter) but it can also be written without a cast, as if ($obj->getter) to the same effect.

Sun Jun 16 15:02:20 2013 CHOCOLATE [...] cpan.org - Correspondence added

Show quoted text

> For the majority of cases where you'd use a boolean predicate, you > don't even need a cast though.

It depends how they're being used. If they're being printed, or serialised (e.g. to a database or JSON), then they're likely to need the "cast".

Mon Jun 17 01:17:41 2013 smueller [...] cpan.org - Correspondence added

On Sun Jun 16 15:02:20 2013, CHOCOLATE wrote: Show quoted text

> > For the majority of cases where you'd use a boolean predicate, you > > don't even need a cast though.

> > It depends how they're being used. If they're being printed, or > serialised (e.g. to a database or JSON), then they're likely to need > the "cast".

Ack. It's a valid desire, but until somebody asks for it, I won't commit more copy&paste. :) I've just released a new version of Class::XSAccessor that has the defined and exists predicates. Closing this ticket for now. --Steffen

Mon Jun 17 01:17:42 2013 smueller [...] cpan.org - Status changed from 'open' to 'resolved'