Revert "Allow returning of temps and ro’s from lv subs"

This reverts commit b724cc14b25929aee44eee20bd26102cceb520b6.

Lvalue subroutines are more of a mess than I realised when I made
that commit.

I just tried removing the syntactical restriction on what the last
statement or the argument to return can be in an lvalue sub. It opened
a whole can of worms.

PADTMPs are bizarre creatures that have somewhat erratic behaviour.
Since assigning to a PADTMP is almost always a mistake (since the
value will definitely be discarded), those *should* be disallowed,
when the sub is called in lvalue context. That also avoids propagating
a whole load of bugs when referencing PADTMPs, aliasing to them, etc.

Read-only scalars end up triggering a ‘Modification of a read-only
value attempted’ message without the restrictions in pp_leavesublv,
but the message the latter was providing (which this revert restores)
is clearer (‘Can't return a readonly value from lvalue subroutine’).

Speaking of lvalue context, there are three types of context with
regard to lvalue-ness (orthogonal to the usual void/scalar/list
contexts):
• rvalue context ($x = func())
• lvalue context (func() = $x)
• semi-lvalue context (\func())

Each is handle by a separate code path in pp_leavesublv:
• rvalue context - any value can be returned; it’s copied (waste-
  ful, perhaps?)
• semi-lvalue context - any value can be returned; it’s not copied
• lvalue context - stringent rules about what can and cannot be
  returned (which this revert restores)

So it is perfectly fine to restrict what can be returned from an
lvalue sub *in true lvalue context*, without affected rvalue use.

Now, regarding TEMPs, although this commit restores the restriction on
returning TEMPs, a forthcoming commit will relax that restriction once
more, since it causes bugs.

diff --git a/pp_hot.c b/pp_hot.c
index 75811a27f4d3ee2b6da2113ec2e0a7cd9bebd6b3..3bafefb79cc1f5b4e80006437d5522233753dd47 100644 (file)
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -2725,10 +2725,27 @@ PP(pp_leavesublv)
            MARK = newsp + 1;
            EXTEND_MORTAL(1);
            if (MARK == SP) {
-                   /* Can be a localized value
-                    * subject to deletion. */
+               /* Temporaries are bad unless they happen to have set magic
+                * attached, such as the elements of a tied hash or array */
+               if ((SvFLAGS(TOPs) & (SVs_TEMP | SVs_PADTMP) ||
+                    (SvFLAGS(TOPs) & (SVf_READONLY | SVf_FAKE))
+                      == SVf_READONLY
+                   ) &&
+                   !SvSMAGICAL(TOPs)) {
+                   LEAVE;
+                   cxstack_ix--;
+                   POPSUB(cx,sv);
+                   PL_curpm = newpm;
+                   LEAVESUB(sv);
+                   DIE(aTHX_ "Can't return %s from lvalue subroutine",
+                       SvREADONLY(TOPs) ? (TOPs == &PL_sv_undef) ? "undef"
+                       : "a readonly value" : "a temporary");
+               }
+               else {                  /* Can be a localized value
+                                        * subject to deletion. */
                    PL_tmps_stack[++PL_tmps_ix] = *mark;
                    SvREFCNT_inc_void(*mark);
+               }
            }
            else {                      /* Should not happen? */
                LEAVE;

diff --git a/t/op/sub_lval.t b/t/op/sub_lval.t
index e51936fdd808cf42c454fa1d7a3efa0ca27a3ca9..9990fb8bef2316fcdf778126937ed519a9e4ae0a 100644 (file)
--- a/t/op/sub_lval.t
+++ b/t/op/sub_lval.t
@@ -218,7 +218,7 @@ eval <<'EOE' or $_ = $@;
   1;
 EOE
 
-like($_, qr/Modification of a read-only value attempted/);
+like($_, qr/Can't return undef from lvalue subroutine/);
 
 sub lv10 : lvalue {}
 
@@ -238,7 +238,7 @@ eval <<'EOE' or $_ = $@;
   1;
 EOE
 
-like($_, qr/Modification of a read-only value attempted/);
+like($_, qr/Can't return undef from lvalue subroutine/);
 
 $_ = undef;
 eval <<'EOE' or $_ = $@;