3.8.0b2 no longer optimizes away "if 0:" ?

@gvanrossum

BPO	37500
Nosy	@gvanrossum, @tim-one, @terryjreedy, @nedbat, @ambv, @serhiy-storchaka, @pganssle, @pablogsal, @miss-islington, @tirkarthi, @aldwinaldwin
PRs	[3.7] bpo-37500: Revert commit 85ed1712e428f93408f56fc684816f9a85b0ebc0 #14605 bpo-37500: Make sure dead code does not generate bytecode but also detect syntax errors #14612 [3.8] bpo-37500: Make sure dead code does not generate bytecode but also detect syntax errors (GH-14612) #14780 [3.8] bpo-37500: Make sure dead code does not generate bytecode but also detect syntax errors (GH-14612) #15002

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-07-29.15:17:11.699>
created_at = <Date 2019-07-05.03:02:39.832>
labels = ['3.8', '3.9', 'release-blocker']
title = '3.8.0b2 no longer optimizes away "if 0:" ?'
updated_at = <Date 2019-07-29.15:17:11.697>
user = 'https://github.com/nedbat'

bugs.python.org fields:

activity = <Date 2019-07-29.15:17:11.697>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2019-07-29.15:17:11.699>
closer = 'pablogsal'
components = []
creation = <Date 2019-07-05.03:02:39.832>
creator = 'nedbat'
dependencies = []
files = []
hgrepos = []
issue_num = 37500
keywords = ['patch', '3.8regression']
message_count = 44.0
messages = ['347296', '347300', '347302', '347308', '347309', '347342', '347349', '347362', '347368', '347369', '347372', '347375', '347377', '347380', '347381', '347382', '347383', '347385', '347386', '347387', '347388', '347389', '347392', '347393', '347394', '347396', '347397', '347398', '347401', '347402', '347404', '347405', '347406', '347408', '347409', '347410', '347500', '347504', '347511', '347952', '348654', '348659', '348668', '348669']
nosy_count = 11.0
nosy_names = ['gvanrossum', 'tim.peters', 'terry.reedy', 'nedbat', 'lukasz.langa', 'serhiy.storchaka', 'p-ganssle', 'pablogsal', 'miss-islington', 'xtreak', 'aldwinaldwin']
pr_nums = ['14605', '14612', '14780', '15002']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue37500'
versions = ['Python 3.8', 'Python 3.9']

CPython 3.8.0b2 behaves differently than previous releases: it no longer optimizes away "if 0:" branches. This is something that has been done since at least 2.4. It was done in 3.8.0b1, but no longer is.

Was this a purposeful change? Why? It seems like 3.8 is adding more optimizations, why was this removed?

Test code (/tmp/no0.py):

import dis
import sys

print(sys.version)

def with_if_0():
    print(1)
    if 0:
        print(2)
    print(3)

dis.dis(with_if_0)

$ /usr/local/pythonz/pythons/CPython-3.8.0b2/bin/python3.8 /tmp/no0.py
3.8.0b2 (default, Jul  4 2019, 22:38:04)
[Clang 10.0.0 (clang-1000.10.44.4)]
  7           0 LOAD_GLOBAL              0 (print)
              2 LOAD_CONST               1 (1)
              4 CALL_FUNCTION            1
              6 POP_TOP

8 8 LOAD_CONST 2 (0)
10 POP_JUMP_IF_FALSE 20

9 12 LOAD_GLOBAL 0 (print)
14 LOAD_CONST 3 (2)
16 CALL_FUNCTION 1
18 POP_TOP

10 >> 20 LOAD_GLOBAL 0 (print)
22 LOAD_CONST 4 (3)
24 CALL_FUNCTION 1
26 POP_TOP
28 LOAD_CONST 0 (None)
30 RETURN_VALUE

$ /usr/local/pythonz/pythons/CPython-3.8.0b1/bin/python3.8 /tmp/no0.py
3.8.0b1 (default, Jun  4 2019, 21:21:14)
[Clang 10.0.0 (clang-1000.10.44.4)]
  7           0 LOAD_GLOBAL              0 (print)
              2 LOAD_CONST               1 (1)
              4 CALL_FUNCTION            1
              6 POP_TOP

10 8 LOAD_GLOBAL 0 (print)
10 LOAD_CONST 4 (3)
12 CALL_FUNCTION 1
14 POP_TOP
16 LOAD_CONST 0 (None)
18 RETURN_VALUE

