Fix false positive on callables with non-constant parameter mappings

crates/pyrefly_types/src/callable.rs

@@ -895,6 +895,16 @@ impl Param {
         }
     }
 
+    /// Convert a positional-or-keyword parameter to keyword-only.
+    pub fn passed_by_name(&self) -> Self {
+        match self {
+            Param::Pos(name, ty, required) => {
+                Param::KwOnly(name.clone(), ty.clone(), required.clone())
+            }
+            param => param.clone(),
+        }
+    }
+
     pub fn is_required(&self) -> bool {
         match self {
             Param::PosOnly(_, _, Required::Required)

pyrefly/lib/alt/functools.rs

@@ -626,8 +626,8 @@ fn partial_params(
         // A positional can't follow a bound keyword, so demote later positionals to keyword-only.
         if matches!(remaining[idx], Param::Pos(..)) {
             for later in remaining.iter_mut().skip(idx + 1) {
-                if let Param::Pos(n, t, r) = later {
-                    *later = Param::KwOnly(n.clone(), t.clone(), r.clone());
+                if matches!(later, Param::Pos(..)) {
+                    *later = later.passed_by_name();
                 }
             }
         }

pyrefly/lib/solver/solver.rs

@@ -603,6 +603,13 @@ impl SubsetWithSnapshotResult {
     pub fn is_ok(&self) -> bool {
         matches!(self, SubsetWithSnapshotResult::Ok)
     }
+
+    pub fn into_result(self) -> Result<(), SubsetError> {
+        match self {
+            SubsetWithSnapshotResult::Ok => Ok(()),
+            SubsetWithSnapshotResult::Err(e) => Err(e),
+        }
+    }
 }
 
 impl Solver {

pyrefly/lib/solver/subset.rs

@@ -41,6 +41,7 @@ use pyrefly_types::typed_dict::TypedDictField;
 use pyrefly_types::types::Forall;
 use pyrefly_types::types::Overload;
 use pyrefly_types::types::OverloadType;
+use pyrefly_types::types::Var;
 use pyrefly_util::owner::Owner;
 use pyrefly_util::visit::Visit;
 use ruff_python_ast::name::Name;
@@ -239,32 +240,64 @@ impl<'solver, 'subset, Ans: LookupAnswer> Subset<'solver, 'subset, Ans> {
         self.is_subset_eq(unpack, &accepted)
     }
 
-    /// Can a function with l_args be called as a function with u_args?
+    /// Can a function with `l_args` be called as a function with `u_args`?
     fn is_subset_param_list(
         &mut self,
         l_args: &[Param],
         u_args: &[Param],
         l_gradual: bool,
         u_gradual: bool,
     ) -> Result<(), SubsetError> {
-        // Don't short-circuit because we may want to pin/solve variables
-        let result = self.is_subset_param_list_impl(l_args, u_args);
-        match result {
-            Err(_) if !self.solver.config.strict_callable_subtyping && (l_gradual || u_gradual) => {
-                Ok(())
-            }
-            _ => result,
+        // Run the walk before the fallback below, because it may pin/solve variables.
+        let Err(pairwise_error) = self.is_subset_param_list_single_mapping(l_args, u_args) else {
+            return Ok(());
+        };
+        if !self.solver.config.strict_callable_subtyping && (l_gradual || u_gradual) {
+            return Ok(());
+        }
+        // With no positional-or-keyword parameters there is only one mapping, already checked.
+        if !u_args.iter().any(|param| matches!(param, Param::Pos(..))) {
+            return Err(pairwise_error);
         }
+        // The splits are speculative, so a `Var` pinned by one that then fails must be rolled back.
+        let vars: Vec<Var> = l_args
+            .iter()
+            .chain(u_args)
+            .flat_map(|param| param.as_type().collect_maybe_placeholder_vars())
+            .unique()
+            .collect();
+
+        self.with_snapshot(&vars, |me| {
+            let mut u_split: Vec<Param> = u_args.iter().map(Param::passed_by_name).collect();
+            // A failing all-by-name split reports `pairwise_error`, which diagnoses the
+            // signatures as written; later splits report their own, more specific failure.
+            me.is_subset_param_list_single_mapping(l_args, &u_split)
+                .map_err(|_| pairwise_error)?;
+
+            // Move the boundary right one parameter at a time; walk `u_args`,
+            // so parameters that were keyword-only are never made positional.
+            for (i, param) in u_args.iter().enumerate() {
+                if let Param::Pos(name, ty, required) = param {
+                    u_split[i] = Param::PosOnly(Some(name.clone()), ty.clone(), required.clone());
+                    me.is_subset_param_list_single_mapping(l_args, &u_split)?;
+                }
+            }
+            Ok(())
+        })
+        .into_result()
     }
 
-    /// Can a function with l_args be called as a function with u_args?
-    fn is_subset_param_list_impl(
+    /// Can a function with `l_args` be called as a function with `u_args`, under the single
+    /// mapping that passes each of `u_args`'s positional-or-keyword parameters positionally?
+    fn is_subset_param_list_single_mapping(
         &mut self,
         l_args: &[Param],
         u_args: &[Param],
     ) -> Result<(), SubsetError> {
-        let mut l_args = l_args.iter();
-        let mut u_args = u_args.iter();
+        let l_params = l_args;
+        let u_params = u_args;
+        let mut l_args = l_params.iter();
+        let mut u_args = u_params.iter();
         let mut l_arg = l_args.next();
         let mut u_arg = u_args.next();
         // This holds any Param::Pos from `u` that matched *args from `l`.
@@ -500,7 +533,9 @@ impl<'solver, 'subset, Ans: LookupAnswer> Subset<'solver, 'subset, Ans> {
                     l_keywords.insert(name.clone(), (ty.clone(), *required == Required::Required));
                 }
                 Param::Kwargs(_, ty) => l_kwargs = Some(ty.clone()),
-                _ => (),
+                // A leftover positional-only parameter cannot be filled by name.
+                Param::PosOnly(_, _, Required::Required) => return Err(SubsetError::Other),
+                Param::PosOnly(_, _, Required::Optional(_)) | Param::Varargs(..) => (),
             }
         }
         let mut u_keywords = SmallMap::new();
@@ -511,7 +546,16 @@ impl<'solver, 'subset, Ans: LookupAnswer> Subset<'solver, 'subset, Ans> {
                     u_keywords.insert(name.clone(), (ty.clone(), *required == Required::Required));
                 }
                 Param::Kwargs(_, ty) => u_kwargs = Some(ty.clone()),
-                _ => (),
+                // A by-name split can leave `u`'s `*args` after the keyword block, where
+                // no argument can reach it; that needs every earlier parameter passed
+                // positionally, which is the final split.
+                Param::Varargs(..) => (),
+                Param::PosOnly(..) | Param::Pos(..) => {
+                    return Err(SubsetError::InternalError(
+                        "positional parameter of `want` left unconsumed by the positional loop"
+                            .to_owned(),
+                    ));
+                }
             }
         }
         let object_type = self
@@ -589,6 +633,28 @@ impl<'solver, 'subset, Ans: LookupAnswer> Subset<'solver, 'subset, Ans> {
             }
             (l_kwargs, _) => l_kwargs,
         };
+        // Only the explicit positional prefix is certainly filled. An unpacked `*args` may be
+        // empty even when the positional walk traversed optional parameters from `l`.
+        if l_params
+            .iter()
+            .zip(u_params)
+            .take_while(|(l, u)| {
+                matches!(l, Param::PosOnly(..) | Param::Pos(..))
+                    && matches!(u, Param::PosOnly(..) | Param::Pos(..))
+            })
+            .filter_map(|(l, _)| match l {
+                Param::Pos(name, ..) => Some(name),
+                _ => None,
+            })
+            .any(|l_name| {
+                u_keywords.contains_key(l_name)
+                    || u_param_matched_with_l_varargs
+                        .iter()
+                        .any(|(u_name, _)| *u_name == l_name)
+            })
+        {
+            return Err(SubsetError::Other);
+        }
         // These parameters from `u` may be passed by name or position. We matched the positional
         // case with *args from `l` already; now we check that they can be passed by name.
         for (name, u_ty) in u_param_matched_with_l_varargs {
@@ -1609,10 +1675,8 @@ impl<'solver, 'subset, Ans: LookupAnswer> Subset<'solver, 'subset, Ans> {
                         want,
                     );
                     let vars = fresh_forall.handle.vars().to_vec();
-                    match self.with_snapshot(&vars, |me| me.is_subset_forall(fresh_forall, want)) {
-                        SubsetWithSnapshotResult::Ok => Ok(()),
-                        SubsetWithSnapshotResult::Err(e) => Err(e),
-                    }
+                    self.with_snapshot(&vars, |me| me.is_subset_forall(fresh_forall, want))
+                        .into_result()
                 }
             })
             .is_ok(),

pyrefly/lib/test/callable.rs

@@ -2368,3 +2368,156 @@ class C:
         self.callback: Callable[[int], int] = lambda value: value + 1
     "#,
 );
