Bug #92438 for version: version->new(undef, '1.2') version::new(undef, '1.2') questionable behavior

Fri Jan 24 10:42:29 2014 BULKDD [...] cpan.org - Ticket created

Subject:

version->new(undef, '1.2') version::new(undef, '1.2') questionable behavior

I was trying to clean up issues mentioned in https://rt.perl.org/Ticket/Display.html?id=115660#txn-1169402 by me, I read http://search.cpan.org/~jpeacock/version-0.9907/lib/version/Internals.pod#Object_Methods but I didn't see it specifying what will happen in the 1st example below. Currently the 1st example below complicates the new XS code I'm working on, logic tree wise. I've attached a patch of what I have so far, but none of the examples below were run using it. Also note the last example doesn't have a prefixed "v". ------------------------------------------------------ C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version->new(undef, 1.2)" 0 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version->new('rev', 1.2)" v1.2 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version->new(qw!rev 1.2!)" v1.2 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version->new(qw!1.2!)" 1.2 C:\Documents and Settings\Owner\Desktop\cpan libs\version> ------------------------------------------------------ Also here is another edge case I have no clue on what should happen. Notice the XS vs PP difference, 1 fatal errors, the other doesn't. ------------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version->new('rev', 'v1.2')" Invalid version format (dotted-decimal versions require at least three parts) at -e line 1. C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version->new(undef, 'v1.2')" 0 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " say version::vpp->new('rev', '1.2')" Use of "goto" to jump into a construct is deprecated at C:/perl519/site/lib/vers ion/vpp.pm line 258. v1.2 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " say version::vpp->new(undef, '1.2')" 0 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " say version::vpp->new(undef, 'v1.2')" 0 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " say version::vpp->new('rev', 'v1.2')" v1.2 C:\Documents and Settings\Owner\Desktop\cpan libs\version> ------------------------------------------------------- Some more PP vs XS differences regarding passing garbage class names (AKA undef), ------------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version::new('rev', '1.2')" rev=HASH(0x369c94) C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version::new(undef, '1.2')" main=HASH(0x369c94) C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -E" say version::new(undef, undef, '1.2')" main=HASH(0x369cc4) C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " say version::vpp::new(undef, undef, '1.2')" Usage: version::new(class, version) at -e line 1. C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " say version::vpp::new(undef, '1.2')" Usage: version::new(class, version) at -e line 1. C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " say version::vpp::new('rev', '1.2')" rev=HASH(0x369024) C:\Documents and Settings\Owner\Desktop\cpan libs\version> -----------------------------------------------------------------------

Subject:

0002-WIP-version_new.patch

