compiler: ignore constants used as statements (don't emit LOAD_CONST+POP_TOP) #70392
Comments
|
The bytecode compilers ignores ast.Str and ast.Num nodes: ---------------------------- >>> def func():
... 123
... "test"
...
>>> import dis; dis.dis(func)
3 0 LOAD_CONST 0 (None)
3 RETURN_VALUEBut other ast nodes which are constant are not ignored: ---------------------------- >>> def func2():
... b'bytes'
... (1, 2)
...
>>> import dis; dis.dis(func2)
2 0 LOAD_CONST 1 (b'bytes')
3 POP_TOP3 4 LOAD_CONST 4 ((1, 2)) I don't understand the point of loading a constant and then unload it (POP_TOP). Attached patch changes the compiler to not emit LOAD_CONST+POP_TOP anymore. My patch only affects constants. Example with the patch: >>> def f():
... x
...
>>> import dis
>>> dis.dis(f)
2 0 LOAD_GLOBAL 0 (x)
3 POP_TOP
4 LOAD_CONST 0 (None)
7 RETURN_VALUEThe compiler still emits "LOAD_GLOBAL x" for the instruction "x". Ignoring the Python instruction "x" would change the Python semantics, because the function would not raise a NameError anymore if x is not defined. Note: I noticed this inefficient bytecode while working on the issue bpo-26146 (add ast.Constant). |
vstinner
changed the title
|
The patch looks alright. Will the following code compile OK? What will it compile to? if 1:
42 |
|
LGTM. It looks to me that this optimization was added to avoid spending executing time for docstrings. Other cases almost never occur in real code and are not worth to be optimized. But the patch makes the code cleaner (it would even more cleaner if collapse all kinds of constants in Constant). |
|
Serhiy: "It looks to me that this optimization was added to avoid spending executing time for docstrings." Hum, let me dig Mercurial history. https://bugs.python.org/issue1333982#msg26659: Ah, it was a regression introduced by the new AST compiler. But the change introduced a new optimization: numbers are now also ignored. In Python 2.4 (before AST), numbers were not ignored: >>> def f():
... "a"
... 1
... "b"
...
>>> import dis; dis.dis(f)
3 0 LOAD_CONST 1 (1)
3 POP_TOP
4 LOAD_CONST 2 (None)
7 RETURN_VALUE If we continue to dig deeper, before AST, I found: Added accurate calculation of the actual stack size needed by the Also commented out all fprintf statements (except for a new one to This patch added the following code to com_expr_stmt() in Python/compile.c: + /* Forget it if we have just a doc string here */ I'm unable to find the exact part of the compiler which ignores strings in statement expressions. |
|
""" if 1:
42
"""compile OK: what do you mean? This code doesn't make any sense to me :-) Currently text strings statements are ignored: >>> def f():
... if 1:
... "abc"
...
>>> import dis
>>> dis.dis(f)
3 0 LOAD_CONST 0 (None)
3 RETURN_VALUEBut byte strings emit a LOAD_CONST+POP_TOP: >>> def g():
... if 1:
... b'bytes'
...
>>> dis.dis(g)
3 0 LOAD_CONST 1 (b'bytes')
3 POP_TOP
4 LOAD_CONST 0 (None)
7 RETURN_VALUEWith my patch, all constants statements will be ignored. Example with my patch: >>> def h():
... if 1:
... b'bytes'
...
>>> import dis; dis.dis(h)
3 0 LOAD_CONST 0 (None)
3 RETURN_VALUE |
|
Serhiy: "It looks to me that this optimization was added to avoid spending executing time for docstrings. Other cases almost never occur in real code and are not worth to be optimized. But the patch makes the code cleaner (it would even more cleaner if collapse all kinds of constants in Constant)." Oh, I don't really care of performance. The bytecode just doesn't make any sense to me. I don't understand why we load a constant. Maybe the compiler should emit a warning to say that the code doesn't make sense at all and is ignored? Example with GCC: $ cat x.c
int main()
{
1;
}$ gcc x.c -Wall -o x
x.c: In function 'main':
x.c:3:5: warning: statement with no effect [-Wunused-value]
1;
^ |
Oh ok, now I recall a similar issue that I posted 3 years ago: issue bpo-17516, "Dead code should be removed". Example of suspicious code: def func():
func2(),func() calls func2() and then create a tuple of 1 item with the func2() result. See my changeset 33bdd0a985b9 for examples in the Python source code. The parser or compiler should maybe help to developer to catch such strange code :-) In some cases, the code really contains code explicitly dead: def test_func():
return
do_real_stuff()do_real_stuff() is never called. Maybe it was a deliberate choice, maybe it was a mistake in a very old code base, bug introduced after multiple refactoring, and high turn over in a team? Again, we should emit a warning on such case? Hum, these warnings have a larger scope than this specific issue (don't emit LOAD_CONST for constants in expressions). See also the related thread on python-dev for the specific case of triple quoted strings ("""string"""): https://mail.python.org/pipermail/python-dev/2013-March/124925.html It was admitted that it's a convenient way to insert a comment and it must not emit a warning (at least, not by default?) |
vstinner
changed the title
|
+1 |
|
New changeset a0d053899ff8 by Victor Stinner in branch 'default': New changeset bcf27fa55632 by Victor Stinner in branch 'default': |
|
changeset: 100192:4bdb21380743 The compile ignores constant statements and emit a SyntaxWarning warning. Don't emit the warning for string statement because triple quoted string is a Don't emit the warning on ellipis neither: 'def f(): ...' is a legit syntax for Changes:
|
|
I changed my patch to emit a SyntaxWarning. If too many users complain of the warning, maybe we can remove it. IMHO it's useful to detect bugs. |
vstinner
changed the title
|
Shouldn't the message be "constant statement ignored"? The current wording reads strange to me. |
|
New changeset 15531b10976c by Victor Stinner in branch 'default': |
I removed the warning ;) |
|
I think the warning was helpful; it just had confusing wording. Instead of: """
>>> def f():
... False
...
<stdin>:2: SyntaxWarning: ignore constant statement
"""
perhaps: """
>>> def f():
... False
...
<stdin>:2: SyntaxWarning: ignoring constant statement
"""
or even: """
>>> def f():
... False
...
<stdin>:2: SyntaxWarning: ignoring unused constant 'False'
""" |
|
Sorry you are late :-) I started a thread on python-dev and it was decided |
|
FYI the thread was in February 2016: |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: