[Keviniscool-boy]

Summary

Fixes #5263

TypeGuard/TypeIs narrowing was not applied when the guarded argument is passed by keyword: the binding step only recorded a narrowing operation for a call with at least one positional argument, and attached it to args.args[0]. A keyword-only call such as is_str(x=x) therefore produced no narrowing operation at all, and the subject stayed str | None inside the positive branch. mypy and pyright both narrow whichever argument is bound to the guard's designated parameter, however it is passed.

Approach (reworked from the ground up on this round's review, three parts):

1. pyrefly/lib/alt/callable.rs — standard call resolution retains which actual argument supplies the designated parameter of the resolved TypeGuard/TypeIs result (GuardSubject, derived from the same ArgMap that callable_infer_params builds). The receiver of a method is skipped based on the function being a method (FuncDefId::cls, excluding staticmethods) — independently of how the receiver was supplied — so Guards.is_str(self=g, value=x) narrows x and never the receiver, and unbound positional calls no longer narrow the receiver either. When the information is unavailable (gradual signatures, lambdas), narrowing is declined rather than guessing.

2. pyrefly/lib/binding/narrow.rs — binding records one predicate per subject root holding the argument/facet alternatives of the call (candidates whose value expressions share a root share the entry; candidates of different calls are separate predicates). The old per-keyword AND-of-candidates is gone.

3. pyrefly/lib/alt/narrow.rs — the answers phase selects, before producing any facet-aware narrow and before applying polarity, the candidate that standard call resolution identifies as the guarded argument; it is applied through the facet-aware path (narrow_by_facet, extracted from the existing facet arm so the subject itself is filtered through its facet). Unselected candidates are absent effects, not operands to AND or OR, so both polarities narrow consistently — including the two-facets-of-one-object case (is_text(value=b.value, other=b.other)), where negative TypeIs narrowing was previously erased.

Performance: non-guard callees are rejected before any call resolution (zero resolutions for ordinary bool predicates), and the resolution result (narrowed type + guarded argument) is consumed directly for selection, narrowing and polarity — the call is resolved once per evaluation instead of twice.

Disclosure: this fix was prepared with AI assistance, as marked per AI_POLICY.md. The description above is LLM-generated; the human author will review the diff and complete the attestation below after doing so.

• I am a human, and I fully understand the code I am submitting.

Test Plan

• New testcases in pyrefly/lib/test/narrow.rs: keyword TypeGuard/TypeIs narrowing in both polarities, a keyword bound to a later parameter, a keyword on a positional-only parameter (declined, plus its unexpected-keyword error), bound-method receivers, unbound calls with an explicit receiver in both spellings (the receiver is never narrowed — g.upper() reports missing-attribute as on trunk), overloads (the resolved signature's subject narrows, the other signature's subject is untouched), facet subjects, while/assert, the single-predicate property (value=value, other_parameter=value narrows in both branches), and two facets of one object (is_text(value=b.value, other=b.other) narrows b.value in both branches and leaves b.other untouched).
• Semantics cross-checked against mypy 1.19/1.20-dev and pyright 1.1.414 on probe files, including the reviewer's reported cases.
• cargo test -p pyrefly --lib: 10695 passed, 0 failed (Linux, rustc 1.99.0), with two pre-existing trunk failures excluded and verified to fail identically on clean trunk.
• cargo clippy -p pyrefly: clean. cargo fmt --check: clean.
• The built checker reports the reviewer's cases correctly: checker.is_text(tag="example", value=x) narrows x; the unbound Guards.is_str(self=g, value=x) narrows x and reports g.upper() as missing-attribute; is_text(value=b.value, other=b.other) narrows b.value to str / int across branches.

[meta-cla]

Hi @Keviniscool-boy!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[rchen152]

@Keviniscool-boy Looks like we're missing a few things from you before we can review this PR: a signed CLA (see the meta-cla bot comment for instructions) and some of the checklist items here: https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request.

[meta-cla]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[github-actions]

✅ This PR meets all requirements from our pull request checklist.

Optional recommendations:

• This PR is very large, which makes it slow and difficult to review. We strongly recommend splitting it into smaller PRs that can be merged independently. If that isn't possible, please structure it as a series of small commits. If you've already split it as far as is practical, no action is needed.

Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour.

[stroxler]

Thanks for the PR!

I think this is broadly the right direction, it seems like it addreses the problem.

But based on an initial AI-assisted review, I think there are a combination of correctness and performance issues we need to improve before this can be merged:

Overload eligibility uses a different signature from return inference

Location: pyrefly/lib/alt/narrow.rs:149–164.

from typing import TypeGuard, Literal, overload

@overload
def guard(a: object, b: object, *, mode: Literal['a']) -> TypeGuard[str]: ...
@overload
def guard(b: object, a: object, *, mode: Literal['b']) -> TypeGuard[int]: ...
def guard(*args: object, **kwargs: object) -> bool:
    key = 'a' if kwargs.get('mode') == 'a' else 'b'
    value = kwargs[key] if key in kwargs else args[0]
    return isinstance(value, str if key == 'a' else int)

def unsafe(a: object, b: object) -> None:
    if guard(a=a, b=b, mode='b'):
        print(a + 1)

unsafe(None, 1)

The second overload's guard checks b, not a. Head nevertheless narrows both to int: keyword a qualifies through the first overload while the inferred guard result comes from the second. The isolated file produces zero errors at head; base diagnoses a + 1 on object. Python raises TypeError because a is None.

The implementation signature accepts the overloads without an inconsistent-overload diagnostic. This is a crafted case, but it is an actual soundness regression. Mypy and Pyright also choose an incorrect subject in this crafted ordering case; they are not used to validate the expected subject here. Runtime behavior and the guard's single-input meaning establish the problem.

Unbound keyword method call narrows self

Location: pyrefly/lib/alt/narrow.rs:119–148.

from typing import TypeGuard, assert_type

class Guard:
    def is_text(self, value: str | None) -> TypeGuard[str]:
        return value is not None

def unbound(c: Guard, value: str | None) -> None:
    if Guard.is_text(self=c, value=value):
        assert_type(c, Guard)
        assert_type(value, str)

At head, c becomes str and value remains str | None. At base, c stays Guard. The guard's result provides no evidence that its receiver is a string. The helper skips a receiver only for Type::BoundMethod; this unbound function path checks the literal first parameter named self.

Claude's blind unbound-positional example exposed a related preexisting subject-selection issue. This review charges only the newly introduced keyword-receiver narrowing to I1, not the unchanged positional issue.

Candidate collection destroys negative TypeIs narrowing

Location: pyrefly/lib/binding/narrow.rs:1346–1357.

from typing import TypeIs, assert_type

def text_is(value: str | None) -> TypeIs[str]:
    return value is not None

def check(value: str | None) -> None:
    if text_is(value=value):
        assert_type(value, str)
    else:
        assert_type(value, None)  # Head reports str | None.

Starting from an empty map, and_all seeds Placeholder AND Call. Negating the operation produces Placeholder OR NotCall; the unconstrained branch then prevents negative narrowing. This occurs with a single keyword, not just with multiple candidate subjects. The true branch of if not text_is(value=value) fails identically.

The new kw_is test expects the unchanged union in else, which masks the incomplete support. Candidate collection needs an insertion/merge operation appropriate to one predicate, without boolean placeholders.

No-narrow calls pay quadratic work

Location: the same per-keyword binder loop, plus and_all.

Even an ordinary bool predicate with keyword arguments is processed here. The full Arguments AST is cloned for every keyword before determining whether that value has a narrowable subject. Distinct name-valued arguments additionally accumulate placeholders and repeated accumulated-name scans. Base produced no guard-subject bindings for these calls.

Synthetic fixture: 200 repetitions of if ordinary(p0=x0, ..., pN=xN): pass, where ordinary has ordinary object parameters and returns bool. Three runs per size, median elapsed seconds, one checker thread, identical Buck dev configuration, same host. Base executable and changed library were transferred to that host; unchanged dependency libraries were reused, and baseline behavior was rechecked after transfer.

Keywords per call	Base	Head	Head/base
1	1.4647 s	1.4886 s	1.02×
8	1.5150 s	1.6516 s	1.09×
32	1.7290 s	2.5208 s	1.46×
128	2.5188 s	9.7637 s	3.88×

These timings illustrate the structural cost on an adversarial synthetic workload; they do not estimate real-project slowdown. Candidate discovery should avoid repeated full-call AST copies and placeholder expansion, including when guard/shape-related dependencies are absent.

[meta-codesync]

This pull request has been imported. If you are a Meta employee, you can view this in D131105783. (Because this pull request was imported automatically, there will not be any future comments.)

[stroxler]

Thanks!

I think this PR still has some structural issues.

Call argument-to-parameter mapping

It seems like it is trying to reconstruct the argument-to-parameter mapping using new logic and heuristics, but call semantics are pretty complex so we really want to reuse the standard logic.

When I say use the existing logic, I mean that Pyrefly already knows that, for example in

class Checker:
    def is_text(self, value: object, *, tag: str) -> TypeGuard[str]: ...

if checker.is_text(tag="example", value=x):
    ...

we need to associate

self  ← checker       # supplied by method binding
value ← x
tag   ← "example"

The guard's designated parameter is value, so its result constrains x. Keyword order and the spelling of the receiver are irrelevant.

A sketch of how we might implement this

callable_infer_params already matches actual arguments against formal parameters. Its named-keyword path finds the matching parameter. At that point, retain which actual argument supplies the guarded parameter. There is no need to expose a complete new parameter map just for this feature.

Conceptually, the extra result would be:

GuardEffect {
    argument: the actual argument bound to the guarded parameter,
    kind: TypeGuard or TypeIs,
    narrowed_type: the instantiated return-type argument,
}

This is proposed plumbing, not an existing struct. For overloads, the argument association and return type must come from the same call-resolution result. If applicable alternatives disagree on the subject, declining to narrow is fine.

There's exactly one narrow per call

During solving, only the candidate identified by the logic above contributes a narrow. Other candidates contribute nothing—they are not additional predicates to AND together and later negate.

The current logic appears to mishandle this, I think we might be treating each potentially-matched keyword as a separate narrow and combining them with AND, which isn't right because AND has specific consequences, for example when negated.

This shows up as wrong behavior in a case like this:

if is_type_guard_on_value(value=value, other_parameter=value):
    # narrowing works, because we AND'ed two checks on value
else:
   # whoops, negative narrowing does *not* work, because when we
   # negate it we get an OR. This is incorrect for TypeIs

[stroxler]

One more thing: be sure to request review when you're ready. Otherwise, the status stays tagged as waiting-for-author, and I'm likely to not look at it because we use the tags to help us skim the backlog

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

[Keviniscool-boy]

Thanks for the detailed review - both points are addressed in 16f007f, with the argument-to-parameter mapping now coming entirely from the standard call-resolution logic.

Call argument-to-parameter mapping. callable_infer_params now retains which actual argument supplies the designated parameter of a TypeGuard/TypeIs result: after the standard matching produces its ArgMap, a small helper derives a GuardSubject (the positional argument after the receiver, or the named keyword) by looking up the designated parameter in that mapping, and the result is returned alongside the call's type. The keyword path, receiver binding, and keyword order are whatever the standard matching did - e.g. for your checker.is_text(tag="example", value=x) example the mapping is self ← checker, value ← x, tag ← "example" and the result constrains x. No new parameter map is exposed, just the one association.

Overloads. The association comes from the same call-resolution result as the return type: per-candidate results carry their own GuardSubject and the one the call resolves to wins. When resolution is genuinely ambiguous (several applicable signatures with inconsistent results), no guarded argument is identified and nothing narrows, per your "declining to narrow is fine".

Exactly one narrow per call. Binding now records one operation per subject for a keyword-only call, grouping the keywords whose values share that subject into a single candidate set, instead of one AND-combined predicate per keyword. During solving, the call is resolved once through the ordinary call machinery (errors swallowed, as the existing TypeIs/TypeGuard narrowing already does) and only the candidate that standard resolution identifies contributes; the others contribute nothing in either polarity. Your is_type_guard_on_value(value=value, other_parameter=value) example now narrows value in both branches - the old code failed the negative branch exactly as you described, and I kept a testcase pinning it (test_typeguard_keyword_single_narrow_per_call), verified against both the new and the pre-fix commit.

One deliberate behavior change worth flagging: an unbound method call that supplies the receiver by keyword (Guards.is_str(self=g, x=x)) now narrows g, mirroring the positional case where the receiver is the first argument and the narrowed subject - previously the keyword was matched against the first parameter after a name-based self skip. The full suite passes (10301 tests) and clippy/fmt are clean.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[stroxler]

Thanks! I think this is improving.

Unfortunately it needs a rebase, there's already a merge conflict against trunk (not that unusual, sometimes Pyrefly moves pretty fast).

Since we have to rebase anyway before we can merge, I think it makes sense to keep improving the logic, it seems the issues we flagged are partially but not entirely fixed.

Here are the remaining concerns:

unbound keyword calls still narrow the receiver

pyrefly/lib/alt/callable.rs, new lines 950–955:

950     let designated =
951         params
952             .items()
953             .get(usize::from(has_self_arg))
954             .and_then(|param| match param {
955                 Param::Pos(name, _, _) | Param::PosOnly(Some(name), _, _) => Some(name),

has_self_arg describes an implicitly supplied receiver, not whether the function is a method. For Guard.is_text(self=c, value=value), it is false and the designated parameter becomes self. Accurately matching that wrong parameter through ArgMap still narrows the wrong object.

The new test explicitly locks in the earlier reported bug. pyrefly/lib/test/narrow.rs, new lines 1742–1748:

1742 # An unbound method call supplies the receiver by keyword, so the guard's designated
1743 # parameter is `self` itself - mirroring the positional case, where the receiver is the
1744 # first argument and is the narrowed subject.
1745 def kw_unbound(g: Guards, x: str | None) -> None:
1746     if Guards.is_str(self=g, x=x):
1747         assert_type(g, str)
1748         need_str(x)  # E: Argument `str | None` is not assignable to parameter `x` with type `str` in function `need_str`

Verified consequence: a guard checking value lets c.upper() pass with zero head errors, while the base reports that Guard has no upper. Calling it with Guard(), "text" raises AttributeError. The related preexisting positional bug does not justify introducing keyword unsoundness.

Suggested Fix: distinguish the designated formal parameter from the implicit receiver offset. Preserve the method's non-receiver designation independently of how the receiver is supplied; exclude static methods. If that information is unavailable, declining this narrowing is preferable to narrowing the receiver. Correct the test expectations rather than treating the bug as the contract.

different fields of one object still break negative TypeIs

pyrefly/lib/binding/narrow.rs, new lines 987–1002:

987             match self.0.entry(name) {
988                 Entry::Occupied(mut entry) => {
989                     let (op, _) = entry.get_mut();
990                     if let NarrowOp::Atomic(existing_prop, AtomicNarrowOp::Call(existing_call, kws)) =
991                         op
992                         && Arc::ptr_eq(existing_call, call)
993                         && Self::same_facet_subject(existing_prop.as_ref(), prop.as_ref())
994                     {
995                         if !kws.contains(&kw) {
996                             kws.push(kw.clone());
997                         }
998                     } else {
999                         op.and(NarrowOp::Atomic(
1000                            prop,
1001                            AtomicNarrowOp::Call(call.clone(), vec![kw.clone()]),
1002                        ));

For is_text(value=b.value, other=b.other), both candidates occupy the entry for b, but their facets differ. They take the op.and(...) branch. Negation ORs the active subtraction with the inactive candidate's unchanged TypeInfo, erasing negative narrowing.

Verified at head: with both fields declared str | int and is_text(value: object, other: object) -> TypeIs[str], b.value becomes str in the true branch but remains int | str in else, rather than int. Passing b.value twice narrows correctly in both branches. This is the same predicate-composition problem as round two, exposed by ordinary two-field code rather than identical-name arguments.

Suggested Fix: retain one call predicate per root with argument/facet alternatives. Select the guarded argument before producing the existing facet-aware atomic narrow and applying polarity. Unselected candidates are absent effects, not operands to AND or OR. Do not repair this with recursive candidate-tree deduplication. A smaller conservative boundary is to decline narrowing a root when its candidates refer to different facets; that sacrifices precision but avoids further grouping machinery.

ordinary calls still pay for repeated full call resolution

pyrefly/lib/alt/narrow.rs, new lines 707–718 checks keyword eligibility before rejecting non-guard callees. The eligibility helper then resolves the whole call:

pyrefly/lib/alt/narrow.rs, new lines 740–750:

740         let outcome = self.call_infer(
741             *call_target,
742             &args,
743             &keywords,
744             call.args.range(),
745             &swallowed_errors,
746             None,
747             None,
748             None,
749         );
750         if !matches!(outcome.ty, Type::TypeGuard(_) | Type::TypeIs(_)) {

N distinct solved subjects cause N resolutions of the full N-argument call, even for an ordinary bool predicate with no typing or shape dependencies. Arc fixes repeated AST copying, not repeated semantic work. The same-subject path also scans a growing keyword vector at lines 995–996 above. Guard-subject extraction itself is performed for ordinary calls in callable.rs.

For actual guards, the helper discards the resolved return type and returns only a boolean; the selected atomic guard then runs call_infer again to obtain that return type (new lines 2042–2120). The PR's “resolves the call once” claim is not true of this integration.

Suggested Fix: cheaply reject non-guard callees first and gate subject extraction on the return kind. More importantly, return and consume one solver-tracked resolved effect containing the subject and instantiated guard kind/type. Share that result across candidates and polarities through normal solver dependencies/invalidation—not a global AST cache. This also addresses finding 2 without adding more grouping helpers.

[Keviniscool-boy]

Thanks for the careful review - all three concerns are addressed in 15275ab (rebased onto trunk, history rewritten to a single commit as requested).

Unbound keyword calls no longer narrow the receiver. The designated parameter is now skipped based on the function being a method (FuncDefId::cls, excluding staticmethods) rather than on whether an implicit receiver was supplied, so Guards.is_str(self=g, value=x) narrows x and never g, in either keyword or positional spelling. When the method-ness information is unavailable (lambdas, gradual signatures), narrowing is declined instead of guessing. The baseline test that locked in the old behavior was corrected as suggested: g.upper() now reports missing-attribute exactly as on trunk, and the positional form no longer narrows the receiver either.

Two facets of one object no longer break negative TypeIs. Binding now records one predicate per subject root holding the argument/facet alternatives of the call, and the answers phase selects the candidate that standard call resolution identifies before producing the facet-aware narrow and applying polarity. Unselected candidates are absent effects in both polarities - no AND-composition, no candidate trees. is_text(value=b.value, other=b.other) now narrows b.value to str/int across branches while b.other stays untouched, pinned by a testcase; this is implemented as your suggested "one call predicate per root with argument/facet alternatives", not as a smaller conservative boundary, so no precision is sacrificed here.

No repeated full call resolution. resolve_narrowing_call now rejects non-guard callees before resolving anything (an ordinary bool predicate costs zero resolutions), and the single resolution's outcome - the instantiated guard type and the guarded argument - is consumed directly for selection, narrowing and polarity, instead of resolving the call a second time inside the guard application. Each subject's narrowing is computed once per solve and cached by the answer index as before.

Also rebased onto trunk as requested (the branch is now a single commit on top of the current trunk tip).

[stroxler]

Thanks for the changes @Keviniscool-boy!

This is coming along, unfortunately it's a pretty complex change, so it's taking more rounds of review than typical.

At the moment the biggest issue is that your last update has some debug logging we should get rid of, plus it has many thousands of lines of formatting changes. The formatting changes in particular are so big that I can't easily merge it (and almost any attempt to do so is very likely to fail to merge anyway).

Could you rebase and eliminate both debug logs and the large formatting changes in, for example, narrow.rs, call.rs, and callable.rs?

[Keviniscool-boy]

Done - thanks for pointing those out, and sorry about the noise.

The branch is rebased onto the current trunk tip (single commit, 64950aa). The PR diff is now ~944 additions / ~353 deletions across 7 files with no debug logging and no line-ending or formatting churn (the previous push accidentally contained a stray debug print in binding/narrow.rs and a wholesale line-ending flip in call.rs/callable.rs - both are gone).

On top of that, the rebase onto the newer trunk surfaced one more fix this change needed: call_overloads gained a parameter on trunk, so the dataclass caller was updated to pass receiver_offset: 0.

Re-verified on the rebased tree: the keyword-narrowing testcases, the two-facet negative narrowing, the unbound-receiver cases (receiver never narrowed, g.upper() reported), and the facet isinstance union-filtering tests all pass, alongside the rest of the suite.