fix: prevent hints from poisoning generic function inference in OR expressions #1647
Conversation
…pressions Previously, in expressions like 'x = x or get_value(default)', the type of 'x' (e.g., Optional[str]) was passed as a hint to 'get_value'. This caused generic functions to widen their return type to match the hint (Optional[str]) rather than inferring the specific type from arguments (str). This change drops the type hint for Call expressions on the right-hand side of an OR operator. This allows functions to infer their return type strictly from arguments, while preserving hints for literals (like []) where context is needed. Fixes facebook#1635
stroxler
left a comment
stroxler
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Both the root cause and the fix look good to me, I'll try to get this merged today
| } | ||
| }); | ||
| let mut t = self.expr_infer_with_hint(value, hint, errors); | ||
| let operand_hint = if matches!(op, BoolOp::Or) && matches!(value, Expr::Call(_)) { |
There was a problem hiding this comment.
I'm not convinced this is specific to or; for example if we had
x: None | bool | str
and then
x = (not x) and get_value("default")
then we probably should be able to narrow x to bool | str, but the existing logic would pass down the hint and we'd get None | bool | str.
More generally, I think what might really be going on is that return types shouldn't be used contextually in the way they are now, because (using your test case as an example)
config = get_value("default")
really ought to result in config being narrowed to str, and it's not even in a bool op - I think the real issue is that we should only be using context when it's necessary to make the assignment legal; in cases where we can get a narrower type we want the narrower type
cc @samwgoldman for thoughts, it's unclear to me how hard this would be to do. We might be able to just analyze the function twice when necessary, similar to how we handle overloads
There was a problem hiding this comment.
Is this just #881? Agree that the way we contextually type calls to generic functions (and ctors) is wrong. My plan was to treat these hints differently, but I only have a sketch of an idea.
If this PR is working around a specific instance of 881 I think we should probably work on the underlying issue instead.
There was a problem hiding this comment.
Ah, yeah I forgot about #881, I think this probably is just another case of it.
After giving it more thought, it seems like if the constraint solver understood that a return type hint is only an upper bound (and should not affect the result unless necessary) then we'd get the right answer.
I'm guessing that's at least roughly what your idea is?
I do think it's likely possible to use two attempts at solving the call to get this behavior, similar to overloads. But assuming the solver can do it natively in one pass that seems better
stroxler
left a comment
stroxler
left a comment
There was a problem hiding this comment.
Marking as changes requested since I think we likely want a different approach.
For what it's worth I think the diagnosis is right and the fix works, but we probably want a broader solution than this.
3 participants
Summary
Fixes #1635.
This PR fixes a type inference bug where generic functions (like
os.getenv) returned incorrectly widened types when used on the right-hand side of anorexpression.The Issue:
When reassigning a variable like
config: str | None, the type checker passed the variable's current type (str | None) as a "context hint" to the right-hand side of theor. Generic functions likeos.getenv(key, default)accepted this hint, widening their return type to match the hint (inferringTasNone) rather than inferring the specific type from their arguments (default="string"->T=str).This resulted in false positive type errors (e.g.,
Argument 'str | None' is not assignable...) even when a valid default value was provided.The Fix
I updated
boolopinexpr.rsto adjust how hints are propagated inORexpressions:Expr::Call): The context hint is now dropped. This forces the function to infer its return type strictly from its arguments (Inside-Out inference), preventing "poisoning" from the surrounding context.x: List[int] = None or []still correctly inferList[int]instead ofList[Any].Test Plan
Added a regression test
test_or_generic_function_hint_poisoning_fixinoperators.rs.The test simulates the
os.getenvbehavior using a genericidentityfunction and verifies that:str | Noneset toNone.variable or identity("default").str(proving the hintstr | Nonewas ignored by the function inference).Verification:
Ran tests locally:
cargo test -p pyrefly --lib test_or_generic_function_hint_poisoning_fix(Passed)