Fix if-branch pruning for Final bool constants#3571
Conversation
|
The idea here makes sense, but the CI failures suggest this change may be leaking beyond Final-declared constants into general boolean control-flow inference. I’d double-check the branch pruning condition — it should ideally only trigger on Final-backed literals, not inferred constant truthiness. Might be worth adding a regression test for: non-Final bool literal assignment to ensure no accidental over-pruning. |
d6dcf90 to
c902270
Compare
This comment has been minimized.
This comment has been minimized.
Prune the correct if-branch when the condition is a `Final` bool constant by narrowing control flow based on the constant's value. Track `Final` bool literal values (`FLAG: Final = False`) alongside the existing `Final` string values, and consult them in the branch-pruning path so `if FLAG:` / `else` branches are statically resolved. Also make the implicit-return scan consistent with this pruning. The function-body scan that finds the trailing statement(s) for implicit-return analysis (`function_last_expressions`) previously pruned only `sys_info` branches. Once the binding traversal also prunes `Final` bool branches, the scan could record a trailing statement inside a branch that never gets bound, producing a dangling `Idx<Key>` and a "key lacking binding" panic at solve time (observed via mypy_primer on `mypy`'s `mypyc/codegen/emit.py`). The scan now prunes `Final` bool branches the same way, keeping it in lockstep with the traversal.
c902270 to
9efd9c0
Compare
|
Diff from mypy_primer, showing the effect of this PR on open source code: apprise (https://github.com/caronc/apprise)
+ ERROR apprise/plugins/vapid/__init__.py:356:20-43: Object of class `dict` has no attribute `load` [missing-attribute]
+ ERROR apprise/plugins/vapid/__init__.py:391:16-22: `not in` is not supported between `object` and `WebPushSubscriptionManager` [unsupported-operation]
+ ERROR apprise/plugins/vapid/__init__.py:405:28-65: Argument `EllipticCurvePublicKey | Unknown | None` is not assignable to parameter `public_key` with type `EllipticCurvePublicKey` in function `apprise.utils.pem.ApprisePEMController.encrypt_webpush` [bad-argument-type]
+ ERROR apprise/plugins/vapid/__init__.py:405:47-53: Cannot index into `WebPushSubscriptionManager` [bad-index]
+ ERROR apprise/plugins/vapid/__init__.py:406:29-67: Argument `bytes | Unknown | None` is not assignable to parameter `auth_secret` with type `bytes` in function `apprise.utils.pem.ApprisePEMController.encrypt_webpush` [bad-argument-type]
+ ERROR apprise/plugins/vapid/__init__.py:406:48-54: Cannot index into `WebPushSubscriptionManager` [bad-index]
+ ERROR apprise/plugins/vapid/__init__.py:609:42-70: Argument `bytes | None` is not assignable to parameter `data` with type `bytes` in function `apprise.utils.base64.base64_urlencode` [bad-argument-type]
|
|
@connernilsen has imported this pull request. If you are a Meta employee, you can view this in D113574404. |
connernilsen
left a comment
connernilsen
left a comment
There was a problem hiding this comment.
Hey @LeSingh1, thanks for working on this! Here are a few questions/recommendations real quick
| print("pruned and last") | ||
| "#, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Can we add a test to validate that a closer, non-final bool definition doesn't prune branches too early?
Probably something like this:
from typing import Final
flag: Final = True
# this should hit that early return case where a local scope might override a more global scope
def foo(flag: bool):
if flag:
bar = 1
else:
bar = 2
reveal_type(bar) # E: revealed type: Literal[1] | Literal[2] (I'm not sure if this would be literal or collapse to just `int`)| testcase!( | ||
| test_final_bool_if_branch_implicit_return_no_panic, | ||
| r#" | ||
| from typing import Final | ||
| FLAG_FALSE: Final = False | ||
| FLAG_TRUE: Final = True | ||
| def prune_if() -> None: | ||
| if FLAG_FALSE: | ||
| print("pruned") | ||
| else: | ||
| print("kept") | ||
| def prune_else() -> None: | ||
| if FLAG_TRUE: | ||
| print("kept") | ||
| else: | ||
| print("pruned") | ||
| def prune_if_no_else() -> None: | ||
| if FLAG_FALSE: | ||
| print("pruned and last") | ||
| "#, | ||
| ); |
There was a problem hiding this comment.
Add one more check in this test for function-level finals, since those might crash at runtime due to the branch being dropped
def prune_if_no_else_local() -> None:
FLAG_FALSE: Final = False
if FLAG_FALSE
print("pruned and last")
def prune_else_local() -> None:
FLAG_FALSE: Final = False
if FLAG_FALSE
print("pruned")
else:
print("kept")
def prune_if_local() -> None:
FLAG_TRUE: Final = False
if FLAG_TRUE
print("kept")
else:
print("pruned")You may need to fix this by computing implicit_return after the function scope's static state is initialized (inside or after init_static_scope) so both function_last_expressions -> evaluate_bool_for_control_flow and stmt.rs:1265 see the same final_bool_values.
| /// used to resolve Final variable references in synthesized class field names. | ||
| pub final_names: SmallMap<Name, Option<String>>, | ||
| /// Names marked `Final` with bool literal values, e.g. `FLAG: Final = False`. | ||
| /// Used to statically evaluate `if FLAG:` / `if not FLAG:` control flow. |
There was a problem hiding this comment.
You might need to fix the evaluator here (or add a test validating that it does this). I don't think the current logic can handle not FLAG, since we only special case a bare Expr::Name below.
4 participants
Summary
Fixes #3537.
When a variable is declared
Finaland assigned a bool literal (FalseorTrue), control-flow tests likeif flag:now prune unreachable branches the same way as literalif False:/if True:.Before this change, both arms of an
if/elsecould contribute to merged types even when the condition was statically known to be always false viaFinal = False.Test plan
test_final_bool_unreachable_if_branch—Final = False→ else-only assignment typetest_final_bool_unreachable_else_branch—Final = True→ if-only assignment typecargo test test_final_bool_unreachable