From oainikusama@gmail.com

Hi everyone,

I have spoken to Zefram and TonyC on #p5p, and apparently it's not well
documented in perl which operations will eagerly create hash elements. In
foo(\$x{bla}), for example, the \ is treating its operand as an lvalue,
same as if it were on the lhs of an assignment like that. Now, foo($x{bla})
*also* treats $x{bla} as an lvalue, but doesn't eagerly create it. Instead,
it passes a PVLV to foo(), which can then create the hash element by
assigning to $_[0].

Function arguments are usually taken by value but there's no type
information to say which arguments specifically are taken by reference and
so need the lvalue treatment. This patch adds two extra tests for that
behaviour, so at least we can check whether they remain consistent.

Hope it helps :)
Thanks!

garu

From oainikusama@gmail.com

From d43d62c3f2f94c54e8f2ac85bd9d4965bb5d16c9 Mon Sep 17 00:00:00 2001
From: "Breno G. de Oliveira" <garu@cpan.org>
Date: Mon, 30 May 2016 23:56:57 -0300
Subject: [PATCH] Test to see if hash values are eagerly created

It's not well documented which operations will eagerly create hash
elements. In foo(\$x{bla}), for example, the \ is treating its
operand as an lvalue, same as if it were on the lhs of an assignment
like that. Now, foo($x{bla}) *also* treats $x{bla} as an lvalue, but
doesn't eagerly create it. Instead, it passes a PVLV to foo(), which
can then create the hash element by assigning to $_[0].

Function arguments are usually taken by value but there's no type
information to say which arguments specifically are taken by
reference and so need the lvalue treatment. This patch adds two
extra tests for that behaviour, so at least they remain consistent.
---
 t/op/ref.t | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/op/ref.t b/t/op/ref.t