$ python3.7 /tmp/no0.py
3.7.3 (default, Apr 10 2019, 10:27:53)
[Clang 10.0.0 (clang-1000.10.44.4)]
  7           0 LOAD_GLOBAL              0 (print)
              2 LOAD_CONST               1 (1)
              4 CALL_FUNCTION            1
              6 POP_TOP

10 8 LOAD_GLOBAL 0 (print)
10 LOAD_CONST 2 (3)
12 CALL_FUNCTION 1
14 POP_TOP
16 LOAD_CONST 0 (None)
18 RETURN_VALUE

$ python2.7 /tmp/no0.py
2.7.14 (default, Oct  4 2017, 09:45:53)
[GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.37)]
  7           0 LOAD_CONST               1 (1)
              3 PRINT_ITEM
              4 PRINT_NEWLINE

10 5 LOAD_CONST 2 (3)
8 PRINT_ITEM
9 PRINT_NEWLINE
10 LOAD_CONST 0 (None)
13 RETURN_VALUE

$ python2.4 /tmp/no0.py
2.4.6 (#1, Jun 18 2016, 17:49:14)
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)]
  7           0 LOAD_CONST               1 (1)
              3 PRINT_ITEM
              4 PRINT_NEWLINE

10 5 LOAD_CONST 2 (3)
8 PRINT_ITEM
9 PRINT_NEWLINE
10 LOAD_CONST 0 (None)
13 RETURN_VALUE

BTW, the same regression applies to "if not __debug__:" . Python3.8.0b1 optimized these away, but b2 does not. That optimization was new in 3.7.0a4

Not sure if related there were some changes done to __debug__ related checks with bpo-37269

FWIW: this is probably since PR14099

The root cause is PR 13332 for bpo-1875.

Since this was backported to 3.7 but not yet released, I'm adding 3.7regression.

How serious a regression is this? We still have time to revert the 3.7 commit of bpo-1875 (85ed171) from 3.7.4 final if we act immediately.

The real-word implications from my world are this: if your code has "if 0:" clauses in it, and you measure its coverage, then because the lines have not been optimized away, coverage.py will think the lines are a possible execution path, and will be considered a miss because they are not executed. This will reduce your coverage percentage.

I can't estimate how many people this will affect.

Ned, I have prepared PR14605 for the 3.7 branch to revert 85ed171. As this is somehow changing behavior in the 3.7 series, I think is the best course of action for 3.7.

For 3.8 the proposed way to do this is now using an unconditional jump to skip the conditional (in progress in PR 14116). The reason is that we need to compile the blocks to detect syntax errors even if the block will be unreachable. This is also a deep implementation detail and it should not be rely upon, in my opinion.

New changeset 7d93eff by Miss Islington (bot) (Pablo Galindo) in branch '3.7':
[3.7] bpo-37500: Revert commit 85ed171 (GH-14605)
7d93eff

I don't know what you mean by, "it should not be rely upon." Are you saying that in 3.8 you want to keep the lines in the compiled bytecode? Do you understand the implications for coverage measurement?

it should not be rely upon

IMHO, the correct behaviour under coverage analysis is to keep the code and not discard it. After all, it is code that exists, and that is not being executed, so it should count as uncovered code. The fact that some Python versions can detect at compile time that it will never be executed, and thus discard it ahead of time, should not have an impact on the coverage analysis.

I can see the logic of the argument that says the code exists, and should be measured. But this is changing behavior that has been in place for at least 15 years.

Before this change, code could have had an unreported SyntaxError, but it was code that was being discarded by the optimizer anyway. How many people are benefiting from those SyntaxErrors? The only way the code would ever run is if they changed the "if 0:" to something else, at which point they would have seen the SyntaxErrors without this change.

If we keep this change, I will hear from people unhappy with the drop in their coverage measurement. Have users of the language been complaining that they don't see SyntaxErrors in their optimized-away code?

I think I have found a simple way to make everyone happy: we can add a new field to the compiler that tells it to not emit bytecode. In this way we can detect errors by walking the blocks but no bytecode will be emitted.

I have updated PR 14612 to use this approach.

Using jumps is not removing the optimization
entirely, is just a weaker and more incomplete
way of doing the same.

Sorry, I'm afraid I have no idea what that means. The generated code before and after was wildly different, as shown in Ned's original report here. In what possible sense was his "if 0:" being "optimized" if it generated code to load 0 onto the stack, then POP_JUMP_IF_FALSE, and then jumped over all the code generated for the dead block?

The generated code after is nearly identical if I replace his "if 0:" with "if x:" (which the compiler takes to mean a global or builtin about whose truthiness it knows nothing at all). Indeed, the only difference in the byte code is that instead of doing a LOAD_CONST to load 0, it does a LOAD_GLOBAL to load x.

