- 
                Notifications
    You must be signed in to change notification settings 
- Fork 602
Apply HINT_BLOCK_SCOPE to lexical variable declarations #23867
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
base: blead
Are you sure you want to change the base?
Conversation
At present, any `else {...}` block is wrapped in an ENTER/LEAVE pair, even
if no new scope is warranted, which causes runtime inefficiency. For
example, this block doesn't need a scope but gets one anyway:
```
    } else {
        return 1;
    }
```
However, this behaviour means that an object like `$kaboom` will reliably
be destroyed when the `else` block is exited:
```
    } else {
        my $kaboom = TickTick->new;
    }
```
In contrast, `if () {...}` or `elsif () {...}` blocks default to not
having an `ENTER/LEAVE` pair, which is more efficient but arguably
incorrect, as `$marvin` in this code won't be destroyed when the `if`
block is exited:
```
    if ( $x ) {
        my $marvin = TickTick->new;
    }
```
Exactly when it is destroyed is dependent upon where the next scope
exit happens to be, and this could be some ways away from its block.
This behaviour is also very brittle, as shown in this case where the
no-op `0;` statement causes - via a quirk of parsing - the `if`
block to be assigned an `ENTER/LEAVE` pair:
```
    if ( $x ) {
        0; my $marvin = TickTick->new;
    }
```
Whether a block gains a scope via `ENTER/LEAVE` pair or is just parented
by a `SCOPE` OP (that will be optimized away), is decided by
`Perl_op_scope` on the basis of the `OPf_PARENS` flag on a block's
`LINESEQ` OP. The parser always sets this flag for `else` blocks,
a variety of circumstances determines whether it is set otherwise.
This commit makes two changes:
* Removes the enforced use of the `OPf_PARENS` flag on `else` blocks.
The same rules now apply to `if`, `elsif`, and `else`, hopefully making
destruction behaviour more predictable.
* Changes the tokenizer such that occurrances of `my`, `our`, and `state`
set the `OPf_PARENS` flag, and the containing block will be wrapped in
an `ENTER/LEAVE` pair.
After this commit, neither of the `if` or `else` block in this example
will have an unnecessary`ENTER/LEAVE` pair:
```
    if ($x ) {
        return 0;
    } else {
        return 1;
    }
```
And in this example, both objects will be destroyed when the relevant
block is entered:
```
    if ( $x ) {
        my $kaboom_true = TickTick->new;
    } else {
        my $kaboom_else = TickTick->new;
    }
```
Sadly, both blocks in this example will still have unnecessary scopes,
but fixing that is for a different PR:
```
    if ($x ) {
        0; return 0;
    } else {
        0; return 1;
    }
```
There are two downsides to this commit:
1. It has the potential to change the destruction timing of objects created
in an `if` block and assigned to a lexical declared within the block, if
the block hasn't ended up with an `ENTER/LEAVE` pair. As noted above though.
that behaviour is very brittle and already sensitive to even minor changes
to the Perl code - and also to minor parser/optimization changes within the
interpreter.
2. The old `else` behaviour meant that the first statement's `NEXTSTATE` OP
stays in place and consequently any error messages arising from the first
statement mention the correct (or closer) line number. The `if`/`elsif`
behaviour is more likely to cause the `NEXTSTATE` OP to be optimized away,
causing any error messages to contain the line number for where the `if`
or `elsif` keyword keyword occurred, which could be many lines away.
Following this commit, first-line error messages will be just as bad
for an `else` block as they are for `if`/`elsif` blocks. However, that's
a separate issue and should not be a reason to avoid this commit.
    5cf7e60    to
    192d369      
    Compare
  
    | Rebased for merge conflicts. | 
| Please add some detail to the message for the final commit, I don't fiddle with the op tree much so I don't see off-hand why those changes are needed. | 
Until `else` blocks were subject to the same block scoping rules as `if`
and `elsif`, an empty `else` block or ternary condition would arrive at
the peephole optimizer as a bare stub OP:
    +--null (ex-stub) OP(0x562e555a7310)
        FLAGS = (...,PARENS,SLABBED)
Now, it could arrive in that form OR as a SCOPE + NULL:
    scope LISTOP(0x5603b282b190)
    PARENT ===> 3 [cond_expr 0x5603b282b770]
    FLAGS = (VOID,KIDS,SLABBED)
    |
    +--null (ex-stub) OP(0x5603b282b350) ===> 1 [scope 0x5603b282b190]
        FLAGS = (VOID,SLABBED)
