Skip to content
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

Incorrect constant folding for list comprehension with always-false condition #1920

Closed
asottile opened this issue Oct 8, 2017 · 3 comments
Closed

Comments

@asottile
Copy link

asottile commented Oct 8, 2017

Originally found here

Reproduction:

# test.py
assert not [l for l in [1] if False], "FAIL"
$ python test.py  # succeeds
$ cython -3 --embed test.py && gcc -I/usr/include/python3.5m -O3 -o test test.c -lpython3.5m -lm -lutil -ldl && ./test
Traceback (most recent call last):
  File "test.py", line 1, in init test
    assert not [l for l in [1] if False], "FAIL"
AssertionError: FAIL

I've traced the problematic code to

def visit_IfStatNode(self, node):
self.visitchildren(node)
# eliminate dead code based on constant condition results
if_clauses = []
for if_clause in node.if_clauses:
condition = if_clause.condition
if condition.has_constant_result():
if condition.constant_result:
# always true => subsequent clauses can safely be dropped
node.else_clause = if_clause.body
break
# else: false => drop clause
else:
# unknown result => normal runtime evaluation
if_clauses.append(if_clause)
if if_clauses:
node.if_clauses = if_clauses
return node
elif node.else_clause:
return node.else_clause
else:
return Nodes.StatListNode(node.pos, stats=[])

Notably: line 4204 is False so the condition gets dropped

I think to properly prune this, the IfStatNode shouldn't be treated as an if statement (which this code seems to do) and specialized code in visit_ComprehensionNode should handle this. Though I'm not familiar with the codebase at all :)

@BrenBarn
Copy link

BrenBarn commented Oct 8, 2017

Your output shows a not in the assert but your example file has no not.

@asottile
Copy link
Author

asottile commented Oct 8, 2017

@BrenBarn indeed, I retyped it in the issue instead of copy pasting, I've fixed the description.

@scoder
Copy link
Contributor

scoder commented Oct 10, 2017

Interesting, thanks for the report. It works when I say return not [...], but not assert not [...] or if not [...].

@scoder scoder closed this as completed in 2ad640d Oct 10, 2017
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

No branches or pull requests

3 participants