Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

more .perl string quoting problems #4887

Open
p6rt opened this issue Dec 18, 2015 · 11 comments
Open

more .perl string quoting problems #4887

p6rt opened this issue Dec 18, 2015 · 11 comments

Comments

@p6rt
Copy link

p6rt commented Dec 18, 2015

Migrated from rt.perl.org#126955 (status was 'open')

Searchable as RT126955$

@p6rt
Copy link
Author

p6rt commented Dec 18, 2015

From zefram@fysh.org

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

@p6rt
Copy link
Author

p6rt commented Dec 18, 2015

From @lizmat

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-archive.perl.org/perl6/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

@p6rt
Copy link
Author

p6rt commented Dec 18, 2015

The RT System itself - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

p6rt commented Dec 20, 2015

From zefram@fysh.org

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. 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.

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

@p6rt
Copy link
Author

p6rt commented Dec 21, 2015

From @lizmat

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 .

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

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

@p6rt
Copy link
Author

p6rt commented Dec 21, 2015

From zefram@fysh.org

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.

-zefram

@p6rt
Copy link
Author

p6rt commented Dec 21, 2015

From @lizmat

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

@p6rt
Copy link
Author

p6rt commented Dec 21, 2015

From zefram@fysh.org

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})"
  }

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

@p6rt
Copy link
Author

p6rt commented Dec 21, 2015

From @lizmat

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 .

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

@p6rt
Copy link
Author

p6rt commented Dec 21, 2015

From zefram@fysh.org

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

@p6rt
Copy link
Author

p6rt commented Mar 13, 2018

From @dogbert17

On Mon, 21 Dec 2015 11​:09​:40 -0800, zefram@​fysh.org wrote​:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant