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.

1 files changed, 19 insertions, 2 deletions

diff --git a/pp_hot.c b/pp_hot.c
index 75811a27f4..3bafefb79c 100644
--- 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;