Re: [PATCH] New force_package_version uses scan_version
David Golden
If patches are a problem, you can also fork perl on github and publish
a branch there. My github perl repo gets updated from the master perl
git server every 5 minutes or so. http://github.com/dagolden/perl.git
-- David
On Mon, Jan 4, 2010 at 8:27 PM, John Peacock
<john.p...@adatshalom.net> wrote:
> Another attempt; apparently my original emails have been eaten by
> SpamAssassin, so I'll gzip the actual patch and hope I can get it through
> this time...
>
> John
>
> -----------------------------------------------------------------
>
> Instead of using a custom tokenizer to create an old-style dualvar
> version, use the version object code's scan_version, and produce
> a version object directly. This is only called by
>
> package name version;
>
> and requires that scan_version be made more strict about legal
> version formats.
>
> Check that all variations of legal and illegal strict version
> formats either pass or fail, as appropriate, and with the
> expected warning thrown.
> ---
> embed.fnc | 3 +
> embed.h | 4 +
> global.sym | 1 +
> handy.h | 3 +
> lib/version.pm | 122 ++++++++++++++++++++++++-
> proto.h | 10 ++
> t/comp/package.t | 34 +-------
> t/comp/packagev.t | 50 ++++++++++
> t/porting/diag.t | 6 +
> toke.c | 50 ++++++++++-
> util.c | 264
> ++++++++++++++++++++++++++++++++++++++++++-----------
> 11 files changed, 456 insertions(+), 91 deletions(-)
> create mode 100644 t/comp/packagev.t
>
>
David Golden
I've had a chance to read the code but not really play with it or test
it, but I'm very, very short on bandwidth so wanted to get you some
feedback sooner rather than waiting until I can do more with it. For
the same reason, I apologize for providing just text feedback and not
patches.
Overall -- I think is on the right track, particularly because it
appears to be completely independent of how version numbers are parsed
for "use NAME VERSION", which was the approach we all discussed for
Perl 5.12. Though I think I found a few minor bugs, in general I
don't think this will break anyone's existing code.
Before I get into my list of nits, I want to play back my
understanding of the various code paths for version number strings.
(1) "use NAME VERSION" -- the version part is parsed by
S_force_version (which is unchanged by John's patch), which I think
results in a dual-var
(2) "package NAME VERSION" -- the version parsed by this calling chain:
* S_force_package_version
- calls Perl_prescan_version (in 'strict' mode)
- calls Perl_scan_version
* calls Perl_prescan_version (in 'lax' mode)
* constructs a version object
(3) version->new, qv(), and UNIVERSAL::VERSION all call either
Perl_new_version or Perl_upg_version. Perl_new_version itself calls
Perl_upg_version. Perl_upg_version calls Perl_scan_version, which
follows the chain above in (2) to result in a version object
This means that (1) is unchanged, (2) is strict and (3) is lax --
albeit subject to the new, more consistent "lax" rules. It also means
that "package NAME VERSION" assigns a version object to $NAME::VERSION
rather than a strict, but I don't think that's a big deal. It might
even be smart, since any naive comparisons will get properly
overloaded operators.
What bothers, however, is that in (2), above, prescan_version gets
called twice, once in strict mode (by S_force_package_version) and
again in lax mode (by Perl_scan_version). If the idea is that
eventually in Perl 5.14 or 5.16, Perl_scan_version will switch from
calling Perl_prescan_version in lax mode to strict mode and all three
paths will route through Perl_scan_version, then this is just a
temporary issues and I can live with it.
Separately, I'd like to see a major change to the testing. I like
that John has split out a new file for version number testing, but
what I would like to see is for each test string to have a "pass/fail"
for each of strict and lax modes. If either fails, the error string
should be the same. E.g.
v1.2 fail pass Invalid version format...
v1.2.3 pass pass
Each string should be tested against the strict & lax regexes in
version.pm. Strictness should also be tested with 'package NAME
VERSION' and laxness should be tested with version->new. I suspect
that laxness should also be tested against "use NAME VERSION", except
those rules are yet different and might need a new fail/pass column
just for that. In summary -- I'd like to have a really good list of
candidate version strings and a test that actually shows what we
expect each of the three paths above to do with them. That will make
it *much* easier to track down bugs and catch regressions in the
future.
Now, on to the list of nits/issues.
* I don't like the name 'S_force_package_version'. I'd rather call it
'S_force_strict_version' since that describes its effect rather than
how it happens to be used
* Rather than defining is_VERSION(a,b) as an alias to
Perl_prescan_version, I'd rather have an explicit is_STRICT_VERSION(a)
as an alias that sets the strict flag and is_LAX_VERSION(a) as an
alias that clear the strict flag. Otherwise is_VERSION(s, TRUE) has
an obscure extra TRUE that is inscrutable unless knows how the macro
is defined.
* In the version.pm strict/lax regular expressions, there are several
repeated sections. (e.g. non-leading-zero integer). I'd would be
easier to see the differences between regexes if those common parts
could be refactored out.
* In the lax regexes, the trailing alpha stanza has a star (*)
quantifier and I think it should have a question mark(?)
* I think I found two test cases that Perl_prescan_version doesn't
handle well -- though I admit I didn't actually test them: v.2.3.4
and v1.2345.6
* For the error messages, I have mixed feelings about the phrase
"Invalid strict version format" as it might mislead people to thing it
has something to do with strict.pm. I think just "Invalid version
format" is fine.
* I think the error message "(v1.2.3 required)" should be
"(dotted-decimal versions need at least three parts)"
* S_force_package_version is commented with copy/paste from
S_force_version that mentions "guessing" but has no such parameter, so
that comment needs to be re-written
* Several of the errors found in Perl_prescan_version warn with
Perl_ck_warner and others die with Perl_croak. I don't understand
why. Also, the actual error output looks pretty clumsy. I think we
need better diagnostic output. E.g.
$ ./perl -wE 'package Foo v1'
Invalid strict version format (v1.2.3 required) at -e line 1.
syntax error at -e line 1, near "package Foo "
syntax error at -e line 1, near "package Foo v1"
Execution of -e aborted due to compilation errors.
I'm sure there's a way to avoid those repeated "syntax error" messages.
* strict dotted-decimal section needs to check that there is at least
one digit after 'v' before "while (isDIGIT(*d)) d++". (e.g. "package
Foo v.1.2.3" is an error but the message isn't as clear as it should
be).
* I think the strict dotted-decimal section for "max 3 digits between
decimals" is breaking on j=3 when it should instead be croaking with
an error when j>3 (e.g. "package Foo v1.2345.6" succeeds when it
should be an error)
* very minor nit -- rather than "goto version_saw_decimal", I think it
would be clearer as "goto decimal_mantissa"
* I don't understand what's going on with width and *swidth, but I'm
going to guess it's part of the legacy API so we just stick with it.
* I don't understand the point of adding HvSHAREKEYS_on(hv), so I
can't comment one way or the other about it.
I hope these comments help. Generally, I'd like to see revised
testing and some of the nits addressed and then have the patch applied
for 5.12. I think it's a nice step forward towards formalizing the
behaviors and laying the groundwork for a consistent code path for
dealing with version numbers.
-- David
John Peacock
> I hope these comments help. Generally, I'd like to see revised
> testing and some of the nits addressed and then have the patch applied
> for 5.12. I think it's a nice step forward towards formalizing the
> behaviors and laying the groundwork for a consistent code path for
> dealing with version numbers.
Rather than a point-by-point review, I'll say YES instead. I agree with
all of your nits and I will revise the patch as soon as I can to use
your very fine suggestions. A couple of specific comments (in no
particular order):
> * In the lax regexes, the trailing alpha stanza has a star (*)
> quantifier and I think it should have a question mark(?)
That was by design, to eventually allow multiple underscores in alpha
versions, raised by a recent discussion on the M::B and the bug here:
https://rt.cpan.org/Ticket/Display.html?id=50841
It was caused by an apparently bad integration script at ActiveState and
was only supposed to be temporary, but I have code that doesn't fit in
the margin of this e-mail to handle at a least some forms of repeated
alpha blocks. Ideally, I don't want to change either of these regexes
once published (modulo your suggested reduction of duplicated sections).
I've already pushed version-0.79 to CPAN, so I'm going to have to
break that promise already, but such is life... ;-)
> * Several of the errors found in Perl_prescan_version warn with
> Perl_ck_warner and others die with Perl_croak. I don't understand
> why. Also, the actual error output looks pretty clumsy. I think we
> need better diagnostic output. E.g.
>
> $ ./perl -wE 'package Foo v1'
> Invalid strict version format (v1.2.3 required) at -e line 1.
> syntax error at -e line 1, near "package Foo "
> syntax error at -e line 1, near "package Foo v1"
> Execution of -e aborted due to compilation errors.
>
> I'm sure there's a way to avoid those repeated "syntax error" messages.
This was actually the hardest part to deal with in the code. The issue
is that prescan_version can be called both at compile time and at run
time. In the former case, we want/need both the advisory-level errors
from prescan_version, as well as the fatal error from the tokenizer (the
'syntax error' part in your example). I tried very hard _not_ to have
more than one 'syntax error' line, but I must have missed one.
If prescan_version is going to croak for all exceptions, then I would
need to be able to provide that second line as well, which includes
information that is harder to come by. The tokenizer routines, like
scan_number and now prescan_version, really only have their own context
(the current pointer into the code, e.g. 's'), and don't have the higher
level information about what has been tokenized already (e.g. what line
number we are currently on).
In the end, it was much easier to just have prescan_version warn and
fail to produce a new token (which would cause the wrapping code to
throw the 'syntax error at ...' croak). I can revisit that to see if I
can provide the same level of diagnostics completely isolated within
prescan_version itself (probably as yet another macro).
As to why some are ck_warner and some are croak - if you look at the
code, the only time ck_warner is called is in the strict case; all of
the other code paths are the run-time croaks that were formerly part of
scan_version itself. If I can make it work as described above, all
failures in parsing version objects can throw errors using a common macro.
> * For the error messages, I have mixed feelings about the phrase
> "Invalid strict version format" as it might mislead people to thing it
> has something to do with strict.pm. I think just "Invalid version
> format" is fine.
I also had mixed feelings, but I thought it was important to call out
the two different types of errors:
1) basic failures in providing appropriate input to the version
assignment (e.g. $VERSION = "99 and 44/100 percent pure");
2) the higher level failures to follow the STRICT_VERSION semantics.
If, as we have discussed, in v5.14.x and beyond, we provide a way to
turn on the strict semantics for other code paths, it would still be
useful to see which type of failure mode we were griping about. That
being said, I'm perfectly happy to take out the "strict" now.
John
David Golden
<john.p...@havurah-software.org> wrote:
>> * In the lax regexes, the trailing alpha stanza has a star (*)
>> quantifier and I think it should have a question mark(?)
>
> That was by design, to eventually allow multiple underscores in alpha
> versions, raised by a recent discussion on the M::B and the bug here:
>
> https://rt.cpan.org/Ticket/Display.html?id=50841
>
> It was caused by an apparently bad integration script at ActiveState and was
> only supposed to be temporary, but I have code that doesn't fit in the
> margin of this e-mail to handle at a least some forms of repeated alpha
> blocks. Ideally, I don't want to change either of these regexes once
> published (modulo your suggested reduction of duplicated sections). I've
> already pushed version-0.79 to CPAN, so I'm going to have to break that
> promise already, but such is life... ;-)
If we want to say that multiple alpha stanzas are OK (to prevent that
kind of issue in the future), then Perl_prescan_version needs to be
modified to match. I don't care if we allow multiple alpha stanzas in
lax mode, but I do want the regexes to be consistent with the
character-by-character parsing.
>> I'm sure there's a way to avoid those repeated "syntax error" messages.
>
> This was actually the hardest part to deal with in the code. The issue is
> that prescan_version can be called both at compile time and at run time. In
> the former case, we want/need both the advisory-level errors from
> prescan_version, as well as the fatal error from the tokenizer (the 'syntax
> error' part in your example). I tried very hard _not_ to have more than one
> 'syntax error' line, but I must have missed one.
I'm getting out of my C comfort zone here, but what if we add an
&error_msg parameter to Perl_prescan_version and populate that and
return on error rather than warning or dying? Then
S_force_package_version or Perl_scan_version can each die in a
context-appropriate way with that error message. You might still
need to change the isVERSION (or is_STRICT_VERSION) macro to also
return the error, but I think that approach might work better than
what we have now.
-- David