+
+// `Want(1, 2)` fills `got`'s `pos` and `lower`, while `Want(lower=1, upper=2)` fills its
+// `lower` and `upper`: a non-constant mapping. See https://github.com/python/typing/issues/2224.
+testcase!(
+    test_non_constant_param_mapping,
+    r#"
+from typing import Protocol
+
+class Want(Protocol):
+    def __call__(self, lower: int, upper: int) -> None: ...
+
+def got(pos: str | int | None = None, /, lower: int | None = None, upper: int | None = None) -> None: ...
+ok: Want = got
+
+# `Want(1, upper=2)` may use `upper` both as a positional-only name and in `**kwargs`.
+def posonly_collision(upper: int = 0, /, lower: int = 0, **kwargs: int) -> None: ...
+ok_posonly: Want = posonly_collision
+    "#,
+);
+
+// An empty unpack does not fill an optional positional parameter before a later keyword.
+testcase!(
+    test_non_constant_param_mapping_empty_unpack,
+    r#"
+from typing import Protocol, Unpack
+
+class Want(Protocol):
+    def __call__(self, *args: Unpack[tuple[()]], b: int) -> None: ...
+
+def got(b: int = 0, **kwargs: int) -> None: ...
+ok: Want = got
+    "#,
+);
+
+// Each of these rejects at least one call form permitted by `Want`.
+testcase!(
+    test_non_constant_param_mapping_negative,
+    r#"
+from typing import Protocol
+
+class Want(Protocol):
+    def __call__(self, lower: int, upper: int) -> None: ...
+
+# `Want(1, 2)` passes `lower` to a slot that rejects `int`.
+def bad_pos_type(pos: str | None = None, /, lower: int | None = None, upper: int | None = None) -> None: ...
+e1: Want = bad_pos_type  # E: is not assignable to `Want`
+
+# `Want(lower=1, upper=2)` has no `lower` to pass by name.
+def no_lower_keyword(lower_pos: int | None = None, /, upper: int | None = None) -> None: ...
+e2: Want = no_lower_keyword  # E: is not assignable to `Want`
+
+# `Want(lower=1, upper=2)` leaves the required `pos` unfilled.
+def required_pos(pos: int, /, lower: int | None = None, upper: int | None = None) -> None: ...
+e3: Want = required_pos  # E: is not assignable to `Want`
+
+# `Want(1, upper=2)` would pass two values for `upper`.
+def swapped(upper: int | None = None, lower: int | None = None) -> None: ...
+e4: Want = swapped  # E: is not assignable to `Want`
+
+# `Want(1, upper=2)` passes `upper` both positionally and by name; `**kwargs` cannot absorb it.
+def duplicate_binding(upper: int = 0, lower: int = 0, **kwargs: int) -> None: ...
+e5: Want = duplicate_binding  # E: is not assignable to `Want`
+    "#,
+);
+
+// Parameter names need not match: `Want(1)` fills `b`, and `Want(a=1)` lands in `**kwargs`.
+testcase!(
+    test_non_constant_param_mapping_kwargs,
+    r#"
+from typing import Protocol
+
+class Want(Protocol):
+    def __call__(self, a: int) -> None: ...
+
+def got(b: int = 0, **kwargs: int) -> None: ...
+ok: Want = got
+
+# Without `**kwargs`, `Want(a=1)` has nowhere to go.
+def no_kwargs(b: int = 0) -> None: ...
+e: Want = no_kwargs  # E: is not assignable to `Want`
+    "#,
+);
+
+// A split that fails must not leak the type variables it pinned on the way: `T` is solved by
+// `v`, not by the `**kwargs: str` that the by-name split matched `a` against.
+testcase!(
+    test_non_constant_param_mapping_no_var_leak,
+    r#"
+from typing import Protocol, assert_type
+
+class Want[T](Protocol):
+    def __call__(self, a: T, b: int) -> None: ...
+
+def use[T](w: Want[T], v: T) -> T: ...
+
+def got(x: int = 0, **kwargs: str) -> None: ...
+assert_type(use(got, 1), int)  # E: is not assignable to parameter `w`
+    "#,
+);
+
+// A name mismatch is only the reason for failure when the by-name call form is what fails.
+testcase!(
+    test_non_constant_param_mapping_error_cause,
+    r#"
+from typing import Protocol
+
+class Want(Protocol):
+    def __call__(self, a: int, b: int) -> None: ...
+
+# `Want(a=1, b=2)` and `Want(1, b=2)` both work, so the reason is `Want(1, 2)` passing an `int`
+# to `y: str`, not the `x`/`a` mismatch.
+def late_type_error(x: int = 0, y: str = "", **kwargs: int) -> None: ...
+e1: Want = late_type_error  # E: is not assignable to `Want` # !E: Positional parameter name mismatch
+
+# `Want(a=1, b=2)` cannot be satisfied at all, so the mismatch is the reason.
+def swapped_names(b: int = 0, a: str = "") -> None: ...
+e2: Want = swapped_names  # E: Positional parameter name mismatch: got `b`, want `a`
+    "#,
+);
+
+// `Want`'s callers pass only keywords, so the required `pos` can never be filled.
+testcase!(
+    test_unmatched_required_pos_only,
+    r#"
+from typing import Protocol
+
+class Want(Protocol):
+    def __call__(self, *, lower: int, upper: int) -> None: ...
+
+def got(pos: int, /, lower: int | None = None, upper: int | None = None) -> None: ...
+e: Want = got  # E: is not assignable to `Want`
+    "#,
+);
+
+// A by-name split strands `Want`'s `*args` where no argument can reach it, which
+// is harmless because the split that passes `a` positionally still checks `*args`.
+testcase!(
+    test_split_strands_varargs,
+    r#"
+from typing import Protocol
+
+class Want(Protocol):
+    def __call__(self, a: int, *args: int) -> None: ...
+
+# `Want(a=1)` reaches `**kwargs`, and `Want(1, 2)` reaches `b` and `*args`.
+def got(b: int = 0, *args: int, **kwargs: int) -> None: ...
+ok: Want = got
+
+# `Want(1, 2)` passes an `int` to `*args: str`.
+def bad_varargs(b: int = 0, *args: str, **kwargs: int) -> None: ...
+e: Want = bad_varargs  # E: is not assignable to `Want`
+    "#,
+);

pyrefly/lib/test/overload.rs

@@ -2577,3 +2577,28 @@ def f(any_frame: Frame[Any], owner: type[Frame[Any]]) -> None:
     assert_type(any_frame.impl, int)
     "#,
 );
+
+// `f(1, 2)` fills the implementation's `lower_or_string` and `lower`, while `f(lower=1, upper=2)`
+// fills its `lower` and `upper`: a non-constant mapping. https://github.com/facebook/pyrefly/issues/3587
+testcase!(
+    test_non_constant_param_mapping,
+    r#"
+from typing import overload
+
+@overload
+def f(string: str, /) -> None: ...
+@overload
+def f(lower: int, upper: int) -> None: ...
+def f(
+    lower_or_string: str | int | None = None,
+    /,
+    lower: int | None = None,
+    upper: int | None = None,
+) -> None: ...
+
+f("a")
+f(1, 2)
+f(1, upper=2)
+f(lower=1, upper=2)
+    "#,
+);