Skip to content

Conversation

tonycoz
Copy link
Contributor

@tonycoz tonycoz commented Dec 18, 2023

Since return isn't actually a function, I didn't think the "function" part of the original message applied.

Fixes #21716

Copy link
Contributor

@leonerd leonerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this change makes a few things become compiletime errors that previously had been either runtime errors or potentially fine if subs were defined late, it's probably worth at least a mention in the perldelta so if people find code breaks they know why.

Since return isn't actually a function, I didn't think the "function"
part of the original message applied.

Fixes Perl#21716
@tonycoz tonycoz force-pushed the 21716-return-ck-handle branch from 83308cc to 12ea353 Compare December 18, 2023 22:00
@tonycoz
Copy link
Contributor Author

tonycoz commented Dec 18, 2023

As this change makes a few things become compiletime errors that previously had been either runtime errors or potentially fine if subs were defined late, it's probably worth at least a mention in the perldelta so if people find code breaks they know why.

If the sub is defined late the indirect object has already been compiled into an rv2gv op:

# system perl without this patch
$ perl -MO=Concise,xx -e 'sub xx { return sum map { $_ } 1..5 } sub sum(@); '
main::xx:
a  <1> leavesub[1 ref] K/REFC,1 ->(end)
-     <@> lineseq KP ->a
1        <;> nextstate(main 2 -e:1) v ->2
-        <@> return KS ->-
-           <0> pushmark s ->2
3           <1> rv2gv sKR/1 ->4
2              <#> gv[*sum] s ->3
8           <|> mapwhile(other->9)[t4] K ->a
7              <@> mapstart K ->8
4                 <0> pushmark s ->5
-                 <1> null lK/1 ->5
-                    <1> null lK/1 ->8
-                       <@> scope lK ->-
-                          <;> ex-nextstate(main 3 -e:1) v ->9
-                          <1> ex-rv2sv sK/1 ->-
9                             <#> gvsv[*_] s ->8
6                 <1> rv2av lKPM/1 ->7
5                    <$> const[AV ARRAY] s ->6
-e syntax OK

It even runs and returns a GV:

# system perl without this patch
$ perl -MDevel::Peek -e 'sub xx { return sum map { $_ } 1..5 } sub sum(@); Dump([xx()])'
SV = IV(0x561df481e1a8) at 0x561df481e1b8
  REFCNT = 1
  FLAGS = (TEMP,ROK)
  RV = 0x561df481d750
  SV = PVAV(0x561df47f2f50) at 0x561df481d750
    REFCNT = 1
    FLAGS = ()
    ARRAY = 0x561df482da10
    FILL = 5
    MAX = 5
    FLAGS = (REAL)
    Elt No. 0
    SV = PVGV(0x561df48497b0) at 0x561df481d738
      REFCNT = 1
      FLAGS = (FAKE,MULTI)
      NAME = "sum"
      NAMELEN = 3
      GvSTASH = 0x561df47f1518  "main"
      FLAGS = 0x2
      GP = 0x561df482e180
        SV = 0x0
        REFCNT = 3
        IO = 0x0
        FORM = 0x0  
        AV = 0x0
        HV = 0x0
        CV = 0x561df481e3e0
        CVGEN = 0x0
        GPFLAGS = 0x0 ()
        LINE = 1
        FILE = "-e"
        EGV = 0x561df481e0c8    "sum"
    Elt No. 1
    SV = IV(0x561df481d7d0) at 0x561df481d7e0
      REFCNT = 1
      FLAGS = (IOK,pIOK)
      IV = 1
    Elt No. 2
    SV = IV(0x561df481d7e8) at 0x561df481d7f8
      REFCNT = 1
      FLAGS = (IOK,pIOK)
      IV = 2
    Elt No. 3
    SV = IV(0x561df481d878) at 0x561df481d888
      REFCNT = 1
      FLAGS = (IOK,pIOK)
      IV = 3

So perhaps it should be a deprecation instead.

That would make me sad for such obviously broken code, since it seems more likely that the intent is to call a function rather than return a glob.

@tonycoz
Copy link
Contributor Author

tonycoz commented Dec 18, 2023

That would make me sad for such obviously broken code, since it seems more likely that the intent is to call a function rather than return a glob.

which my perldelta illustrates.

Copy link
Contributor

@leonerd leonerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I think this looks fine, though we should keep an eye out for BBC reports in case it turns out there's modules around that break on it - or even that actually rely on it to work.

@tonycoz
Copy link
Contributor Author

tonycoz commented Dec 18, 2023

I also considered a parser approach to fixing this (splitting LSTOP into list ops that take an indirob and those that don't), but that's more complex and riskier.

@tonycoz tonycoz merged commit f41b73e into Perl:blead Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Return with a bareword compiles under strict

2 participants