Bug #63000 for Moose: Regression from Moose 1.05 to 1.06 on HP-UX 11.31

Mon Nov 15 12:42:14 2010 philip.monsen [...] gmail.com - Ticket created

Subject:	Regression from Moose 1.05 to 1.06 on HP-UX 11.31
Date:	Mon, 15 Nov 2010 11:41:55 -0600
To:	bug-moose [...] rt.cpan.org
From:	Philip Monsen <philip.monsen [...] gmail.com>

Message body is not shown because it is too large.

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.06 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.07 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.08 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.09 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.10 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.11 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.12 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.13 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.14 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.15 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.16 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.17 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.18 added

Mon Nov 15 12:46:18 2010 philip.monsen [...] gmail.com - Broken in 1.19 added

Sun Nov 21 11:23:16 2010 DROLSKY [...] cpan.org - Correspondence added

I don't really know what the problem is. I'm inclined to blame it on a Perl core bug, though that's a bit of a cop out. The changes in 1.06 were really minor. Basically, we changed some code so that instead of comparing the refaddr() of two objects, the objects overloading numification by returning their refaddr, and now we just compare the objects directly. Could you post the results of testing the latest version of Moose? The test suite and internals have changed a fair bit since 1.06. You might also try with other versions of Perl, and/or with another compiler if you can, just to see if any of those are the source of the problem.

Sun Nov 21 11:23:17 2010 The RT System itself - Status changed from 'new' to 'open'

Tue Nov 23 10:36:28 2010 philip.monsen [...] gmail.com - Correspondence added

I'm pretty sure the problem is with some subtle effects introduced by the new overloading behavior added in v1.06. It's not that the defined overloads are wrong per se, it's just that there aren't enough of them -- at least for my brew of perl. If you also overload '==' and adjust '""' to allow for normal strings, then all tests for 1.06 pass. I arrived at this conclusion by experimenting with the effect of removing each of the overloads in turn. In particular when I removed the fallback overload, some tests failed with errors indicating that we were missing overloads for '==' and 'eq', i.e. that the numification and stringification weren't getting the job done in every possible case (which I suppose is why 'fallback' is set to true in the first place). Overloading 'eq' was problematic as it bled over into the tests and produced strange failures of things like cmp_ok() that should have succeeded, so I didn't do that. In the end, after adjusting the '""' spec and adding '==' I put the 'fallback' spec back. Here's a rough patch for the TypeContraints.pm in 1.06, I am guessing something similar would hold for later versions. With this patch, all tests pass given Class::MOP v1.02. On Sun Nov 21 11:23:16 2010, DROLSKY wrote: Show quoted text

> The changes in 1.06 were really minor. Basically, we changed some code > so that instead of comparing the refaddr() of two objects, the objects > overloading numification by returning their refaddr, and now we just > compare the objects directly.

Subject:

TypeConstraint.pm-patch_for_v1_06

Download TypeConstraint.pm-patch_for_v1_06
application/octet-stream 780b

Message body not shown because it is not plain text.

Tue Nov 23 10:43:12 2010 doy [...] tozt.net - Correspondence added

Subject:	Re: [rt.cpan.org #63000] Regression from Moose 1.05 to 1.06 on HP-UX 11.31
Date:	Tue, 23 Nov 2010 09:42:59 -0600
To:	Philip Monsen via RT <bug-Moose [...] rt.cpan.org>
From:	Jesse Luehrs <doy [...] tozt.net>

On Tue, Nov 23, 2010 at 10:36:28AM -0500, Philip Monsen via RT wrote: Show quoted text

> Queue: Moose > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=63000 > > > I'm pretty sure the problem is with some subtle effects introduced by > the new overloading behavior added in v1.06. It's not that the defined > overloads are wrong per se, it's just that there aren't enough of them > -- at least for my brew of perl. If you also overload '==' and adjust > '""' to allow for normal strings, then all tests for 1.06 pass. > > I arrived at this conclusion by experimenting with the effect of > removing each of the overloads in turn. In particular when I removed > the fallback overload, some tests failed with errors indicating that we > were missing overloads for '==' and 'eq', i.e. that the numification and > stringification weren't getting the job done in every possible case > (which I suppose is why 'fallback' is set to true in the first place). > Overloading 'eq' was problematic as it bled over into the tests and > produced strange failures of things like cmp_ok() that should have > succeeded, so I didn't do that. In the end, after adjusting the '""' > spec and adding '==' I put the 'fallback' spec back. > > Here's a rough patch for the TypeContraints.pm in 1.06, I am guessing > something similar would hold for later versions. With this patch, all > tests pass given Class::MOP v1.02.

That patch doesn't really make any sense at all... do you know why it works, or were you just fiddling with things until the tests passed? It wouldn't surprise me if the tests are only passing with that patch due to a lack of sufficient test coverage, because the patch doesn't seem to be doing anything in a more correct way (in particular, the changes to the stringification overload don't look like they should ever be triggered). -doy