This commit updates the "empty else" optimization on `OP_COND_EXPR`s so
that the OPs associated with empty "else" conditions are removed (when
not in scalar context) regardless of which form they arrive in.
    192d369    to
    ef13941      
    Compare
  
    | 
 Thanks, I've revised the final commit message to explain the difference made by the first commit. | 
| Have we encountered actual bugs due to the behavior you describe in the first post? That is, more than just "runtime inefficiency"? | 
| 
 I'm not aware of a currently open or historical Issue (besides mine) relating to the scoping of  I think it is a bug - or at least, some very undesirable behaviour - that the scoping of those blocks is so brittle and end users would have to dump the optree, or carefully instrument/test variable destruction, to be sure of what scoping has been applied. I haven't tested older versions of Perl to see if the behaviour has always been consistent, perhaps that would be useful. | 
| 
 Seems to be the same behaviour in 5.40.1, 5.24.0, 5.22.0, 5.20.0, 5.18.0, 5.16.0, 5.14.0, 5.12.0, 5.10.0 | 
| 
 Also consistent through 5.38.0, 5.36.0, 5.34.0, 5.32.0, 5.30.0, 5.28.0, 5.26.0 There are some commits that modified perly.y during 1999-2001 which could have changed behaviour, so it would be useful to see release builds prior to and during that time. | 
| 5.8.9 also behaves the same way, so that behaviour goes way back. I still think it's a bug. It might be rare in practice to experience it though. The user needs a variable which is expected to have its lifetime bounded by the block scope and that not happening is noticeable (e.g. by DESTROY timing or a bug around ref counts). As soon as the block has more than a simple, single statement it will often get a proper scope applied. These are examples I tried, and all get wrapped in an ENTER/LEAVE: A more complicated single statement might not get an ENTER/LEAVE. For example:  | 
Apply HINT_BLOCK_SCOPE to lexical variable declarations
At present, any
else {...}block is wrapped in an ENTER/LEAVE pair, evenif no new scope is warranted, which causes runtime inefficiency. For
example, this block doesn't need a scope but gets one anyway:
However, this behaviour means that an object like
$kaboomwill reliablybe destroyed when the
elseblock is exited:In contrast,
if () {...}orelsif () {...}blocks default to nothaving an
ENTER/LEAVEpair, which is more efficient but arguablyincorrect, as
$marvinin this code won't be destroyed when theifblock is exited:
Exactly when it is destroyed is dependent upon where the next scope
exit happens to be, and this could be some ways away from its block.
This behaviour is also very brittle, as shown in this case where the
no-op
0;statement causes - via a quirk of parsing - theifblock to be assigned an
ENTER/LEAVEpair and so$marvinwill be destroyed when the block exits:
Whether a block gains a scope via
ENTER/LEAVEpair or is just parentedby a
SCOPEOP (that will be optimized away), is decided byPerl_op_scopeon the basis of theOPf_PARENSflag on a block'sLINESEQOP. The parser always sets this flag forelseblocks,a variety of circumstances determines whether it is set otherwise.
This commit makes two changes:
Removes the enforced use of the
OPf_PARENSflag onelseblocks.The same rules now apply to
if,elsif, andelse, hopefully makingdestruction behaviour more predictable.
Changes the tokenizer such that occurrances of
my,our, andstateset the
OPf_PARENSflag, and the containing block will be wrapped inan
ENTER/LEAVEpair.After this commit, neither of the
iforelseblocks in this examplewill have an unnecessary
ENTER/LEAVEpair:And in this example, both objects will be destroyed when the relevant
block is exited:
Sadly, both blocks in this example will still have unnecessary scopes,
but fixing that is for a different PR:
There are two downsides to this commit:
It has the potential to change the destruction timing of objects created
in an
ifblock and assigned to a lexical declared within the block, ifthe block hasn't ended up with an
ENTER/LEAVEpair. As noted above though.that behaviour is very brittle and already sensitive to even minor changes
to the Perl code - and also to minor parser/optimization changes within the
interpreter.
The old
elsebehaviour meant that the first statement'sNEXTSTATEOPstays in place and consequently any error messages arising from the first
statement mention the correct (or closer) line number. The
if/elsifbehaviour is more likely to cause the
NEXTSTATEOP to be optimized away,causing any error messages to contain the line number for where the
ifor
elsifkeyword keyword occurred, which could be many lines away.Following this commit, first-line error messages will be just as bad
for an
elseblock as they are forif/elsifblocks. However, that'sa separate issue and should not be a reason to avoid this commit.