Skip to content

binding: treat assert False as terminating control flow #603

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

Closed
wants to merge 1 commit into from

Conversation

Abel981
Copy link
Contributor

@Abel981 Abel981 commented Jun 29, 2025

This change makes Pyrefly recognize assert False as a control flow terminator, similar to raise. It adds a call to mark_flow_termination when the assert condition is statically known to be False.

closes #599

This change makes Pyrefly recognize `assert False` as a control flow terminator, similar to `raise`. It adds a call to `mark_flow_termination` when the assert condition is statically known to be `False`.
x = 10
assert False, "This assertion should cause control flow to terminate in this branch."
# This line should be unreachable.
raise Exception("Something unexpected happened here!")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how this test demonstrates the difference the code change makes.

Based on the sandbox, Pyrefly currently behaves the same way for this test https://pyrefly.org/sandbox/?code=MQAg6gpgNgxg9gWwiALnVALZAFAngJwgDMpcQBlAQwDsATAIzgA8AaTASwGcQuRKQADpQDmEAFCgA7lkIhccAK4gYNEJPzsUENpwyVZNWiCgR91PowUpUuAe2rCe5vCgxxqAOgkSQACQgGBsqKAiYgRKYoCoTccEQc3JyGjEwAXD4AtCAA4hDWhJRQGSjsSDYCyDBYMADW9o4REAyUtZkgAKqcyIQAbqZQAPoothAAFACUqOj2nBUw1vYR+IRGwxWcbRBMFRpI1NaSmhiYyACSACIAouGR0RDcrpQHyJwKAgJw+NajEB7CHnwrHB4AhQnltCBhHBitDaMR7Jp2O42G4+vg2HkYOM2mANFo+HQQPgFM5cK53ME4SBaOxCPNSE45Ip8CB6Pg4JIuvgxGIiOyEOV6jxQZ9rAAqHlw+JaTgoAYkgpVSj0EwDeBwgaUIhafCazhcuVEQpdCYgDIAPhAADl3BB0iAHSBQAAVDC8GXWNHsIjse6YJ6U5ClJA0p7QMhEOBQKAcoUAcko+oC1gAYsaIHGxI6nTxuPBqJx2HCViAFaYlSrkPQyHhCCRcHGHiNlNUagEvNmmCAALwgACMAAYs47EwaQGmoF02AAiV28UfJpHmXSKKBGFQKLrBfbsqDhGOSKaoAIIexhxmuXhsmhVDzT4cOl1u7hQewvNwKNes5Bllp6Ssdo6+CUFwyCXEwMAQAIJTuKM07kIgeRug4pbUFscxaEYejvBAaFYQEEAAITTuMQA

I would suggest marking the test function to return something like int, and seeing if Pyrefly complains about implicit None return.

For example, here:

https://pyrefly.org/sandbox/?code=MQAg6gpgNgxg9gWwiALnVALZAFAngJwgDMpcQBlAQwDsATAIzgA8AaTASwGcQuRKQADpQDmEAFCgA7lkIhccAK4gYNEJPzsUENpwyVZNWiCgR91PowUpUuAe2rCe5vCgxxqAOgkSQACQgGBsqKAiYgRKYoCoTccEQc3JyGjEwAXD4AtCAA4hDWhJRQGSjsSDYCyDBYMADW9o4REAyUtZkgAKqcyIQAbqZQAPoothAAFACUqOj2nBUw1vYR+IRGwxWcbRBMFRpI1NaSmhiYyACSACIAouGR0RDcrpQHyJwKAgJw+NajEB7CHnwrHB4AhQnltCBhHBitDaMR7Jp2O42G4+vg2HkYOM2mANFo+HQQPgFM5cK53ME4SBaOxCPNSE45Ip8CB6Pg4JIuvgxGIiOyEOV6jxQZ9rAAqHlw+JaTgoACMExAGQAfE4UOkQJqiZQuMhLkwYBABCV3BNJcRUPcUAAmRUqtUarWUThc6wAMUKXSAA

With your code change, the error in the second function will not be there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've patched the test and added handling in another place that was missed

@facebook facebook deleted a comment from github-actions bot Jul 8, 2025
@facebook-github-bot
Copy link
Contributor

@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D78165436.

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

Review automatically exported from Phabricator review in Meta.

@facebook-github-bot
Copy link
Contributor

@yangdanny97 merged this pull request in c83d376.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assert False is not cutting branches
4 participants