From d4a043abe2e939ddc092f168e28fbf90074ce73d Mon Sep 17 00:00:00 2001 From: bulk88 <bulk88@hotmail.com> Date: Fri, 24 Jan 2014 10:23:16 -0500 Subject: [PATCH 2/2] WIP version_new --- vutil/vxs.inc | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/vutil/vxs.inc b/vutil/vxs.inc index 0a02056..bef2d72 100644 --- a/vutil/vxs.inc +++ b/vutil/vxs.inc @@ -173,7 +173,7 @@ VXS(version_new) { dVAR; dXSARGS; - SV *vs = items ? ST(1) : &PL_sv_undef; + SV *vs; SV *rv; const char * classname = ""; STRLEN len; @@ -186,8 +186,13 @@ VXS(version_new) if (items > 3 || items == 0) Perl_croak(aTHX_ "Usage: version::new(class, version)"); + if(items >= 2) { + vs = ST(1); /* Just in case this is something like a tied hash */ - SvGETMAGIC(vs); + SvGETMAGIC(vs); + } else { + vs =&PL_sv_undef; /* dont do SvOK on this! use a goto instead???? */ + } if ( items == 1 || ! SvOK(vs) ) { /* no param or explicit undef */ /* create empty object */ -- 1.7.9.msysgit.0

Sat Jan 25 07:46:50 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92438] version->new(undef, '1.2') version::new(undef, '1.2') questionable behavior
Date:	Sat, 25 Jan 2014 07:46:34 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 01/24/2014 10:42 AM, Daniel Dragan via RT wrote: Show quoted text

> Fri Jan 24 10:42:29 2014: Request 92438 was acted upon. > Transaction: Ticket created by BULKDD > Queue: version > Subject: version->new(undef, '1.2') version::new(undef, '1.2') > questionable behavior > Broken in: 0.9907 > Severity: (no value) > Owner: Nobody > Requestors: BULKDD@cpan.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=92438 > > > > I was trying to clean up issues mentioned in https://rt.perl.org/Ticket/Display.html?id=115660#txn-1169402 by me, I read http://search.cpan.org/~jpeacock/version-0.9907/lib/version/Internals.pod#Object_Methods but I didn't see it specifying what will happen in the 1st example below. Currently the 1st example below complicates the new XS code I'm working on, logic tree wise. I've attached a patch of what I have so far, but none of the examples below were run using it. Also note the last example doesn't have a prefixed "v". >

All of your contrived examples are unsupported by the API as specified in version::Internals. The only things that version->new() and ilk promise to support are: 1) a single scalar value that can be used to create a version object which may be an NV, an IV, a string that looks like a number or a v-string (magical or otherwise); 2) an array of values consisting of the string literal 'Revision:' followed by a string that looks like a number (which will be interpreted as if it had a leading 'v'). The second form is only documented in version::Internals because that is an obsolete form used with CVS which I may be the only person to ever use. I wish I had deprecated that form long ago, but I have no idea who might be using it. FWIW, I abandoned CVS 10 1/2 years ago for Subversion (and then later Mercurial). But the immediate problem is that the code as it stands today doesn't do a good job of restricting to that documented API. This is both because there are some edge cases (which I will discuss next) and because I could never consider myself to be an XS master, nor do I have a deep understanding of the Perl core macros, many of which are under- or completely un-documented. The code in new() attempts (and sometimes fails) to distinguish between the following cases: A. items==3 (the CVS case only) ($Class, 'Revision:', $version) which we could easily validate, i.e. we should actively check the second element before processing the third as a qv number; note that in this case, we can assume that $version is a string that looks like a number. B. items==2 This could be either: i. ($Class, $version) - the classic version->new($version) case; this could also be $vobj->new($version) where $vobj is an object in a class implementing the version API; or ii. ('Revision:', $version) - which would be the far less likely functional call version::new(qw$Revision: 1.2); I pretty much ignored this possibility. C. items==1 This could be either: i. ($Class) - the unusual version->new() with an undef initializer; I don't know why someone would do this, but it is allowed or ii. ($version) - this is the functional call version::new($version) or more commonly version::qv($number). D. items==0 This can only be the functional call version::new() with no initializer at all. So the difficult case of items==1 is the only place where we have to use some heuristics to determine which of those two scenarios is the case. I wish that, at least in C, there was a way to cleanly distinguish between being called as an object or class METHOD as opposed to a fully qualified FUNCTIONAL call. I would happily forbid the functional calls altogether, but I only know how to do that in vpp, and besides, the existing core implementation in 5.10.0 onwards wouldn't have that limitation. Sorry for the long discussion, but I thought it would be better to document this (and my thinking) rather that dither about implementation questions back and forth. Thanks for helping me improve this code! I really do appreciate it. John

Sat Jan 25 07:46:51 2014 The RT System itself - Status changed from 'new' to 'open'

Sun Jan 26 02:42:30 2014 BULKDD [...] cpan.org - Correspondence added

On Sat Jan 25 07:46:50 2014, john.peacock@havurah-software.org wrote: Show quoted text

> > All of your contrived examples are unsupported by the API as specified > in version::Internals. The only things that version->new() and ilk > promise to support are: > > 1) a single scalar value that can be used to create a version object > which may be an NV, an IV, a string that looks like a number or a > v-string (magical or otherwise);

Ok. No debate. Show quoted text

> > 2) an array of values consisting of the string literal 'Revision:' > followed by a string that looks like a number (which will be > interpreted > as if it had a leading 'v'). >

Must the array have 2 elements or atleast 2 elements and further are ignored? ----------------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " $VERSION = version::vpp->new(qw$Revision: 1.4 foo$); print $VERSION" Invalid version format (non-numeric data) at -e line 1. C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " $VERSION = version->new(qw$Revision: 1.4 foo$); print $VERSION" Usage: version::new(class, version) at -e line 1. C:\Documents and Settings\Owner\Desktop\cpan libs\version> ----------------------------------------------------------- Show quoted text

> The second form is only documented in version::Internals because that > is > an obsolete form used with CVS which I may be the only person to ever > use. I wish I had deprecated that form long ago, but I have no idea > who > might be using it. FWIW, I abandoned CVS 10 1/2 years ago for > Subversion (and then later Mercurial).

There is a chunk of perl code that syntax errors in version::Internals, the qw has no terminator -------------------------------------------------------------- If you use a mathematic formula that resolves to a floating point number, you are dependent on Perl's conversion routines to yield the version you expect. You are pretty safe by dividing by a power of 10, for example, but other operations are not likely to be what you intend. For example: $VERSION = version->new((qw$Revision: 1.4)[1]/10); print $VERSION; # yields 0.14 $V2 = version->new(100/9); # Integer overflow in decimal number print $V2; # yields something like 11.111.111.100 Perl 5.8.1 and beyond are able to automatically quote v-strings but that is not possible in earlier versions of Perl. In other words: ---------------------------------------------------------------- Show quoted text