So, to my eyes, absolutely nothing of the optimization remained. At least not in the example Ned posted here.

So, to my eyes, absolutely nothing of the optimization remained. At least not in the example Ned posted here.

This is because his example did not include the changes in PR14116 that implemented that.

we could say that it does not matter if

def f():
if 0:
yield

should be or not a generator

Slippery slope arguments play better if they're made _before_ a decade has passed after the slope was fully greased.

There's nothing accidental about how yield behaves here. I wrote the original generator PEP, and very deliberately added these to its doctests (in test_generators.py):

"""
>>> def f():
...    if 0:
...        yield
>>> type(f())
<class 'generator'>


>>> def f():
...     if 0:
...         yield 1
>>> type(f())
<class 'generator'>

>>> def f():
...    if "":
...        yield None
>>> type(f())
<class 'generator'>
"""

Any alternate implementation that decided that whether "it's a generator" depended on optimizations would likely fail at least one of those tests. It was intended to be solely a compile-time decision, based purely on syntactic analysis.

So I've seen no reason to believe - or expect - that the damage here goes - or will ever go - deeper than that some dead code isn't raising compile-time errors in some rare cases (statements allowed only at function level being used in dead code outside function level).

Which should be fixed, if possible. But, as damage goes - sorry! - it just seems minimal to me.

Which should be fixed, if possible. But, as damage goes - sorry! - it just seems minimal to me.

I think I failed to express myself correctly there :( I have absolutely no problem with the way that behaves currently, I just wanted to point out the possible differences and view points regarding optimization and how "implementation details" percolate to observable behavior.

Sure seems to me as if we need to have a separate pass that looks for out-of-place `return`, `yield`, `break` and `continue`, run before the optimization removes `if 0:` blocks etc.

Yep, that is more or less the approach in PR14612. What I tried to achieve there is also minimize the amount of changes to make the pass for errors non intrusive.

New changeset 4834c80 by Ned Deily (Pablo Galindo) in branch '3.7':
[3.7] bpo-37500: Revert commit 85ed171 (GH-14605)
4834c80

New changeset 87a918a by Ned Deily in branch '3.7':
[3.7] bpo-37500: update Misc/NEWS entries to mention revert
87a918a

New changeset 18c5f9d by Miss Islington (bot) (Pablo Galindo) in branch 'master':
bpo-37500: Make sure dead code does not generate bytecode but also detect syntax errors (GH-14612)
18c5f9d

Victor closed the 3.8 backport, stating on GitHub:

"I closed the 3.8 backport (without merging it), until we agree on what should be done."

This is marked as release blocker. I will be releasing 3.8b3 as is, please decide what to do here before b4, I will block the last beta on this issue.

I am fine with backporting the 3.9 solution to 3.8. Sorry for the delay.

New changeset 9ea738e by Miss Islington (bot) in branch '3.8':
bpo-37500: Make sure dead code does not generate bytecode but also detect syntax errors (GH-14612)
9ea738e

I will close this for now, we can revisit this if we find a better solution for the issue.

Navigation Menu

Repository navigation

3.8.0b2 no longer optimizes away "if 0:" ? #81681

21 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

3.8.0b2 no longer optimizes away "if 0:" ? #81681

Description

Activity

nedbat commented on Jul 5, 2019

nedbat commented on Jul 5, 2019

tirkarthi commented on Jul 5, 2019

aldwinaldwin commented on Jul 5, 2019

serhiy-storchaka commented on Jul 5, 2019

nedbat commented on Jul 5, 2019

ned-deily commented on Jul 5, 2019

nedbat commented on Jul 5, 2019

pablogsal commented on Jul 5, 2019

miss-islington commented on Jul 5, 2019

nedbat commented on Jul 5, 2019

scoder commented on Jul 5, 2019

nedbat commented on Jul 5, 2019

21 remaining items

pablogsal commented on Jul 5, 2019

tim-one commented on Jul 6, 2019

pablogsal commented on Jul 6, 2019

tim-one commented on Jul 6, 2019

pablogsal commented on Jul 6, 2019

gvanrossum commented on Jul 8, 2019

pablogsal commented on Jul 8, 2019

ned-deily commented on Jul 8, 2019

miss-islington commented on Jul 15, 2019

ambv commented on Jul 29, 2019

serhiy-storchaka commented on Jul 29, 2019

miss-islington commented on Jul 29, 2019

pablogsal commented on Jul 29, 2019

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions