Bug #68471 for Test-Spelling: Code tries to modify constants

Wed May 25 18:10:28 2011 SILASMONK [...] cpan.org - Ticket created

Subject:

Code tries to modify constants

Sometimes, in particular when using Build, I get the following error. Modification of a read-only value attempted at /usr/share/perl5/Test/Spelling.pm line 223, <DATA> line 1002. If I try to debug the code, the issue goes away. However I can see a very plausible patch, and I attach it. The bug was also reported to Debian as follows. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=627963

Subject:

dont_modify_constants.patch

--- a/lib/Test/Spelling.pm +++ b/lib/Test/Spelling.pm @@ -217,13 +217,9 @@ sub add_stopwords { for my $word (@_) { - # XXX: the processing this performs is to support "perl t/spell.t 2>> - # t/spell.t" which is bunk. in the near future the processing here will - # become more modern - $word =~ s/^#?\s*//; - $word =~ s/\s+$//; - next if $word =~ /\s/ or $word =~ /:/; - $Pod::Wordlist::Wordlist{$word} = 1; + if ($word =~ m{\A\#?\s*([^\s\:]+)\s*\z}xms) { + $Pod::Wordlist::Wordlist{$1} = 1; + } } }

Wed May 25 18:26:29 2011 rt.cpan [...] sartak.org - Correspondence added

On 2011-5月-25 水 18:10:28, SILASMONK wrote: Show quoted text

> Sometimes, in particular when using Build, I get the following error. > > Modification of a read-only value attempted at > /usr/share/perl5/Test/Spelling.pm line 223, <DATA> line 1002. > > If I try to debug the code, the issue goes away. However > I can see a very plausible patch, and I attach it. The bug was also > reported to Debian as follows. > > > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=627963

Hi Nicholas, Thanks for your bug report. I think I can make the patch a little bit simpler and still solve the issue. See attached. What do you think? If it solves your problem I'll release with it. Thanks, Shawn

Subject:

0001-Copy-each-stopword-to-avoid-modifying-readonly-value.patch

From 9224d06b71b309250a5b3d482d154ea494c556c6 Mon Sep 17 00:00:00 2001 From: Shawn M Moore <sartak@bestpractical.com> Date: Wed, 25 May 2011 18:23:09 -0400 Subject: [PATCH] Copy each stopword to avoid modifying readonly values This avoids "Modification of a read-only value attempted" errors when you use add_stopwords("constant", "strings", "here") This, similar to what we were doing, doesn't work: perl -e 'sub foo { for my $x (@_) { $x =~ s/./X/; print $x } } foo("hello")' This, what we do as of this commit, does: perl -e 'sub foo { for (@_) { my $x = $_; $x =~ s/./X/; print $x } } foo("hello")' --- lib/Test/Spelling.pm | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/lib/Test/Spelling.pm b/lib/Test/Spelling.pm index e134633..0c32f8d 100644 --- a/lib/Test/Spelling.pm +++ b/lib/Test/Spelling.pm @@ -216,7 +216,10 @@ sub _is_perl { } sub add_stopwords { - for my $word (@_) { + for (@_) { + # explicit copy so we don't modify constants as in add_stopwords("SQLite") + my $word = $_; + # XXX: the processing this performs is to support "perl t/spell.t 2>> # t/spell.t" which is bunk. in the near future the processing here will # become more modern -- 1.7.5.1

Wed May 25 18:26:30 2011 The RT System itself - Status changed from 'new' to 'open'

Thu May 26 02:49:44 2011 SILASMONK [...] cpan.org - Correspondence added

On Wed May 25 18:26:29 2011, SARTAK wrote: Show quoted text

> On 2011-5月-25 水 18:10:28, SILASMONK wrote:

> > Sometimes, in particular when using Build, I get the following error. > > > > Modification of a read-only value attempted at > > /usr/share/perl5/Test/Spelling.pm line 223, <DATA> line 1002. > > > > If I try to debug the code, the issue goes away. However > > I can see a very plausible patch, and I attach it. The bug was also > > reported to Debian as follows. > > > > > > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=627963

> > Hi Nicholas, > > Thanks for your bug report. I think I can make the patch a little bit > simpler and still solve the issue. See attached. What do you think? If > it solves your problem I'll release with it. > > Thanks, > Shawn

Shawn, It appears to work but I am not very happy with it. 1.) Knowing which one works seems to require a very good understanding of the semantics of $_. 2.) It is a reversion to earlier code so what is to stop another flip-flop? 3.) My proposal does NOT modify $word, so unambiguously works. 4.) There is a comment in the code promising something more modern. Is this not a great time to deliver? Also it would be great if the next version could have some decent tests.