index 19a44bb..4752b9f 100644
--- a/t/op/ref.t
+++ b/t/op/ref.t
@@ -8,7 +8,7 @@ BEGIN {
 
 use strict qw(refs subs);
 
-plan(235);
+plan(237);
 
 # Test this first before we extend the stack with other operations.
 # This caused an asan failure due to a bad write past the end of the stack.
@@ -114,6 +114,15 @@ is (join(':',@{$spring[5]}), "123:456:789");
 $spring2{"foo"}->[3] = 4;
 is (join(':',@{$spring2{"foo"}}), "1:2:3:4");
 
+# Test to see if hash values are eagerly created
+{
+    sub myspringersub {}
+    myspringersub($spring3{"foo"});
+    is (scalar keys %spring3, 0, 'regular key was not eagerly created after sub call');
+    myspringersub(\$spring3{"foo"});
+    ok (exists $spring3{"foo"}, 'referenced key was eagerly created after sub call');
+}
+
 # Test references to subroutines.
 
 {
-- 
2.5.1

From @jkeenan

On Tue May 31 05​:29​:03 2016, oainikusama@​gmail.com wrote​:

Hi everyone,

I have spoken to Zefram and TonyC on #p5p, and apparently it's not well
documented in perl which operations will eagerly create hash elements. In
foo(\$x{bla}), for example, the \ is treating its operand as an lvalue,
same as if it were on the lhs of an assignment like that. Now, foo($x{bla})
*also* treats $x{bla} as an lvalue, but doesn't eagerly create it. Instead,
it passes a PVLV to foo(), which can then create the hash element by
assigning to $_[0].

Function arguments are usually taken by value but there's no type
information to say which arguments specifically are taken by reference and
so need the lvalue treatment. This patch adds two extra tests for that
behaviour, so at least we can check whether they remain consistent.

Hope it helps :)
Thanks!

garu

Here's the commit message​:

#####
It's not well documented which operations will eagerly create hash
elements. In foo(\$x{bla}), for example, the \ is treating its
operand as an lvalue, same as if it were on the lhs of an assignment
like that. Now, foo($x{bla}) *also* treats $x{bla} as an lvalue, but
doesn't eagerly create it. Instead, it passes a PVLV to foo(), which
can then create the hash element by assigning to $_[0].

Function arguments are usually taken by value but there's no type
information to say which arguments specifically are taken by
reference and so need the lvalue treatment. This patch adds two
extra tests for that behaviour, so at least they remain consistent.
#####

... and here are the added tests​:

#####
+# Test to see if hash values are eagerly created
+{
+ sub myspringersub {}
+ myspringersub($spring3{"foo"});
+ is (scalar keys %spring3, 0, 'regular key was not eagerly created after sub call');
+ myspringersub(\$spring3{"foo"});
+ ok (exists $spring3{"foo"}, 'referenced key was eagerly created after sub call');
+}
+
#####

Now, there's a lot of analysis packed into that commit message -- but I'm not sure that all of it is reflected in the tests.

The commit message has two paragraphs. The first focuses on lvalues; the second on function arguments. The tests seem to me to focus only on the second.

I don't think there is anything really *wrong* here -- but I wonder if the combination of the commit message and the tests really clarifies the issue for people (like me) who are not experts in the perlguts.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

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

From zefram@fysh.org

James E Keenan via RT wrote​:

Now, there's a lot of analysis packed into that commit message --
but I'm not sure that all of it is reflected in the tests.

Yes. The commit message is mostly a rearrangement of comments I made
on IRC while we were discussing the issue, setting out the background
for the behaviour. It's not well structured to provide context for the
tests; in fact it's not well structured at all.

Furthermore, the form of the test code suggests some misunderstanding
of what I said. I contrasted "foo($x{bla})" against "\$x{bla}".
"foo(\$x{bla})", which has both kinds of lvalueness (but not applied to
the same subexpression) is not a coherent choice of test expression.

I approve of adding tests for this part of the language, but we should do
better than this patch. Better test code can be readily be constructed​:
there are a bunch of additional nearby cases that should be covered.
The commit message doesn't need to explain the semantics at the level
of detail that was attempted.

-zefram

From zefram@fysh.org

I wrote​:

                    Better test code can be readily be constructed​:

there are a bunch of additional nearby cases that should be covered.

Here's some code to be thinking about​:

  sub noop { }
  sub overwrite { $_[0] = 123; }

  my %a;
  my $r = \$a{x};
  is_deeply \%a, {x=>undef};
  $$r = 123;
  is_deeply \%a, {x=>123};

  my %b;
  noop($b{x});
  is_deeply \%b, {};

  my %c;
  overwrite($c{x});
  is_deeply \%c, {x=>123};

  my @​a;
  my $s = \$a[1];
  is_deeply \@​a, [undef, undef];
  $$s = 123;
  is_deeply \@​a, [undef, 123];

  my @​b;
  noop($b[1]);
  is_deeply \@​b, [];

  my @​c;
  overwrite($c[1]);
  is_deeply \@​c, [undef, 123];

-zefram

From @demerphq

On 31 May 2016 at 14​:29, B O <perlbug-followup@​perl.org> wrote​:

# New Ticket Created by "B O"
# Please include the string​: [perl #128301]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=128301 >

Hi everyone,

I have spoken to Zefram and TonyC on #p5p, and apparently it's not well
documented in perl which operations will eagerly create hash elements. In
foo(\$x{bla}), for example, the \ is treating its operand as an lvalue,
same as if it were on the lhs of an assignment like that. Now, foo($x{bla})
*also* treats $x{bla} as an lvalue, but doesn't eagerly create it. Instead,
it passes a PVLV to foo(), which can then create the hash element by
assigning to $_[0].

Function arguments are usually taken by value but there's no type
information to say which arguments specifically are taken by reference and
so need the lvalue treatment. This patch adds two extra tests for that
behaviour, so at least we can check whether they remain consistent.

While I think its useful to have some kind of testing for this
behavior I worry that what you are testing for is accidental or
side-effect kind of behaviour of not fully thought through changes,
and that by adding tests for it we bake a commitment to treating them
as "expected behaviour" when they are not.

I am fine if we write a bunch of tests which becomes an assay of the
current state of affairs, but once we have the list we should probably
discuss whether any of them are controversial or actually bugs which
need to be fixed before we commit to them being specific tests.
Perhaps some of them should actually be TODO tests of the opposite of
what we do now.

Hope that makes sense. I do appreciate the thought and effort that
went into this.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

From zefram@fysh.org

demerphq wrote​:

    I worry that what you are testing for is accidental or

side-effect kind of behaviour of not fully thought through changes,

No, it's long-standing behaviour. Probably all the way back to 5.000,
which introduced references. We shouldn't change any of it.

-zefram

From @demerphq

On 1 June 2016 at 11​:32, Zefram <zefram@​fysh.org> wrote​:

demerphq wrote​:

    I worry that what you are testing for is accidental or

side-effect kind of behaviour of not fully thought through changes,

No, it's long-standing behaviour. Probably all the way back to 5.000,
which introduced references. We shouldn't change any of it.

A bug that lives long enough is a feature? I don't think that is the
right standard. I think anything like this that gets a test should
also have a clear independent rationale that it is correct.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

From zefram@fysh.org

demerphq wrote​:

A bug that lives long enough is a feature?

A bug that lives long enough may be troublesome to fix. In this case
I'm satisfied that it's not a bug, it's Larry's design, but even if it
were clearly buggy it shouldn't be changed accidentally. The behaviour
is readily accessible in ordinary programs, and affects their semantics
in obvious ways. On simple stability grounds, it's worth having test
cases to enforce that we don't change this behaviour without specific
intent to do so.

If you want to change it, presumably you wouldn't want to make
foo($x{bla}) eager, because that would be creating unintended hash entries
all over where the parameter is taken by value. Presumably you'd want
to make \$x{bla} lazy. I don't have a clear idea of what impact that
would have. Maybe you should do a CPAN smoke of that version of the
core (it's a very easy change to make), though obviously changes of
the underlying language aren't really what CPAN module test suites are
looking for. Presumably we'd want a deprecation cycle for the change,
or a feature flag, and that seems an awful lot of fuss to go to for a
design wart of such low wartiness.

If all you want is for a lazy version of \$x{bla} to be available, that
could be easily implemented by an XS module. (Alas, "sub lazy_lv {
\$_[0] }" doesn't do it​: it becomes eager.)

-zefram

From zefram@fysh.org

Yves sent a reply directly to me, but has clarified privately that he
intended it to go to the list. Here it is in full​:

Date​: Thu, 2 Jun 2016 16​:19​:58 +0200
From​: demerphq <demerphq@​gmail.com>
Message-ID​: <CANgJU+VCAALbCtxMF5=Y62X-rMvvfYMCZot-8e8-yesDUOqmSw@​mail.gmail.com>

On 1 Jun 2016 6​:41 p.m., "Zefram" <zefram@​fysh.org> wrote​:

demerphq wrote​:

A bug that lives long enough is a feature?

A bug that lives long enough may be troublesome to fix.

Heh. Dont I wish i had no idea what you mean!

In this case
I'm satisfied that it's not a bug, it's Larry's design,

If you are satisfied of that then its good enough for me.

but even if it
were clearly buggy it shouldn't be changed accidentally.

Agreed. But there is history of people using tests to justify that a
particular behaviour is expected when there is no documentation to say
otherwise.

To me there is a tension between the desire to create tests which tell us
when something has changed and tests that tell us something is right.

So i am all fine with us 'blindly' enumerating how things work so long as
we note that the test is not binding on us that it *should* work that way.
I am also fine when a qualified person (like yourself) does the required
research to determine that the test corresponds with what we consider to be
right. But i feel we shouldnt confuse the two types of test intent.

The behaviour
is readily accessible in ordinary programs, and affects their semantics
in obvious ways. On simple stability grounds, it's worth having test
cases to enforce that we don't change this behaviour without specific
intent to do so.

Again no arguments there.

If you want to change it, presumably you wouldn't want to make
foo($x{bla}) eager, because that would be creating unintended hash entries
all over where the parameter is taken by value. Presumably you'd want
to make \$x{bla} lazy. I don't have a clear idea of what impact that
would have. Maybe you should do a CPAN smoke of that version of the
core (it's a very easy change to make), though obviously changes of
the underlying language aren't really what CPAN module test suites are
looking for. Presumably we'd want a deprecation cycle for the change,
or a feature flag, and that seems an awful lot of fuss to go to for a
design wart of such low wartiness.

If all you want is for a lazy version of \$x{bla} to be available, that
could be easily implemented by an XS module. (Alas, "sub lazy_lv {
\$_[0] }" doesn't do it​: it becomes eager.)

Really i am fine with you saying 'ive reviewed the tests and as far as i
can tell they all match expected behaviour' i just want to be sure that
someone does so or otherswise we should note the test as nonbinding.

Fwiw i have encountered this scenario a few times and it is a pain
Eg two tests that are contradictory but due to bugs both pass. If both are
binding then we have a problem.

Yves
Ps writingvthis mail on a phone with my fat fingers was a real pain. Please
forgive any typos.

-zefram

From @cpansprout

On Thu Jun 02 13​:31​:51 2016, zefram@​fysh.org wrote​:

From​: demerphq <demerphq@​gmail.com>
i just want to be sure
that
someone does so or otherswise we should note the test as nonbinding.

$ ack normative t
t/mro/package_aliases.t
371​: # The next line of code is *not* normative. If the structure changes,

t/mro/package_aliases_utf8.t
439​: # The next line of code is *not* normative. If the structure changes,

t/op/join.t
79​: # this warning isn't normative, the implementation may choose to

t/op/sub_lval.t
323​:# These next two tests are not necessarily normative. But this way we will

I’m responsible for most of those. :-)

Eg two tests that are contradictory but due to bugs both pass.

I’ve also fixed broken tests. If there is consensus that the tests are wrong, we can change (or delete) them.

--

Father Chrysostomos

From @demerphq

On 2 June 2016 at 22​:57, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Thu Jun 02 13​:31​:51 2016, zefram@​fysh.org wrote​:

From​: demerphq <demerphq@​gmail.com>
i just want to be sure
that
someone does so or otherswise we should note the test as nonbinding.

$ ack normative t
t/mro/package_aliases.t
371​: # The next line of code is *not* normative. If the structure changes,

t/mro/package_aliases_utf8.t
439​: # The next line of code is *not* normative. If the structure changes,

t/op/join.t
79​: # this warning isn't normative, the implementation may choose to

t/op/sub_lval.t
323​:# These next two tests are not necessarily normative. But this way we will

I’m responsible for most of those. :-)

I am responsible for some, but Iacked the foresight to choose a
standard term like normative to describe the test.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"