Skip Menu |
Report information
Id: 126955
Status: open
Priority: 0/
Queue: perl6

Owner: Nobody
Requestors: zefram [at] fysh.org
Cc:
AdminCc:

Severity: (no value)
Tag: testneeded
Platform: (no value)
Patch Status: (no value)
VM: (no value)



To: rakudobug [...] perl.org
From: Zefram <zefram [...] fysh.org>
Date: Fri, 18 Dec 2015 07:21:27 +0000
Subject: [BUG] more .perl string quoting problems
Download (untitled) / with headers
text/plain 373b
The roles CompUnit::Repository::Locally and IO::Local each have a bug similar to [perl #126935], where a .perl method rolls its own string quoting to inadequate effect. (Version also rolls its own string quoting, but the string content is sufficiently restricted that it works.) In each case, delegating to .perl on the Str value would produce a correct result. -zefram
From: Elizabeth Mattijsen <liz [...] dijkmat.nl>
To: "Zefram (via RT)" <perl6-bugs-followup [...] perl.org>
Date: Fri, 18 Dec 2015 11:41:04 +0100
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
Download (untitled) / with headers
text/plain 847b
Show quoted text
> On 18 Dec 2015, at 08:21, Zefram (via RT) <perl6-bugs-followup@perl.org> wrote: > > # New Ticket Created by Zefram > # Please include the string: [perl #126955] > # in the subject line of all future correspondence about this issue. > # <URL: https://rt.perl.org/Ticket/Display.html?id=126955 > > > > The roles CompUnit::Repository::Locally and IO::Local each have a bug > similar to [perl #126935], where a .perl method rolls its own string > quoting to inadequate effect. (Version also rolls its own string quoting, > but the string content is sufficiently restricted that it works.) In each > case, delegating to .perl on the Str value would produce a correct result.
Fixed with ae8d9809c432f071643384 , tests still needed. Zefram: I assume you checked all .perl methods in core for this, or should I look at them some more? Liz
From: Zefram <zefram [...] fysh.org>
To: Elizabeth Mattijsen via RT <perl6-bugs-followup [...] perl.org>
Date: Sun, 20 Dec 2015 22:18:03 +0000
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
Download (untitled) / with headers
text/plain 810b
Elizabeth Mattijsen via RT wrote: Show quoted text
>Fixed with ae8d9809c432f071643384
It's good that you're now factoring out the escaping code. This means that these methods (including for IO::Path) are now correct. But the factoring is still flawed, in that you've inlined what remains of the body of Str.perl into each of these .perl methods. This inlining doesn't achieve anything; it would be much more sensible for them to call .perl on their Str values. In the case of IO::Path you also have some gratuitous escaping of pipe characters, a relic of former versions of the code that used that as the delimiter. Show quoted text
>Zefram: I assume you checked all .perl methods in core for this, or >should I look at them some more?
I looked systematically through rakudo/src, and the ones I mentioned are all that I found. -zefram
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
Date: Mon, 21 Dec 2015 14:33:41 +0100
From: Elizabeth Mattijsen <liz [...] dijkmat.nl>
To: Elizabeth Mattijsen via RT <perl6-bugs-followup [...] perl.org>
Show quoted text
> On 20 Dec 2015, at 23:18, Zefram <zefram@fysh.org> wrote: > > Elizabeth Mattijsen via RT wrote:
>> Fixed with ae8d9809c432f071643384
> > It's good that you're now factoring out the escaping code. This means > that these methods (including for IO::Path) are now correct. But the > factoring is still flawed, in that you've inlined what remains of > the body of Str.perl into each of these .perl methods. This inlining > doesn't achieve anything; it would be much more sensible for them to > call .perl on their Str values.
Good point. Followed your suggestion in 8ddfff5533d4d77dbd7da65 . Show quoted text
> In the case of IO::Path you also have > some gratuitous escaping of pipe characters, a relic of former versions > of the code that used that as the delimiter.
Another good point, fixed in 8245d9072177f26251f128 Show quoted text
>> Zefram: I assume you checked all .perl methods in core for this, or >> should I look at them some more?
> I looked systematically through rakudo/src, and the ones I mentioned > are all that I found.
Thank you! Liz
From: Zefram <zefram [...] fysh.org>
To: Elizabeth Mattijsen via RT <perl6-bugs-followup [...] perl.org>
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
Date: Mon, 21 Dec 2015 15:58:47 +0000
Download (untitled) / with headers
text/plain 257b
Elizabeth Mattijsen via RT wrote: Show quoted text
>Good point. Followed your suggestion in 8ddfff5533d4d77dbd7da65 .
You're now duplicating the delimiters for those two. For IO::Path you've removed the superfluous pipe escaping but retain the poor factoring. -zefram
From: Elizabeth Mattijsen <liz [...] dijkmat.nl>
To: Elizabeth Mattijsen via RT <perl6-bugs-followup [...] perl.org>
Date: Mon, 21 Dec 2015 17:09:24 +0100
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
Download (untitled) / with headers
text/plain 589b
Show quoted text
> On 21 Dec 2015, at 16:58, Zefram <zefram@fysh.org> wrote: > > Elizabeth Mattijsen via RT wrote:
>> Good point. Followed your suggestion in 8ddfff5533d4d77dbd7da65 .
> > You're now duplicating the delimiters for those two. For IO::Path you've > removed the superfluous pipe escaping but retain the poor factoring.
There is a subtle difference between Str.perl and Rakudo::Internals.PERLIFY-STR: The former puts double quotes around, the latter doesn’t. I’m afraid we’ll have to settle for poor factoring. Or if you have a better solution, a PR is very much welcome! Liz
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
Date: Mon, 21 Dec 2015 17:06:51 +0000
To: Elizabeth Mattijsen via RT <perl6-bugs-followup [...] perl.org>
From: Zefram <zefram [...] fysh.org>
Download (untitled) / with headers
text/plain 1.8k
Elizabeth Mattijsen via RT wrote: Show quoted text
>There is a subtle difference between Str.perl and >Rakudo::Internals.PERLIFY-STR: The former puts double quotes around, the >latter doesn't.
Yes, hence the doubling when you call the former and also wrap the result in quotes. Show quoted text
>I'm afraid we'll have to settle for poor factoring.
What? You need to call .perl and *not* put an extra layer of quotes around the result. What's the difficulty? You managed to do it for $!SPEC in IO::Path.perl. multi method perl(CompUnit::Repository::Locally:D:) { "{$?CLASS.^name}.new({$!prefix.abspath.perl})" } multi method perl(IO::Local:D:) { "{$!abspath.perl}.IO" } multi method perl(IO::Path:D:) { $!is-absolute ?? "{$.abspath.perl}.IO(:SPEC({$!SPEC.perl}))" !! "{$.path.perl}.IO(:SPEC({$!SPEC.perl}),:CWD({$!CWD.perl}))" } Further factoring win is available by taking advanage of Pair.perl. Aside from just being less code, it also gets you potential future wins if Pair.perl later gets clever enough to use :foo<bar> syntax. I showed how to factor this in a previous message on [perl #126935]: multi method perl(IO::Path:D:) { $!is-absolute ?? "{$.abspath.perl}.IO({:$!SPEC.perl})" !! "{$.path.perl}.IO({:$!SPEC.perl},{:$!CWD.perl})" } The same should really be done with $?CLASS, in case class deparsing gets cleverer about name scoping in the future: multi method perl(CompUnit::Repository::Locally:D:) { "{$?CLASS.perl}.new({$!prefix.abspath.perl})" } When writing a pervasive value deparsing system, such as .perl is intended to be, a great convenience is that one of the tools available to you is a pervasive value deparsing system (for all types other than whichever one you're working on right now). So you don't have to do the whole deparsing job yourself, and in fact you get better results by doing less of the work yourself. -zefram
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
Date: Mon, 21 Dec 2015 19:18:20 +0100
To: Elizabeth Mattijsen via RT <perl6-bugs-followup [...] perl.org>
From: Elizabeth Mattijsen <liz [...] dijkmat.nl>
Download (untitled) / with headers
text/plain 2.2k
Show quoted text
> On 21 Dec 2015, at 18:06, Zefram <zefram@fysh.org> wrote: > Elizabeth Mattijsen via RT wrote:
>> There is a subtle difference between Str.perl and >> Rakudo::Internals.PERLIFY-STR: The former puts double quotes around, the >> latter doesn't.
> > Yes, hence the doubling when you call the former and also wrap the result > in quotes. >
>> I'm afraid we'll have to settle for poor factoring.
> > What? You need to call .perl and *not* put an extra layer of quotes > around the result. What's the difficulty? You managed to do it for > $!SPEC in IO::Path.perl. > > multi method perl(CompUnit::Repository::Locally:D:) { > "{$?CLASS.^name}.new({$!prefix.abspath.perl})" > } > > multi method perl(IO::Local:D:) { "{$!abspath.perl}.IO" } > > multi method perl(IO::Path:D:) { > $!is-absolute > ?? "{$.abspath.perl}.IO(:SPEC({$!SPEC.perl}))" > !! "{$.path.perl}.IO(:SPEC({$!SPEC.perl}),:CWD({$!CWD.perl}))" > } > > Further factoring win is available by taking advanage of Pair.perl. > Aside from just being less code, it also gets you potential future > wins if Pair.perl later gets clever enough to use :foo<bar> syntax. > I showed how to factor this in a previous message on [perl #126935]: > > multi method perl(IO::Path:D:) { > $!is-absolute > ?? "{$.abspath.perl}.IO({:$!SPEC.perl})" > !! "{$.path.perl}.IO({:$!SPEC.perl},{:$!CWD.perl})" > } > > The same should really be done with $?CLASS, in case class deparsing > gets cleverer about name scoping in the future: > > multi method perl(CompUnit::Repository::Locally:D:) { > "{$?CLASS.perl}.new({$!prefix.abspath.perl})" > }
All of that goodness now in 591783d116a56d4b2c54f . Show quoted text
> hen writing a pervasive value deparsing system, such as .perl is intended > to be, a great convenience is that one of the tools available to you is > a pervasive value deparsing system (for all types other than whichever > one you're working on right now). So you don't have to do the whole > deparsing job yourself, and in fact you get better results by doing less > of the work yourself.
Indeed! Now, if this had been in a Pull Request, I could have just clicked on Merge, which would have been less work for me :-) Ever considered getting a commit bit ? Liz
Date: Mon, 21 Dec 2015 19:09:18 +0000
Subject: Re: [perl #126955] [BUG] more .perl string quoting problems
From: Zefram <zefram [...] fysh.org>
To: Elizabeth Mattijsen via RT <perl6-bugs-followup [...] perl.org>
Download (untitled) / with headers
text/plain 752b
Elizabeth Mattijsen via RT wrote: Show quoted text
>All of that goodness now in 591783d116a56d4b2c54f .
You left a stray line in IO::Path.perl, which calls PERLIFY-STR in sink context. Show quoted text
>Indeed! Now, if this had been in a Pull Request, I could have just >clicked on Merge, which would have been less work for me :-)
But it being a Pull Request would involve me signing over my firstborn to github. Show quoted text
>Ever considered getting a commit bit ?
I think that would be a bad idea, for a number of reasons. I'm suffering project overload as it is, and haven't used my Perl 5 commit bit in ages. You also know my overall opinion of Perl 6, and that doesn't make Rakudo an attractive bugfixing target for me. It seems best that my involvement remain peripheral. -zefram
Download (untitled) / with headers
text/plain 933b
On Mon, 21 Dec 2015 11:09:40 -0800, zefram@fysh.org wrote: Show quoted text
> Elizabeth Mattijsen via RT wrote:
> >All of that goodness now in 591783d116a56d4b2c54f .
> > You left a stray line in IO::Path.perl, which calls PERLIFY-STR in > sink context. >
> >Indeed! Now, if this had been in a Pull Request, I could have just > >clicked on Merge, which would have been less work for me :-)
> > But it being a Pull Request would involve me signing over my firstborn > to github. >
> >Ever considered getting a commit bit ?
> > I think that would be a bad idea, for a number of reasons. I'm suffering > project overload as it is, and haven't used my Perl 5 commit bit in ages. > You also know my overall opinion of Perl 6, and that doesn't make Rakudo > an attractive bugfixing target for me. It seems best that my involvement > remain peripheral. > > -zefram >
The stray line was removed with commit 34c4fe977e067b4e4eb02b5d. Test needed.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org