diff --git a/crates/pyrefly_types/src/callable.rs b/crates/pyrefly_types/src/callable.rs index 8b14106d10..bf0d89a3d3 100644 --- a/crates/pyrefly_types/src/callable.rs +++ b/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) diff --git a/pyrefly/lib/alt/functools.rs b/pyrefly/lib/alt/functools.rs index c26ec13bcd..f399e5b692 100644 --- a/pyrefly/lib/alt/functools.rs +++ b/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(); } } } diff --git a/pyrefly/lib/solver/solver.rs b/pyrefly/lib/solver/solver.rs index 8388117315..dc4323a7e2 100644 --- a/pyrefly/lib/solver/solver.rs +++ b/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 { diff --git a/pyrefly/lib/solver/subset.rs b/pyrefly/lib/solver/subset.rs index 2c6c5734bb..23889bc80d 100644 --- a/pyrefly/lib/solver/subset.rs +++ b/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,7 +240,7 @@ 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], @@ -247,24 +248,56 @@ impl<'solver, 'subset, Ans: LookupAnswer> Subset<'solver, 'subset, Ans> { 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 = 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 = 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(), diff --git a/pyrefly/lib/test/callable.rs b/pyrefly/lib/test/callable.rs index a021b235e1..5ca7619ab3 100644 --- a/pyrefly/lib/test/callable.rs +++ b/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` + "#, +); diff --git a/pyrefly/lib/test/overload.rs b/pyrefly/lib/test/overload.rs index 04084ecca6..8ed55768c8 100644 --- a/pyrefly/lib/test/overload.rs +++ b/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) + "#, +);