> > But the immediate problem is that the code as it stands today doesn't > do > a good job of restricting to that documented API. This is both > because > there are some edge cases (which I will discuss next) and because I > could never consider myself to be an XS master, nor do I have a deep > understanding of the Perl core macros, many of which are under- or > completely un-documented. > > The code in new() attempts (and sometimes fails) to distinguish > between > the following cases: > > A. items==3 (the CVS case only) > ($Class, 'Revision:', $version) which we could easily validate, > i.e. > we should actively check the second element before processing the > third > as a qv number; note that in this case, we can assume that $version > is a > string that looks like a number.

"Revision:" is not checked in ::vpp ATM. You are asking for a feature request. ----------------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " $VERSION = version::vpp->new(qw$Revision: 1.4$); print $VERSION" Use of "goto" to jump into a construct is deprecated at C:/perl519/site/lib/vers ion/vpp.pm line 258. v1.4 C:\Documents and Settings\Owner\Desktop\cpan libs\version> ----------------------------------------------------------- Show quoted text

> > B. items==2 > This could be either: > > i. ($Class, $version) - the classic version->new($version) case; > this could also be $vobj->new($version) where $vobj is an object in a > class implementing the version API;

This works. --------------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " $VERSION = version->new(1.4); print $VERSION->new(1.5);" 1.5 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " $VERSION = version::vpp->new(1.4); print $VERSION->new(1.5);" 1.5 C:\Documents and Settings\Owner\Desktop\cpan libs\version> --------------------------------------------------------- Show quoted text

> > or ii. ('Revision:', $version) - which would be the far less likely > functional call version::new(qw$Revision: 1.2); I pretty much ignored > this possibility.

This is a feature request since it doesn't work. ----------------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " $VERSION = version::new(version->new(1.4)); print $VERSION" 0 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion::vpp -E " $VERSION = version::vpp::new(version->new(1.4)); print $VERSION" 0 ----------------------------------------------------------- Show quoted text

> > C. items==1 > This could be either: > > i. ($Class) - the unusual version->new() with an undef initializer; > I don't know why someone would do this, but it is allowed > > or ii. ($version) - this is the functional call version::new($version) > or more commonly version::qv($number). > > D. items==0 > > This can only be the functional call version::new() with no > initializer at all. > > So the difficult case of items==1 is the only place where we have to > use > some heuristics to determine which of those two scenarios is the case. > I wish that, at least in C, there was a way to cleanly distinguish > between being called as an object or class METHOD as opposed to a > fully > qualified FUNCTIONAL call. I would happily forbid the functional > calls > altogether, but I only know how to do that in vpp, and besides, the

I don't know how to do that in PP, and I dont really see where it is done in vpp's new my $self = bless ({}, ref ($class) || $class); ????? or if ( ref($value) && eval('$value->isa("version")') ) {???

Mon Jan 27 23:18:40 2014 BULKDD [...] cpan.org - Correspondence added

2 commits https://github.com/bulk88/version/commit/99b245e4f50ad2e2898b905e9e6bdea8ab473f33 I added a difference between the XS new and PP new, since the XS version doesn't check $_[1] for defined-ness anymore before going to 3 arg CVS mode anymore if there are 3 args. There is also a double get magic fix, since sv_isobject calls getmagic once on the classname SV. So the SvPV later should be of no magic type. --------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -Mversi on::vpp -E" say version->new(undef, '1.2')" v1.2 C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -Mversi on::vpp -E" say version::vpp->new(undef, '1.2')" 0 C:\Documents and Settings\Owner\Desktop\cpan libs\version> ---------------------------------------

Tue Jan 28 09:09:11 2014 jpeacock [...] cpan.org - Correspondence added

On Mon Jan 27 23:18:40 2014, BULKDD wrote: Show quoted text

> 2 commits > https://github.com/bulk88/version/commit/99b245e4f50ad2e2898b905e9e6bdea8ab473f33

Thanks applied. I will fix the resulting discrepency in vpp and push that as well. John

Sat Feb 01 09:01:22 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92438] version->new(undef, '1.2') version::new(undef, '1.2') questionable behavior
Date:	Sat, 01 Feb 2014 09:01:08 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 01/27/2014 11:18 PM, Daniel Dragan via RT wrote: Show quoted text

> C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -Mversi > on::vpp -E" say version::vpp->new(undef, '1.2')" > 0

I pushed a fix to version::vpp so that also prints v1.2 (plus some whitespace cleanup). If you are happy with what you've already done, I can prepare and release 0.9908 to CPAN and submit a patch to blead. John

Sat Feb 01 18:38:00 2014 jpeacock [...] cpan.org - Correspondence added

Fixed in 0.9908

Sat Feb 01 18:38:01 2014 jpeacock [...] cpan.org - Status changed from 'open' to 'resolved'