Tue Nov 23 11:04:34 2010 philip.monsen [...] gmail.com - Correspondence added

Show quoted text

> That patch doesn't really make any sense at all... do > you know why it works, or were you just fiddling > with things until the tests passed?

I'll admit, my testing strategy focused exclusively on fixing failing tests (and not breaking others) by adjusting the module code, vs. adjusting the test code. Show quoted text

> It wouldn't surprise me if the tests are only passing > with that patch due to a lack of sufficient test > coverage, because the patch doesn't seem to > be doing anything in a more correct way (in particular, > the changes to the stringification overload don't > look like they should ever be triggered).

I backed out the stringification modification and all tests still pass. So the only interesting piece of the patch that matters is the addition of the '==' modification. I'm just hypothesizing that supplying '0+' and 'fallback' isn't enough to constrain the arguments of '==' to numify via the overloaded numification.

Tue Nov 23 13:00:22 2010 philip.monsen [...] gmail.com - Correspondence added

Just to follow up on the apparent lack of an appropriate '==' method in some cases, and that 'eq' is able to be autogenerated okay, I set 'fallback' to undef rather than to 1 (true), and quite a number of tests (I count 35) fail with 'Operation "==": no method found' exceptions. So, taking a step back, maybe the only issue is that '==' is just ambiguous enough with the supplied overloads that the significant use of it in Moose::Meta::TypeConstraint::equals() simply needs to force its arguments to numify. The patch then just becomes: --- Moose-1.06-orig/lib/Moose/Meta/TypeConstraint.pm 2010-06-01 13:49:54 -0500 +++ Moose-1.06/lib/Moose/Meta/TypeConstraint.pm 2010-11-23 11:50:56 -0600 @@ -135,7 +135,7 @@ my $other = Moose::Util::TypeConstraints::find_type_constraint($type_or_name) or return; - return 1 if $self == $other; + return 1 if 0+$self == 0+$other; if ( $self->has_hand_optimized_type_constraint and $other->has_hand_optimized_type_constraint ) { return 1 if $self->hand_optimized_type_constraint == $other->hand_optimized_type_constraint; This single change seems more in spirit with the previous v1.05 code and with the overloading changes, and also results in all tests passing. On Tue Nov 23 11:04:34 2010, ICERIDER wrote: Show quoted text

> So the only interesting piece of the patch that matters is the addition > of the '==' modification. I'm just hypothesizing that supplying '0+' > and 'fallback' isn't enough to constrain the arguments of '==' to numify > via the overloaded numification. >

Tue Nov 23 13:22:39 2010 philip.monsen [...] gmail.com - Correspondence added

Also, that one-line patch results in all tests passing with the latest Moose (1.19) in conjunction with the latest Class::MOP (1.11) with my perl build as well.

Tue Nov 23 13:26:28 2010 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #63000] Regression from Moose 1.05 to 1.06 on HP-UX 11.31
Date:	Tue, 23 Nov 2010 12:26:13 -0600 (CST)
To:	Philip Monsen via RT <bug-Moose [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Tue, 23 Nov 2010, Philip Monsen via RT wrote: Show quoted text

> - return 1 if $self == $other;

Can you add a warning to the num overloading sub to see if it gets called in this case? It _should_ be called, but if it's not, that would explain the issue. Show quoted text

> + return 1 if 0+$self == 0+$other;

And of course we would expect it to be called in this cas.e -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Wed Nov 24 12:04:47 2010 philip.monsen [...] gmail.com - Correspondence added

On Tue Nov 23 13:26:28 2010, autarch@urth.org wrote: Show quoted text

> Can you add a warning to the num overloading sub to see if it gets called > in this case? It _should_ be called, but if it's not, that would explain > the issue.

Turns out it always *is* called. This made getting to the bottom of things very, very interesting. However, in the end I believe I finally know the underlying issue. To better understand why forcing numification on that one numeric equality test seemed to make all the difference, I decided to compare results of bare '==' with results of forced numification then '=='. In both cases, the overloaded numification is invoked and works as expected. However, mysteriously, the bare '==' returns true when the refaddr() values are different, but '==' after forced numification returns false. "Huh?" That was perplexing. So I looked a little closer at the reference addresses and found that they are 64-bit addresses with high bit set, but stored under the hood as UVs. After playing around a while with various types of numeric operations, I found that under some conditions, this bit was being interpreted as a sign indicator and we were getting rounded floating point results instead of expected 64-bit integer results. This seemed to indicate that use integer; is needed in TypeConstraint.pm, at least if full-strength 64-bitness is in play. This wouldn't necessary guarantee results are "correct" in an abstract mathematical sense, but would seem to require that (at a minimum) numeric operators behave consistently with the numification overload in effect. Indeed, adding this single pragma at package scope, with no other changes to the stock code, returns all tests to PASS. Pragma scope can be restricted to just equals() with the same effect.

Thu Dec 02 12:53:00 2010 philip.monsen [...] gmail.com - Correspondence added

RT-Send-CC:

autarch [...] urth.org

I've attached an updated patch against Moose-1.21 that corrects the issue as per my further investigation described in my last post. Without this patch I still see the same issue manifesting itself in some invocations of Moose::Meta::TypeConstraint->equals(). The same basic patch corrects the issue on Moose-1.06 (and presumably all versions in between).

Subject:

TypeConstraint.pm-use-integer-patch

Download TypeConstraint.pm-use-integer-patch
application/octet-stream 1.7k

Message body not shown because it is not plain text.

Mon Mar 14 20:35:23 2011 DROLSKY [...] cpan.org - Correspondence added

On Thu Dec 02 12:53:00 2010, ICERIDER wrote: Show quoted text

> I've attached an updated patch against Moose-1.21 that corrects the > issue as per my further investigation described in my last post. > Without this patch I still see the same issue manifesting itself in some > invocations of Moose::Meta::TypeConstraint->equals(). The same basic > patch corrects the issue on Moose-1.06 (and presumably all versions in > between).

Sorry to be so slow to reply. This patch is really hard to understand. For some reason you changed the indentation of the entire sub, so the patch is basically "replace the whole sub".

Mon Mar 14 23:24:45 2011 philip.monsen [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #63000] Regression from Moose 1.05 to 1.06 on HP-UX 11.31
Date:	Mon, 14 Mar 2011 22:24:35 -0500
To:	bug-Moose [...] rt.cpan.org
From:	Philip Monsen <philip.monsen [...] gmail.com>

On Mon, Mar 14, 2011 at 7:35 PM, Dave Rolsky via RT <bug-Moose@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=63000 > > Sorry to be so slow to reply.

No worries. Sorry for obscuring my suggested fix (see below). Show quoted text

> This patch is really hard to understand. > For some reason you changed the indentation of the entire sub, so the > patch is basically "replace the whole sub".

In retrospect I wondered whether it (i.e. introducing an enclosing scope to the Moose::Meta::TypeConstraint::equals() method in which 'use integer' was active) was overkill. All I was really trying to accomplish was to scope the 'use integer;' pragma to the equals() method. In fact, on the version I patched and am using on my HP-UX system I inserted the pragma invocation within the method body, as: sub equals { my ( $self, $type_or_name ) = @_; use integer; # Force math ops to be consistent, avoid FP overflow ... } That would amount to a simple one-line patch (or two lines, if you include the additional blank line). I'd like to offer this up as an alternative.

Sat Jul 30 18:30:22 2011 DROLSKY [...] cpan.org - Reference to ticket #59478 added

Thu Feb 07 21:16:37 2013 cpan [...] prather.org - Correspondence added

On Mon Mar 14 23:24:45 2011, ICERIDER wrote: Show quoted text

> On Mon, Mar 14, 2011 at 7:35 PM, Dave Rolsky via RT > <bug-Moose@rt.cpan.org> wrote:

> > <URL: https://rt.cpan.org/Ticket/Display.html?id=63000 > > > Sorry to be so slow to reply.

> > No worries. Sorry for obscuring my suggested fix (see below). >

> > This patch is really hard to understand. > > For some reason you changed the indentation of the entire sub, so the > > patch is basically "replace the whole sub".

> > In retrospect I wondered whether it (i.e. introducing an enclosing > scope to the Moose::Meta::TypeConstraint::equals() method in which > 'use integer' was active) was overkill. All I was really trying to > accomplish was to scope the 'use integer;' pragma to the equals() > method. In fact, on the version I patched and am using on my HP-UX > system I inserted the pragma invocation within the method body, as: > > sub equals { > my ( $self, $type_or_name ) = @_; > > use integer; # Force math ops to be consistent, avoid FP overflow > > ... > } > > That would amount to a simple one-line patch (or two lines, if you > include the additional blank line). I'd like to offer this up as an > alternative.

Is this still an issue? Has an actual patch been produced? A branch in moose.git? I'm marking this stalled until we move forward on this. -Chris

Thu Feb 07 21:16:39 2013 cpan [...] prather.org - Status changed from 'open' to 'stalled'