Thu May 26 13:23:34 2011 rt.cpan [...] sartak.org - Correspondence added

On 2011-5月-26 木 02:49:44, SILASMONK wrote: Show quoted text

> Shawn, > It appears to work but I am not very happy with it. > 1.) Knowing which one works seems to require a very good understanding > of the semantics of $_. > 2.) It is a reversion to earlier code so what is to stop another

flip-flop? Show quoted text

> 3.) My proposal does NOT modify $word, so unambiguously works. > 4.) There is a comment in the code promising something more modern. Is > this not a great time to deliver? > > > Also it would be great if the next version could have some decent tests.

I understand your concerns and agree with most of them. However, the lack of tests is what drives me to produce the minimal patch that is obviously correct, as opposed to the more complicated rewrite in the proposed patch. The original code written by Ivan is what worked, and because we didn't have tests, it sadly broke in a refactor. I don't want it to happen again, so tests are pretty high up on the priority list now. I'm not at all opposed to doing #4, modernizing the add_stopwords function. Or perhaps the modern version should be a new function, along the lines of add_structured_stopwords. That way we don't break everyone's code yet again, and we keep the unusual "perl t/spell.t 2>> t/spell.t" behavior that was the intent of add_stopwords's parsing. Shawn

Thu May 26 18:24:13 2011 nicholas [...] periapt.co.uk - Correspondence added

CC:	SILASMONK [...] cpan.org
Subject:	Re: [rt.cpan.org #68471] Code tries to modify constants
Date:	Thu, 26 May 2011 23:24:48 +0100
To:	bug-Test-Spelling [...] rt.cpan.org
From:	Nicholas Bamber <nicholas [...] periapt.co.uk>

Shawn, The patch is ready to go in Debian and I am confident that although it is a different style is a perfectly good solution. So if you want to take your time we'll put the patch in and remove on the next release. On the other if you want to put in a conservative release immediately, and address the bigger issues later, then we will wait for that immediate release. You have actually been very responsive and I thank you for that. Nicholas On 26/05/11 18:23, Shawn M Moore via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=68471 > > > On 2011-5月-26 木 02:49:44, SILASMONK wrote:

>> Shawn, >> It appears to work but I am not very happy with it. >> 1.) Knowing which one works seems to require a very good understanding >> of the semantics of $_. >> 2.) It is a reversion to earlier code so what is to stop another

> flip-flop?

>> 3.) My proposal does NOT modify $word, so unambiguously works. >> 4.) There is a comment in the code promising something more modern. Is >> this not a great time to deliver? >> >> >> Also it would be great if the next version could have some decent tests.

> > I understand your concerns and agree with most of them. However, the > lack of tests is what drives me to produce the minimal patch that is > obviously correct, as opposed to the more complicated rewrite in the > proposed patch. The original code written by Ivan is what worked, and > because we didn't have tests, it sadly broke in a refactor. I don't want > it to happen again, so tests are pretty high up on the priority list now. > > I'm not at all opposed to doing #4, modernizing the add_stopwords > function. Or perhaps the modern version should be a new function, along > the lines of add_structured_stopwords. That way we don't break > everyone's code yet again, and we keep the unusual "perl t/spell.t 2>> > t/spell.t" behavior that was the intent of add_stopwords's parsing. > > Shawn >

-- Nicholas Bamber | http://www.periapt.co.uk/ PGP key 3BFFE73C from pgp.mit.edu

Download signature.asc
application/pgp-signature 900b

Message body not shown because it is not plain text.

Fri May 27 14:24:51 2011 rt.cpan [...] sartak.org - Reference to http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=627963 added

Fri May 27 14:26:36 2011 rt.cpan [...] sartak.org - Correspondence added

On 2011-5月-26 木 18:24:13, nicholas@periapt.co.uk wrote: Show quoted text

> Shawn, > The patch is ready to go in Debian and I am confident that although it > is a different style is a perfectly good solution. So if you want to > take your time we'll put the patch in and remove on the next release. On > the other if you want to put in a conservative release immediately, and > address the bigger issues later, then we will wait for that immediate > release. > You have actually been very responsive and I thank you for that. > > Nicholas

Hi Nicholas, I'm preparing the 0.14 release now with just the simple bugfix patch. For the next release, we'll shoot for better parsing and tests. Thanks for being understanding and diligent. :) Shawn

Thu Jun 16 17:51:08 2011 rt.cpan [...] sartak.org - Status changed from 'open' to 'resolved'

Thu Jun 16 17:52:05 2011 rt.cpan [...] sartak.org - Reference by ticket #68881 added