Skip to content

bpo-1875: Raise SyntaxError in invalid blocks that will be optimized away #13332

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.

Sign up for GitHub

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

Jump to bottom
Merged
pablogsal merged 1 commit into from
May 17, 2019
Merged

bpo-1875: Raise SyntaxError in invalid blocks that will be optimized away #13332

pablogsal merged 1 commit into from
May 17, 2019

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented May 15, 2019 •
edited by bedevere-bot
Loading

@pablogsal pablogsal changed the title bpo-1875: Raise SyntaxError in blocks that will be optimized away bpo-1875: Raise SyntaxError in invalid blocks that will be optimized away May 15, 2019
@pablogsal pablogsal force-pushed the bpo-1875 branch 2 times, most recently from 770a939 to 9e8f83c Compare May 15, 2019 00:33
@pablogsal
bpo-1875: Raise SyntaxError in invalid blocks that will be optimized …
09ec749 
…away

Move the check for dead conditionals (if 0) to the peephole optimizer
and make sure that the code block is still compiled to report any
existing syntax errors within.
@vstinner
Copy link
Member

vstinner commented May 15, 2019

I wrote similar code in my old fatoptimizer project (AST optimizer):
https://github.com/vstinner/fatoptimizer/blob/master/test_fatoptimizer.py

Extracts:

        # comparison between bytes and str can raise BytesWarning depending
        # on runtime option
        self.check_dont_optimize_func('"x" == b"x"')
        self.check_dont_optimize_func('b"x" == "x"')
        self.check_dont_optimize_func('"x" != b"x"')
        self.check_dont_optimize_func('b"x" != "x"')

        # bytes < str raises TypeError
        self.check_dont_optimize_func('b"bytes" < "str"')

        self.check_dont_optimize("""
            def func():
                if 0:
                    yield
        """)

        self.check_dont_optimize("""
            def func(obj):
                return
                if 0:
                    yield from obj
        """)

        self.check_dont_optimize("""
            try:
                pass
            except Exception:
                yield 3
        """)

        # must raise SyntaxError
        self.check_dont_optimize("""
            for x in (1, 2):
                try:
                    pass
                except Exception:
                    func2()
                finally:
                    # continue is final is illegal
                    continue
        """)

        self.check_dont_optimize("""
            for x in (1, 2):
                try:
                    pass
                except Exception:
                    func2()
                else:
                    func3()
                finally:
                    # continue is final is illegal
                    continue
        """)

        self.check_dont_optimize("""
            for x in (1, 2):
                try:
                    pass
                except Exception:
                    try:
                        func2()
                    finally:
                        # continue is final is illegal
                        continue
        """)

        self.check_dont_optimize("""
            if test:
                pass
        """)

It would be nice to ensure that our optimizer doesn't optimize these examples.

@pablogsal
Copy link
Member Author

pablogsal commented May 15, 2019 •
edited
Loading

@vstinner Ok, I like to add these tests but let's open a different issue for those.

Note that some of you examples are now legal after https://bugs.python.org/issue32489

@vstinner
Copy link
Member

vstinner commented May 15, 2019

When I wrote an AST optimizer, I implemented correctness checks to ensure that I don't remove code that would raise a SyntaxError:
https://github.com/vstinner/fatoptimizer/blob/master/fatoptimizer/dead_code.py#L7

# AST types of nodes that cannot be removed
_CANNOT_REMOVE_TYPES = (ast.Global, ast.Nonlocal, ast.Yield, ast.YieldFrom,
                        # don't remove 'except' block if it contains continue
                        # or break: see can_move_final() for the rationale
                        ast.Continue)

_CANNOT_MOVE_FINAL = (ast.Continue,)

My implementation checks if a tree contains yield (for example) before trying to remove it. If it contains yield, global, etc.: the node is not removed.

Maybe we should implement that in the new builtin AST optimizer, rather than in peephole.c which works at the bytecode level?

@vstinner
Copy link
Member

vstinner commented May 15, 2019

Hum, my previous comment is unclear. I don't understand how your PR still removes "if 0: print(name)" but keeps "if 0: yield". Or it doesn't?

@pablogsal
Copy link
Member Author

pablogsal commented May 15, 2019

Hum, my previous comment is unclear. I don't understand how your PR still removes "if 0: print(name)" but keeps "if 0: yield". Or it doesn't?

It compiles the block (raising errors if any are found) and moves the removal optimization to the peephole optimizer. The only downside (if you consider any) is that the variables inside the dead code will still be considered when building the code stack size and some jumps may not be optimized. But this is a no-problem for three reasons:

  1. We would be prioritizing speed over correctness.
  2. (More important) This only happens when you have dead code that can be detected statically. Slightly unoptimized function compilation and a slightly bigger stack when you have dead code are by far not the biggest problem. The biggest problem is the dead code so I think we are ok if the slightly worst code is generated if dead code blocks are present.
  3. This situation (a dead code block with a falsy constant in the condition) happens extremely rare, so it should not even be a concern. This PR is about correctness more than anything.

@pablogsal pablogsal self-assigned this May 15, 2019
vstinner
vstinner approved these changes May 15, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

First I misunderstood the change. Once I understood it: yeah, it's correct to move an optimization from the compiler to the (peephole) optimizer. First, I didn't understood why a compiler directly implements an optimization.

@pablogsal pablogsal merged commit af8646c into python:master May 17, 2019
@pablogsal pablogsal deleted the bpo-1875 branch May 17, 2019 10:37
@miss-islington
Copy link
Contributor

miss-islington commented May 17, 2019

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 17, 2019
@pablogsal @miss-islington
bpo-1875: Raise SyntaxError in invalid blocks that will be optimised …
813aa59 
…away (pythonGH-13332)

Move the check for dead conditionals (if 0) to the peephole optimizer
and make sure that the code block is still compiled to report any
existing syntax errors within.
(cherry picked from commit af8646c)

Co-authored-by: Pablo Galindo <[email protected]>
@bedevere-bot
Copy link

bedevere-bot commented May 17, 2019

GH-13382 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request May 17, 2019
@miss-islington @pablogsal
bpo-1875: Raise SyntaxError in invalid blocks that will be optimised …
85ed171 
…away (GH-13332)

Move the check for dead conditionals (if 0) to the peephole optimizer
and make sure that the code block is still compiled to report any
existing syntax errors within.
(cherry picked from commit af8646c)

Co-authored-by: Pablo Galindo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pablogsal @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni