diff --git a/Cargo.lock b/Cargo.lock index c4e271172d..ad177ae667 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5008,6 +5008,7 @@ dependencies = [ "colored", "compact_str", "crossbeam", + "dunce", "get-size2", "globset", "insta", diff --git a/crates/ty/docs/rules.md b/crates/ty/docs/rules.md index 08fe076dc3..384ee2a04f 100644 --- a/crates/ty/docs/rules.md +++ b/crates/ty/docs/rules.md @@ -3897,6 +3897,26 @@ def foo() -> None: ... # error def foo(x: int) -> int: ... ``` +**Implementation default an overload's default doesn't describe** + + +A call matching an overload that leaves an argument out is solved from the overload's default, but +the implementation runs with its own. + +```py +from typing import TypeVar, overload + +T = TypeVar("T") + + +@overload +def foo(x: T = 1) -> T: ... # error +@overload +def foo(x: int, y: int) -> int: ... +def foo(x: object = "a", y: int = 0) -> object: + return x +``` + **References** @@ -3955,6 +3975,17 @@ Added in 0. Checks for default values that can't be assigned to the parameter's annotated type. +A default only needs to fit *some* specialization of the type variables the annotation names: a call +that leaves the argument out is solved as if the default had been passed. The default is reported +when no such specialization exists. Only the type variables of the function itself (and, for a +method, of its class) can be chosen that way; one of an enclosing function is fixed for every call. +A default whose own type names one of those type variables, such as `[1]` inferred against `list[T]` +as `list[T | int]`, is reported too: it is a single value shared by every call, so no call can make +it fit. + +In basedpython, a default an override inherits from the method it overrides is checked against the +override's annotation in the same way. + **Why is this bad?** @@ -3965,7 +3996,23 @@ about your code. ```python +from typing import TypeVar + +T = TypeVar("T") +S = TypeVar("S", bound=str) + + def f(a: int = ""): ... # error + + +# fine: `g()` solves `T` from the default +def g(a: T = 1) -> T: + return a + + +# no `S` bounded by `str` holds `1` +def h(a: S = 1) -> S: # error + return a ``` ## `invalid-parametrize` diff --git a/crates/ty_project/Cargo.toml b/crates/ty_project/Cargo.toml index de3fceeee7..bf54a50720 100644 --- a/crates/ty_project/Cargo.toml +++ b/crates/ty_project/Cargo.toml @@ -38,6 +38,7 @@ camino = { workspace = true } colored = { workspace = true } compact_str = { workspace = true, features = ["serde"] } crossbeam = { workspace = true } +dunce = { workspace = true } get-size2 = { workspace = true, features = ["ordermap", "parking_lot"] } globset = { workspace = true } notify = { workspace = true } diff --git a/crates/ty_project/src/watch/project_watcher.rs b/crates/ty_project/src/watch/project_watcher.rs index 4b5f8afef2..ee9eeb4ab8 100644 --- a/crates/ty_project/src/watch/project_watcher.rs +++ b/crates/ty_project/src/watch/project_watcher.rs @@ -10,7 +10,7 @@ use ty_module_resolver::system_module_search_paths; use crate::db::{Db, ProjectDatabase}; use crate::watch::Watcher; -/// Wrapper around a [`Watcher`] that watches the relevant paths of a project. +/// Wrapper around a [`Watcher`] that watches the relevant paths of one or more projects. pub struct ProjectWatcher { watcher: Watcher, @@ -27,6 +27,17 @@ pub struct ProjectWatcher { impl ProjectWatcher { /// Create a new project watcher. pub fn new(watcher: Watcher, db: &ProjectDatabase) -> Self { + Self::for_projects(watcher, [db]) + } + + /// Create a watcher over every project in `dbs`, with one set of watched paths for all of them. + /// + /// For a process that holds several projects at once, such as a language server with several + /// workspace folders. + pub fn for_projects<'a>( + watcher: Watcher, + dbs: impl IntoIterator, + ) -> Self { let mut watcher = Self { watcher, watched_paths: Vec::new(), @@ -34,17 +45,45 @@ impl ProjectWatcher { has_errored_paths: false, }; - watcher.update(db); + watcher.update_projects(dbs); watcher } pub fn update(&mut self, db: &ProjectDatabase) { - let environment = db.project().program(db).resolver_environment(db); - let search_paths: Vec<_> = system_module_search_paths(db, environment).collect(); - let project_path = db.project().root(db); + self.update_projects([db]); + } + + /// Watches what every project in `dbs` needs watched, and stops watching anything else. + pub fn update_projects<'a>(&mut self, dbs: impl IntoIterator) { + let dbs: Vec<&ProjectDatabase> = dbs.into_iter().collect(); + + let mut project_paths = Vec::new(); + let mut search_paths = Vec::new(); + let mut config_paths = Vec::new(); + for db in &dbs { + let project = db.project(); + let project_path = project.root(*db); + project_paths.push(project_path); + project_paths.extend( + project + .included_paths_list(*db) + .iter() + .map(SystemPathBuf::as_path), + ); + + // Module search paths are already canonicalized. The ones under the project root + // are covered by watching the root. + let environment = project.program(*db).resolver_environment(*db); + search_paths.extend( + system_module_search_paths(*db, environment) + .filter(|path| !path.starts_with(project_path)), + ); + + config_paths.extend(project.metadata(*db).extra_configuration_paths()); + } - let new_cache_key = Self::compute_cache_key(project_path, &search_paths); + let new_cache_key = Self::compute_cache_key(&project_paths, &search_paths, &config_paths); if self.cache_key == Some(new_cache_key) { return; @@ -70,29 +109,20 @@ impl ProjectWatcher { self.has_errored_paths = false; - let config_paths = db.project().metadata(db).extra_configuration_paths(); - - // Watch both the project root and any paths provided by the user on the CLI (removing any redundant nested paths). + // Watch both the project roots and any paths provided by the user on the CLI (removing any redundant nested paths). // This is necessary to observe changes to files that are outside the project root. // We always need to watch the project root to observe changes to its configuration. - let included_paths = ruff_db::system::deduplicate_nested_paths( - std::iter::once(project_path).chain( - db.project() - .included_paths_list(db) - .iter() - .map(SystemPathBuf::as_path), - ), - ); + let included_paths = ruff_db::system::deduplicate_nested_paths(project_paths); - // Find the non-overlapping module search paths and filter out paths that are already covered by the project. - // Module search paths are already canonicalized. - let unique_module_paths = ruff_db::system::deduplicate_nested_paths( - search_paths - .into_iter() - .filter(|path| !path.starts_with(project_path)), - ); + // Find the non-overlapping module search paths. Those under a project's own root were + // filtered out above; a search path of one project can still be another project's root. + let unique_module_paths = + ruff_db::system::deduplicate_nested_paths(search_paths).filter(|path| { + !dbs.iter() + .any(|db| path.starts_with(db.project().root(*db))) + }); - // Now add the new paths, first starting with the project path and then + // Now add the new paths, first starting with the project paths and then // adding the library search paths, and finally the paths for configurations. for path in included_paths .chain(unique_module_paths) @@ -127,10 +157,15 @@ impl ProjectWatcher { self.cache_key = Some(new_cache_key); } - fn compute_cache_key(project_root: &SystemPath, search_paths: &[&SystemPath]) -> u64 { + fn compute_cache_key( + project_paths: &[&SystemPath], + search_paths: &[&SystemPath], + config_paths: &[&SystemPath], + ) -> u64 { let mut cache_key_hasher = CacheKeyHasher::new(); search_paths.cache_key(&mut cache_key_hasher); - project_root.cache_key(&mut cache_key_hasher); + project_paths.cache_key(&mut cache_key_hasher); + config_paths.cache_key(&mut cache_key_hasher); cache_key_hasher.finish() } diff --git a/crates/ty_project/src/watch/watcher.rs b/crates/ty_project/src/watch/watcher.rs index ba016f59ba..9faf917d30 100644 --- a/crates/ty_project/src/watch/watcher.rs +++ b/crates/ty_project/src/watch/watcher.rs @@ -3,6 +3,9 @@ reason = "This implementation is specific to real file systems." )] +use std::path::PathBuf; +use std::sync::{Arc, Mutex}; + use notify::event::{CreateKind, MetadataKind, ModifyKind, RemoveKind, RenameMode}; use notify::{EventKind, RecommendedWatcher, RecursiveMode, Watcher as _, recommended_watcher}; @@ -18,6 +21,8 @@ where H: EventHandler, { let (sender, receiver) = crossbeam::channel::bounded(20); + let aliases = Aliases::default(); + let debouncer_aliases = aliases.clone(); let debouncer = std::thread::Builder::new() .name("watcher::debouncer".to_string()) @@ -31,7 +36,7 @@ where } }; - let mut debouncer = Debouncer::default(); + let mut debouncer = Debouncer::new(debouncer_aliases.clone()); debouncer.add_result(event); @@ -89,6 +94,7 @@ where watcher, debouncer_sender, debouncer_thread: debouncer, + aliases, }), }) } @@ -109,13 +115,63 @@ struct WatcherInner { watcher: RecommendedWatcher, debouncer_sender: crossbeam::channel::Sender, debouncer_thread: std::thread::JoinHandle<()>, + aliases: Aliases, +} + +/// The watched paths that are not where they resolve to, by the path they resolve to. +/// +/// macOS reports a change at the path the file system resolved, whatever path was watched: +/// watching `/tmp/project`, a write to `/tmp/project/a.py` is reported at +/// `/private/tmp/project/a.py`. The files in a project are known by the paths under its root as +/// it was given, so an event is put back under the watched path before it is handed on, or it +/// names a file nothing knows about and is lost. +#[derive(Clone, Default)] +struct Aliases(Arc>>); + +impl Aliases { + fn add(&self, watched: &SystemPath) { + // `dunce` keeps windows paths in the `C:\…` spelling events are reported in, rather than + // the verbatim `\\?\C:\…` that would make every watched path look like an alias + let Ok(resolved) = dunce::canonicalize(watched.as_std_path()) else { + return; + }; + if resolved != watched.as_std_path() { + self.lock().push((resolved, watched.to_path_buf())); + } + } + + fn remove(&self, watched: &SystemPath) { + self.lock().retain(|(_, path)| path.as_path() != watched); + } + + /// `path` under the watched path it was reported for, if it was reported under the path + /// that one resolves to. + fn to_watched(&self, path: PathBuf) -> PathBuf { + let aliases = self.lock(); + for (resolved, watched) in aliases.iter() { + if let Ok(rest) = path.strip_prefix(resolved) { + return watched.as_std_path().join(rest); + } + } + path + } + + fn lock(&self) -> std::sync::MutexGuard<'_, Vec<(PathBuf, SystemPathBuf)>> { + // the list is only ever pushed to and filtered, so a panic while it was locked cannot + // have left it half-updated + self.0 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } } impl Watcher { /// Returns a transaction-like view for updating watched paths in one backend operation. pub(crate) fn paths_mut(&mut self) -> WatcherPathsMut<'_> { + let inner = self.inner_mut(); WatcherPathsMut { - inner: self.inner_mut().watcher.paths_mut(), + inner: inner.watcher.paths_mut(), + aliases: &inner.aliases, } } @@ -163,16 +219,21 @@ impl Watcher { pub(crate) struct WatcherPathsMut<'a> { inner: Box, + aliases: &'a Aliases, } impl WatcherPathsMut<'_> { pub(crate) fn add(&mut self, path: &SystemPath) -> notify::Result<()> { tracing::debug!("Watching path: `{path}`"); - self.inner.add(path.as_std_path(), RecursiveMode::Recursive) + self.inner + .add(path.as_std_path(), RecursiveMode::Recursive)?; + self.aliases.add(path); + Ok(()) } pub(crate) fn remove(&mut self, path: &SystemPath) -> notify::Result<()> { tracing::debug!("Unwatching path: `{path}`"); + self.aliases.remove(path); self.inner.remove(path.as_std_path()) } @@ -187,13 +248,21 @@ impl Drop for Watcher { } } -#[derive(Default)] struct Debouncer { events: Vec, rescan_event: Option, + aliases: Aliases, } impl Debouncer { + fn new(aliases: Aliases) -> Self { + Self { + events: Vec::new(), + rescan_event: None, + aliases, + } + } + fn add_result(&mut self, result: notify::Result) { tracing::trace!("Handling file watcher event: {result:?}"); match result { @@ -234,7 +303,7 @@ impl Debouncer { return; }; - let path = match SystemPathBuf::from_path_buf(path) { + let path = match SystemPathBuf::from_path_buf(self.aliases.to_watched(path)) { Ok(path) => path, Err(path) => { tracing::debug!( diff --git a/crates/ty_python_semantic/resources/lint_docs/invalid-overload.md b/crates/ty_python_semantic/resources/lint_docs/invalid-overload.md index 25aa375473..3fa2f6de5f 100644 --- a/crates/ty_python_semantic/resources/lint_docs/invalid-overload.md +++ b/crates/ty_python_semantic/resources/lint_docs/invalid-overload.md @@ -35,6 +35,25 @@ def foo() -> None: ... # error def foo(x: int) -> int: ... ``` +### Implementation default an overload's default doesn't describe + +A call matching an overload that leaves an argument out is solved from the overload's default, but +the implementation runs with its own. + +```py +from typing import TypeVar, overload + +T = TypeVar("T") + + +@overload +def foo(x: T = 1) -> T: ... # error +@overload +def foo(x: int, y: int) -> int: ... +def foo(x: object = "a", y: int = 0) -> object: + return x +``` + ## References - [Python documentation: `@overload`](https://docs.python.org/3/library/typing.html#typing.overload) diff --git a/crates/ty_python_semantic/resources/lint_docs/invalid-parameter-default.md b/crates/ty_python_semantic/resources/lint_docs/invalid-parameter-default.md index 9161cb931e..d24dccddcf 100644 --- a/crates/ty_python_semantic/resources/lint_docs/invalid-parameter-default.md +++ b/crates/ty_python_semantic/resources/lint_docs/invalid-parameter-default.md @@ -2,6 +2,17 @@ Checks for default values that can't be assigned to the parameter's annotated type. +A default only needs to fit *some* specialization of the type variables the annotation names: a call +that leaves the argument out is solved as if the default had been passed. The default is reported +when no such specialization exists. Only the type variables of the function itself (and, for a +method, of its class) can be chosen that way; one of an enclosing function is fixed for every call. +A default whose own type names one of those type variables, such as `[1]` inferred against `list[T]` +as `list[T | int]`, is reported too: it is a single value shared by every call, so no call can make +it fit. + +In basedpython, a default an override inherits from the method it overrides is checked against the +override's annotation in the same way. + ## Why is this bad? This breaks the rules of the type system and weakens a type checker's ability to accurately reason @@ -10,5 +21,21 @@ about your code. ## Examples ```python +from typing import TypeVar + +T = TypeVar("T") +S = TypeVar("S", bound=str) + + def f(a: int = ""): ... # error + + +# fine: `g()` solves `T` from the default +def g(a: T = 1) -> T: + return a + + +# no `S` bounded by `str` holds `1` +def h(a: S = 1) -> S: # error + return a ``` diff --git a/crates/ty_python_semantic/resources/mdtest/basedpython_inherited_defaults.md b/crates/ty_python_semantic/resources/mdtest/basedpython_inherited_defaults.md index 0e4679b452..6ef3417715 100644 --- a/crates/ty_python_semantic/resources/mdtest/basedpython_inherited_defaults.md +++ b/crates/ty_python_semantic/resources/mdtest/basedpython_inherited_defaults.md @@ -69,6 +69,46 @@ class C(B): C().f() ``` +## a default that initialises the override's type parameter + +the override is what the call binds against, so an inherited default is solved against the +override's annotation rather than the one the base wrote it for. `1` fits `T` only when `T` is +something `1` is, so a call that leaves `a` out solves `T` from the default, as it would from a +default the override wrote itself (see [parameter defaults](generics/parameter_defaults.md)). + +```by +class A: + def f(self, a: int = 1) -> int: + return a + +class B(A): + override def f[T](self, a: T) -> T: + return a + +n: int = B().f() +# error: [invalid-assignment] +s: str = B().f() +``` + +## an inherited default the override's annotation can't take + +a default the base's annotation takes may fit no specialization of the override's. a call that +leaves the argument out would still run the override with it, so it is reported where the override +is written, the same as a default the override wrote itself. + +```by +from typing import Any + +class A: + def f(self, a: Any = "x") -> object: + return a + +class B(A): + # error: [invalid-parameter-default] "Inherited default value of type `"x"` is not assignable to annotated parameter type `int`" + override def f(self, a: int) -> int: + return a +``` + ## a parameter the base does not default stays required ```by diff --git a/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclass_transform.md b/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclass_transform.md index c6431f0b1c..0258c8c790 100644 --- a/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclass_transform.md +++ b/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclass_transform.md @@ -2137,8 +2137,6 @@ converter will use the default: ```py from typing_extensions import Any -# TODO: no error here (https://github.com/astral-sh/ty/issues/592) -# error: [invalid-parameter-default] def field_with_default_converter[T, R](*, converter: Callable[[T], R] = str_to_int, default: T | None = None) -> R: raise NotImplementedError diff --git a/crates/ty_python_semantic/resources/mdtest/generics/parameter_defaults.md b/crates/ty_python_semantic/resources/mdtest/generics/parameter_defaults.md new file mode 100644 index 0000000000..0eb70f8b64 --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/generics/parameter_defaults.md @@ -0,0 +1,513 @@ +# parameter defaults that initialise type variables + +```toml +[environment] +python-version = "3.13" +``` + +a parameter's default doesn't have to fit every specialization of the type variables its annotation +names. when it fits only some of them, the default *initialises* those type variables: a call that +leaves the argument out is solved and checked as if the default had been passed in its place. + +## solving from the default + +`1` is not a valid `T` for every `T`, but it is for some. a call without the argument solves `T` +from the default, exactly as `f(1)` would. a call that passes the argument ignores the default. + +```py +def f[T](t: T = 1) -> T: + return t + +reveal_type(f()) # revealed: Literal[1] +reveal_type(f("a")) # revealed: Literal["a"] +``` + +the same holds for a keyword-only parameter, and for a legacy type variable, whose constraints the +default picks between. + +```py +from typing import TypeVar + +def keyword[T](*, t: T = 1) -> T: + return t + +reveal_type(keyword()) # revealed: Literal[1] + +IntOrStr = TypeVar("IntOrStr", int, str) + +def constrained(t: IntOrStr = 1) -> IntOrStr: + return t + +reveal_type(constrained()) # revealed: int +reveal_type(constrained("a")) # revealed: str +``` + +## variadic type parameters + +a default initialises a `ParamSpec` or a `TypeVarTuple` the same way. + +```py +from collections.abc import Callable + +def one(x: int) -> str: + return "" + +def forward[**P](c: Callable[P, str] = one) -> Callable[P, str]: + return c + +reveal_type(forward()) # revealed: (x: int) -> str + +def pack[*Ts](t: tuple[*Ts] = (1, "a")) -> tuple[*Ts]: + return t + +reveal_type(pack()) # revealed: tuple[Literal[1], Literal["a"]] +reveal_type(pack((b"x",))) # revealed: tuple[Literal[b"x"]] +``` + +## a default no call can use + +a default is only accepted when some specialization fits it. a bound or a constraint it violates +rules them all out. + +```py +from typing import TypeVar + +# error: [invalid-parameter-default] "Default value of type `Literal[1]` is not assignable to annotated parameter type `T@bounded`" +def bounded[T: str](t: T = 1) -> T: + return t + +IntOrStr = TypeVar("IntOrStr", int, str) + +# error: [invalid-parameter-default] "Default value of type `Literal[b"x"]` is not assignable to annotated parameter type `IntOrStr@constrained`" +def constrained(t: IntOrStr = b"x") -> IntOrStr: + return t +``` + +a type variable of an enclosing function is fixed throughout the body the inner function is defined +in, so no call to the inner function can specialize it to fit. + +```py +def outer[T](value: T) -> None: + # error: [invalid-parameter-default] + def inner(t: T = 1) -> None: ... +``` + +## a default whose type names the type variable + +a default is typed as one value, made once when the function is defined and shared by every call. +`[1]` inferred against `list[T]` is a `list[T | int]`, a type that names `T`, but no call can make +the list that is actually there hold its own `T`: appending to it in one call would put that call's +`T` into the list the next call sees. so such a default initialises nothing, and is reported. + +```py +# error: [invalid-parameter-default] "Default value of type `list[T@first_or | int]` is not assignable to annotated parameter type `list[T@first_or]`" +def first_or[T](value: T, fallback: list[T] = [1]) -> list[T]: + return fallback +``` + +a default that fits whatever `T` is, like `[]` against `list[T]`, is unaffected. + +```py +def items[T](value: T, into: list[T] = []) -> list[T]: + return into +``` + +## defaults that fit every specialization + +a default that fits the annotation whatever the type variables are, like `None` in `T | None`, says +nothing about them, so a call that falls back on it is solved from its arguments alone. + +```py +def optional[T](t: T | None = None) -> list[T]: + return [] + +reveal_type(optional()) # revealed: list[Unknown] +reveal_type(optional(1)) # revealed: list[int] +``` + +## defaults solved together with arguments + +a default that the call falls back on takes part in solving alongside the arguments that were +passed, just as a second argument would. + +```py +def pair[T](first: T, second: T = 1) -> T: + return first + +reveal_type(pair("a")) # revealed: Literal["a", 1] +reveal_type(pair("a", "b")) # revealed: Literal["a", "b"] + +def both[T](first: T = 1, second: T = "a") -> T: + return first + +reveal_type(both()) # revealed: Literal[1, "a"] +reveal_type(both(second=2)) # revealed: Literal[2, 1] +``` + +when an argument pins the type variable to something the default doesn't fit, the call is rejected. +here the invariant `list[T]` takes `T` to be `str`, which `0` is not, and the diagnostic names the +default that took part. + +```py +def first[T](items: list[T], fallback: T = 0) -> T: + return items[0] if items else fallback + +def _(names: list[str]): + first(names, "") + first(names) # snapshot: invalid-argument-type +``` + +```snapshot +error[invalid-argument-type]: Argument to function `first` is incorrect + --> src/mdtest_snippet.py:17:11 + | +17 | first(names) # snapshot: invalid-argument-type + | ^^^^^ Expected `list[str | Literal[0]]`, found `list[str]` +info: element `Literal[0]` of union `str | Literal[0]` is not assignable to `str` +info: no argument is passed for `fallback`, so its default of type `Literal[0]` takes part in solving this call + --> src/mdtest_snippet.py:12:30 + | +12 | def first[T](items: list[T], fallback: T = 0) -> T: + | ^^^^^^^^^^^^^^^ +info: Function defined here + --> src/mdtest_snippet.py:12:5 + | +12 | def first[T](items: list[T], fallback: T = 0) -> T: + | ^^^^^ -------------- Parameter declared here +info: `list` is invariant in its type parameter +info: Consider using the covariant supertype `collections.abc.Sequence` +info: For more information, see https://docs.astral.sh/ty/reference/typing-faq/#invariant-generics +``` + +## unpacked arguments + +an unpacked argument that may be too short to reach the parameter may supply it or leave it out: +`f(*maybe)` is `f()` when `maybe` is empty. so the default takes part alongside the unpacked +elements, and the same goes for the keys of an unpacked mapping and the ones a `TypedDict` doesn't +require. + +```py +from typing import TypedDict + +def f[T](t: T = 1) -> T: + return t + +class MaybeNamed(TypedDict, total=False): + t: str + +def _(maybe: tuple[()] | tuple[str], by_name: dict[str, str], maybe_named: MaybeNamed): + reveal_type(f(*maybe)) # revealed: str | Literal[1] + reveal_type(f(**by_name)) # revealed: str | Literal[1] + reveal_type(f(**maybe_named)) # revealed: str | Literal[1] +``` + +a list may be empty too, and it may also be longer than the parameters it unpacks into. + +```py +def _(names: list[str]): + # error: [refutable-unpacking] + reveal_type(f(*names)) # revealed: str | Literal[1] +``` + +an unpacked tuple of known length, or a required key of a `TypedDict`, always supplies the +parameter, so the default plays no part. + +```py +class Named(TypedDict): + t: str + +def _(pair: tuple[str], named: Named): + reveal_type(f(*pair)) # revealed: str + reveal_type(f(**named)) # revealed: str +``` + +## class type parameters + +a constructor's defaults initialise the class's type parameters, whether they are declared on +`__init__` or on `__new__`. + +```py +from typing import Self + +class Box[T]: + def __init__(self, item: T = 1) -> None: + self.item = item + + def replace(self, item: T = 1) -> T: + return item + +reveal_type(Box()) # revealed: Box[int] +reveal_type(Box("a")) # revealed: Box[str] + +class Made[T]: + def __new__(cls, item: T = 1) -> Self: + return super().__new__(cls) + +reveal_type(Made()) # revealed: Made[Literal[1]] +``` + +a method is given its class's type parameters by the receiver, so a method's default is checked +against the receiver's specialization. an explicitly specialized `Box[str]` can't be built without +an item either. + +```py +reveal_type(Box().replace()) # revealed: int + +def _(box: Box[str]): + box.replace("a") + box.replace() # snapshot: invalid-argument-type + +# error: [invalid-argument-type] "Default value of parameter `item` of class `Box` does not fit this call: Expected `str`, found `Literal[1]`" +Box[str]() +``` + +```snapshot +error[invalid-argument-type]: Default value of parameter `item` of bound method `Box.replace` does not fit this call + --> src/mdtest_snippet.py:22:5 + | +22 | box.replace() # snapshot: invalid-argument-type + | ^^^^^^^^^^^^^ Expected `str`, found `Literal[1]` +info: Parameter declared here + --> src/mdtest_snippet.py:7:23 + | +7 | def replace(self, item: T = 1) -> T: + | ^^^^^^^^^^^ +``` + +class methods and static methods solve their own type parameters from their defaults like any other +function. + +```py +class Factory: + @classmethod + def make[T](cls, item: T = 1) -> T: + return item + + @staticmethod + def build[T](item: T = 1) -> T: + return item + +reveal_type(Factory.make()) # revealed: Literal[1] +reveal_type(Factory().make()) # revealed: Literal[1] +reveal_type(Factory.build()) # revealed: Literal[1] +``` + +## placeholder defaults + +the `...` that a declaration without a body of its own writes in place of a value is not a value. it +neither has to fit the annotation nor solves anything: a call to such a declaration leaves the type +variable unsolved. that covers a stub, an overload, an abstract method, a protocol member, and a +declaration under `if TYPE_CHECKING:`. + +```pyi +def stub[T](t: T = ...) -> T: ... +``` + +```py +from abc import ABC, abstractmethod +from typing import TYPE_CHECKING, Protocol, overload + +@overload +def overloaded[T](t: T = ...) -> T: ... +@overload +def overloaded(t: int, u: int) -> int: ... +def overloaded(t=1, u=2): + return t + +reveal_type(overloaded()) # revealed: Unknown + +class Abstract(ABC): + @abstractmethod + def get[T](self, t: T = ...) -> T: ... + +class Gettable(Protocol): + def get[T](self, t: T = ...) -> T: ... + +def _(abstract: Abstract, gettable: Gettable): + reveal_type(abstract.get()) # revealed: Unknown + reveal_type(gettable.get()) # revealed: Unknown + +if TYPE_CHECKING: + def declared[T](t: T = ...) -> T: ... + +else: + def declared(t=1): + return t + +reveal_type(declared()) # revealed: Unknown +``` + +outside of those, `...` is an ordinary value of type `EllipsisType`. + +```py +from typing import Self + +class Node: + # error: [invalid-parameter-default] "Default value of type `EllipsisType` is not assignable to annotated parameter type `Self@copy`" + def copy(self, other: Self = ...) -> Self: + return self +``` + +## an overload's default is a promise about the implementation + +a call that matches an overload and leaves the argument out is solved from the overload's default, +but it is the implementation that runs, with its own default. so the implementation's default has to +be a value the overload's describes. + +```py +from typing import overload + +@overload +def get[T](t: T = 1) -> T: ... +@overload +def get(t: int, u: int) -> int: ... +def get(t: object = 1, u: int = 0) -> object: + return t + +reveal_type(get()) # revealed: Literal[1] + +@overload +def mismatched[T](t: T = 1) -> T: ... # snapshot: invalid-overload +@overload +def mismatched(t: int, u: int) -> int: ... +def mismatched(t: object = "a", u: int = 0) -> object: + return t +``` + +```snapshot +error[invalid-overload]: Implementation's default for parameter `t` does not fit this overload's default: expected `Literal[1]`, found `Literal["a"]` + --> src/mdtest_snippet.py:13:5 + | +13 | def mismatched[T](t: T = 1) -> T: ... # snapshot: invalid-overload + | ^^^^^^^^^^ -------- Overload's default declared here +14 | @overload +15 | def mismatched(t: int, u: int) -> int: ... +16 | def mismatched(t: object = "a", u: int = 0) -> object: + | ---------- Implementation defined here +``` + +## a default is part of the type a method declares + +a caller that holds an `A` and calls `a.m()` solves `T` from `A.m`'s default, whatever subclass it +really holds. so an override, an implementation of a protocol, or a function passed where a callable +protocol is expected has to fall back on a default that `A.m`'s describes. + +```py +from typing import Protocol, override + +class A: + def m[T](self, t: T = 1) -> T: + return t + +class Same(A): + @override + def m[T](self, t: T = 1) -> T: + return t + +class Different(A): + @override + # error: [invalid-method-override] + def m[T](self, t: T = "a") -> T: + return t + +def _(a: A): + reveal_type(a.m()) # revealed: Literal[1] + +class Gettable(Protocol): + def get[T](self, t: T = 1) -> T: ... + +class DefaultsToStr: + def get[T](self, t: T = "a") -> T: + return t + +# error: [invalid-assignment] +gettable: Gettable = DefaultsToStr() + +class Call(Protocol): + def __call__[T](self, t: T = 1) -> T: ... + +def defaults_to_str[T](t: T = "a") -> T: + return t + +# error: [invalid-assignment] +call: Call = defaults_to_str +``` + +## passing a function with such a default + +a function whose default initialises a type variable can stand in for a callable that doesn't take +that argument. the type variable is solved from the default. + +```py +from collections.abc import Callable + +def g[T](t: T = 1) -> T: + return t + +def call[R](f: Callable[[], R]) -> R: + return f() + +reveal_type(call(g)) # revealed: Literal[1] +``` + +a callable that forwards its parameters with a `ParamSpec` keeps the default, and a call through it +falls back on the default like a direct call does. + +```py +def forward[**P, R](f: Callable[P, R]) -> Callable[P, R]: + return f + +reveal_type(forward(g)) # revealed: [T](t: T = 1) -> T +reveal_type(forward(g)()) # revealed: Literal[1] +reveal_type(forward(g)("a")) # revealed: Literal["a"] +``` + +a partial application may leave the parameter to the later call, which may fall back on the default, +so the default takes part in solving the partial application too. + +```py +from functools import partial + +def pair[T](first: T, second: T = 1) -> T: + return first + +reveal_type(partial(pair, "a")()) # revealed: str | int +``` + +assigning such a function to a declared callable type doesn't yet solve its type variables, so the +default isn't taken into account there. + +```py +# TODO: error: [invalid-assignment] +returns_str: Callable[[], str] = g +``` + +## `some` parameters + +`some` opens an anonymous type parameter, which a default initialises too. another parameter +annotated with that type is solved from the default as well as its own argument. + +```by +def twice(n: some int = 1) -> n: + return n + +reveal_type(twice()) # revealed: 1 +reveal_type(twice(3)) # revealed: 3 + +def f(n: some int = 1, m: n = 2) -> n: + return n + +reveal_type(f(m=5)) # revealed: 5 | 1 +``` + +## a default that names its own function + +a default can name the function it belongs to. solving `T` from it then asks for `T` again, which +stops at a cycle rather than nesting forever. + +```py +def g[T](x: T = lambda: g()) -> T: + return x + +result = g() +``` diff --git a/crates/ty_python_semantic/resources/mdtest/type_of/generics.md b/crates/ty_python_semantic/resources/mdtest/type_of/generics.md index 2de8042120..867f34205e 100644 --- a/crates/ty_python_semantic/resources/mdtest/type_of/generics.md +++ b/crates/ty_python_semantic/resources/mdtest/type_of/generics.md @@ -534,8 +534,6 @@ from typing import Any class Foo[T]: ... -# TODO: This should not error. -# error: [invalid-parameter-default] "Default value of type `` is not assignable to annotated parameter type `type[T@f]`" def f[T: Foo[Any]](x: type[T] = Foo): ... ``` diff --git a/crates/ty_python_semantic/src/types/call/bind.rs b/crates/ty_python_semantic/src/types/call/bind.rs index c6274514dd..536c456b17 100644 --- a/crates/ty_python_semantic/src/types/call/bind.rs +++ b/crates/ty_python_semantic/src/types/call/bind.rs @@ -5470,6 +5470,10 @@ struct ArgumentMatcher<'a, 'db> { /// This is used to prevent variadic arguments from greedily matching parameters that will be /// explicitly provided via keyword arguments. explicit_keyword_parameters: FxHashSet, + + /// basedpython: whether the matches being made now are ones the argument may not make at + /// runtime. see [`MatchedParameter::conditional`] + conditional: bool, } impl<'a, 'db> ArgumentMatcher<'a, 'db> { @@ -5502,6 +5506,7 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { variable_length_positional_arguments: SmallVec::new(), variadic_argument_matched_to_variadic_parameter: false, explicit_keyword_parameters, + conditional: false, } } @@ -5573,6 +5578,7 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { argument_type, expected_type: None, provenance, + conditional: self.conditional, }); matched_argument.matched = true; self.parameter_info[parameter_index].matched = true; @@ -5666,6 +5672,20 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { argument_index: usize, argument: Argument<'a>, argument_type: Option>, + ) -> Result<(), ()> { + let result = + self.match_variadic_positions(db, env, argument_index, argument, argument_type); + self.conditional = false; + result + } + + fn match_variadic_positions( + &mut self, + db: &'db dyn Db, + env: &ProgramEnvironment<'db>, + argument_index: usize, + argument: Argument<'a>, + argument_type: Option>, ) -> Result<(), ()> { enum VariadicArgumentType<'db> { ParamSpec(Type<'db>), @@ -5823,6 +5843,8 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { is_variable, )?; } + // every position past the guaranteed minimum is only there for some of the values + self.conditional = true; // For a union of fixed-length tuples, positions beyond the guaranteed minimum are only // present in the longer union members. They therefore cannot satisfy a required @@ -5915,6 +5937,7 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { // Special case TypedDict-shaped values because we know which keys are present. for (name, unpacked_key) in unpacked.keys { + self.conditional = !unpacked_key.is_required; let _ = self.match_keyword( argument_index, Argument::Keywords, @@ -5922,8 +5945,11 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { name.as_str(), ); } + self.conditional = false; self.match_typed_dict_openness(argument_index, openness); } else { + // a mapping of unknown keys may hold any of them, or none + self.conditional = true; for (parameter_index, parameter) in self.parameters.iter().enumerate() { if self.parameter_info[parameter_index].matched && !parameter.is_keyword_variadic() { @@ -5961,6 +5987,7 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { true, ); } + self.conditional = false; } } @@ -5993,6 +6020,8 @@ impl<'a, 'db> ArgumentMatcher<'a, 'db> { argument_type: Some(extra_items_ty), expected_type: None, provenance: InvalidArgumentTypeProvenance::Argument, + // an extra item may or may not be there + conditional: true, }); } } @@ -6220,6 +6249,10 @@ struct ArgumentTypeChecker<'a, 'db> { /// See [`typevars_reached_by_arguments`]. typevars_reached_by_arguments: Box<[BoundTypeVarIdentity<'db>]>, + /// basedpython: the parameters the call may leave out whose defaults take part in it as if + /// they had been passed. see [`initialising_defaults`] + initialising_defaults: Box<[DefaultRelation<'db>]>, + /// Argument indices for which specialization inference has already produced a sufficiently /// precise argument mismatch. We can then silence `check_argument_type` for those arguments to /// avoid duplicate diagnostics. @@ -6267,6 +6300,67 @@ impl<'db> ArgumentRelation<'db> { } } +/// basedpython: a parameter the call may leave out, whose default is passed in its place because +/// it initialises a type variable of the parameter's annotation. see +/// [`initialising_default_type`](crate::types::signatures::initialising_default_type) +#[derive(Clone, Copy, Debug)] +struct DefaultRelation<'db> { + parameter_index: usize, + declared_type: Type<'db>, + default_type: Type<'db>, +} + +/// basedpython: the parameters the call may leave out whose defaults initialise a type variable, +/// as `1` initialises `T` in `def f[T](t: T = 1)` +/// +/// such a default only fits some specializations of the annotation, so the call that falls back +/// on it has to pick one it fits: `f()` solves `T` from the default, and `f(names)` with +/// `names: list[str]` on `def f[T](x: list[T], t: T = 1)` is rejected, because the invariant +/// `list[T]` leaves no `T` that both fit +/// +/// a parameter only an unpacked argument reaches may still be left out — `f(*names)` is `f()` +/// when `names` is empty — so its default takes part alongside what the unpacking supplies. a +/// partial application is solved the same way, since the call it makes later may leave the +/// parameter out too +fn initialising_defaults<'db>( + db: &'db dyn Db, + env: &ProgramEnvironment<'db>, + signature: &Signature<'db>, + argument_matches: &[MatchedArgument<'db>], +) -> Box<[DefaultRelation<'db>]> { + let parameters = signature.parameters(); + let mut supplied = vec![false; parameters.len()]; + for matched in argument_matches.iter().flat_map(MatchedArgument::iter) { + if !matched.conditional + && let Some(supplied) = supplied.get_mut(matched.index) + { + *supplied = true; + } + } + parameters + .iter() + .zip(supplied) + .enumerate() + .filter(|(_, (_, supplied))| !supplied) + .filter_map(|(parameter_index, (parameter, _))| { + Some(DefaultRelation { + parameter_index, + declared_type: parameter.annotated_type().erase_overlapping(db, env), + default_type: parameter.initialising_default_type(db)?, + }) + }) + .collect() +} + +/// basedpython: a default the call falls back on that was solved together with an argument whose +/// check failed. see [`BindingError::InvalidArgumentType`] +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct DefaultInSolve<'db> { + name: Name, + parameter_index: usize, + default_ty: Type<'db>, +} + /// Result of checking only the key type of a keyword-unpack argument. enum KeywordUnpackKeyTypeCheck<'db> { /// The argument type is handled by a more specific path, or does not expose mapping keys. @@ -6345,6 +6439,7 @@ impl<'a, 'db> ArgumentTypeChecker<'a, 'db> { inferable_typevars: TypeVarSet::None, inference: None, typevars_reached_by_arguments, + initialising_defaults: initialising_defaults(db, env, signature, argument_matches), constraint_set_errors: vec![false; arguments.len()], } } @@ -6897,7 +6992,12 @@ impl<'a, 'db> ArgumentTypeChecker<'a, 'db> { Ok(inference) => inference, Err(()) => builder.build_diagnostic_inference_with( self.argument_relations() - .map(|relation| (relation.declared_type, relation.argument_type)), + .map(|relation| (relation.declared_type, relation.argument_type)) + .chain( + self.initialising_defaults + .iter() + .map(|relation| (relation.declared_type, relation.default_type)), + ), choose, ), }; @@ -7111,6 +7211,7 @@ impl<'a, 'db> ArgumentTypeChecker<'a, 'db> { provided_ty: actual, provenance: InvalidArgumentTypeProvenance::Argument, parameter_source: None, + defaults_in_solve: Box::default(), }); } @@ -7350,6 +7451,12 @@ impl<'a, 'db> ArgumentTypeChecker<'a, 'db> { } } + // a default that can't join the solve is reported against the specialization the call + // settles on, by `check_initialising_defaults` + for relation in &self.initialising_defaults { + let _ = builder.infer(relation.declared_type, relation.default_type); + } + if let Some((parameter_index, kind)) = keyword_aggregate { let aggregate_value = match kind { KeywordAggregateKind::ParameterPack => Type::paramspec_value_callable( @@ -7559,6 +7666,7 @@ impl<'a, 'db> ArgumentTypeChecker<'a, 'db> { provided_ty: argument_type, provenance: matched_parameter.provenance, parameter_source: None, + defaults_in_solve: self.defaults_sharing_typevars_with(declared_type), }); } // We still update the actual type of the parameter in this binding to match the argument, @@ -7843,6 +7951,79 @@ impl<'a, 'db> ArgumentTypeChecker<'a, 'db> { } } } + + self.check_initialising_defaults(constraints); + } + + /// basedpython: the defaults the call falls back on that name one of the type variables in + /// `declared`, so were solved together with an argument for it + fn defaults_sharing_typevars_with(&self, declared: Type<'db>) -> Box<[DefaultInSolve<'db>]> { + if self.initialising_defaults.is_empty() { + return Box::default(); + } + let db = self.db; + let mentions = |ty: Type<'db>| { + let typevars = std::cell::RefCell::new(FxHashSet::default()); + any_over_type(db, self.env, ty, false, |ty: Type<'db>| { + if let Type::TypeVar(typevar) = ty { + typevars.borrow_mut().insert(typevar.identity(db)); + } + false + }); + typevars.into_inner() + }; + let declared = mentions(declared); + self.initialising_defaults + .iter() + .filter(|relation| !mentions(relation.declared_type).is_disjoint(&declared)) + .filter_map(|relation| { + let name = self.signature.parameters()[relation.parameter_index].name()?; + Some(DefaultInSolve { + name: name.clone(), + parameter_index: relation.parameter_index, + default_ty: relation.default_type, + }) + }) + .collect() + } + + /// basedpython: reports each default the call falls back on that does not fit the + /// specialization the call settled on + /// + /// that is a specialization the call is given — by the receiver, as `box.replace()` is on a + /// `Box[str]`, or explicitly, as `Box[str]()` is — or one solved from arguments the default + /// can't be reconciled with. when an argument's own check fails first over the same type + /// variable, the conflict is reported against the argument instead, with a note naming the + /// default (see [`Self::defaults_sharing_typevars_with`]) + fn check_initialising_defaults(&mut self, constraints: &ConstraintSetBuilder<'db>) { + let db = self.db; + for index in 0..self.initialising_defaults.len() { + let relation = self.initialising_defaults[index]; + let default_ty = relation.default_type; + let expected_ty = match self.argument_specialization() { + Some(specialization) => relation + .declared_type + .apply_specialization(db, specialization), + None => relation.declared_type, + }; + if default_ty + .when_assignable_to( + db, + self.env, + expected_ty, + constraints, + self.inferable_typevars, + ) + .is_never_satisfied(db, self.env) + { + let parameter = &self.signature.parameters()[relation.parameter_index]; + self.errors.push(BindingError::InvalidDefaultArgument { + parameter: ParameterContext::new(parameter, relation.parameter_index, false), + expected_ty, + default_ty, + }); + } + } } /// Invoke a sub-call for the given `ParamSpec` type variable, using the forwarded arguments. @@ -8155,6 +8336,11 @@ pub struct MatchedParameter<'db> { /// Why this parameter match exists. provenance: InvalidArgumentTypeProvenance, + + /// basedpython: whether the argument may not supply this parameter at runtime — an unpacked + /// argument of unknown length or with keys that may be missing. a call that leaves the + /// parameter to such an argument may still fall back on its default + conditional: bool, } impl<'db> MatchedArgument<'db> { @@ -8382,7 +8568,9 @@ fn precise_unsolved_typevars<'db>(db: &'db dyn Db, signature: &Signature<'db>) - /// genuinely holds nothing. /// /// A gradual parameter puts every type variable in the set: the argument went somewhere the -/// annotation does not describe, so nothing about it has been established. +/// annotation does not describe, so nothing about it has been established. A parameter whose +/// initialising default the call falls back on reaches its type variables the same way an +/// argument does (see [`initialising_defaults`]). fn typevars_reached_by_arguments<'db>( db: &'db dyn Db, env: &ProgramEnvironment<'db>, @@ -8433,6 +8621,14 @@ fn typevars_reached_by_arguments<'db>( false }); } + for relation in initialising_defaults(db, env, signature, argument_matches) { + any_over_type(db, env, relation.declared_type, false, |ty: Type<'db>| { + if let Type::TypeVar(typevar) = ty { + reached.borrow_mut().insert(typevar.identity(db)); + } + false + }); + } // a boxed slice rather than the set: this rides along in a `Binding`, it is only ever // scanned for membership, and it holds a handful of entries at most reached.into_inner().into_iter().collect() @@ -10162,6 +10358,9 @@ pub(crate) enum BindingError<'db> { provenance: InvalidArgumentTypeProvenance, /// The callable that actually declared the parameter, when reached through a `ParamSpec`. parameter_source: Option>, + /// basedpython: the parameters the call left out whose defaults were solved together with + /// this one, which the argument may be failing to fit rather than the other way round + defaults_in_solve: Box<[DefaultInSolve<'db>]>, }, /// The type of the keyword-variadic argument's key is not `str`. InvalidKeyType { @@ -10225,6 +10424,14 @@ pub(crate) enum BindingError<'db> { expected_ty: Type<'db>, provided_ty: Type<'db>, }, + /// basedpython: the call left out a parameter whose default does not fit the specialization + /// the call settled on. see + /// [`initialising_default_type`](crate::types::signatures::initialising_default_type) + InvalidDefaultArgument { + parameter: ParameterContext, + expected_ty: Type<'db>, + default_ty: Type<'db>, + }, /// Multiple arguments were provided for a single parameter. ParameterAlreadyAssigned { argument_index: Option, @@ -10295,6 +10502,7 @@ impl BindingError<'_> { matches!( self, Self::InvalidArgumentType { .. } + | Self::InvalidDefaultArgument { .. } | Self::InvalidKeyType { .. } | Self::UnknownArgument { .. } | Self::UnknownKeywordVariadicArgument { .. } @@ -10374,6 +10582,7 @@ impl BindingError<'_> { | BindingError::InvalidDataclassApplication(..) | BindingError::InvalidDataclassArgument(..) | BindingError::MissingArguments { .. } + | BindingError::InvalidDefaultArgument { .. } | BindingError::NoContextArgument { .. } | BindingError::AmbiguousContextArgument { .. } | BindingError::RepeatedUnderscoreContextArgument { .. } @@ -10451,6 +10660,7 @@ impl<'db> BindingError<'db> { // Matching errors: the overload doesn't apply to these arguments Self::InvalidArgumentType { .. } + | Self::InvalidDefaultArgument { .. } | Self::InvalidKeyType { .. } | Self::MissingArguments { .. } | Self::NoContextArgument { .. } @@ -10496,6 +10706,7 @@ impl<'db> BindingError<'db> { provided_ty, provenance, parameter_source, + defaults_in_solve, } => { // TODO: Ideally we would not emit diagnostics for `TypedDict` literal arguments // here (see `diagnostic::is_invalid_typed_dict_literal`). However, we may have @@ -10579,6 +10790,25 @@ impl<'db> BindingError<'db> { let error_context = provided_ty.assignability_error_context(db, env, *expected_ty); error_context.attach_to(db, env, &mut diag); + for default in defaults_in_solve { + let mut sub = SubDiagnostic::new( + SubDiagnosticSeverity::Info, + format_args!( + "no argument is passed for `{}`, so its default of type `{}` takes \ + part in solving this call", + default.name, + default.default_ty.display(db, env) + ), + ); + if let Some((_, parameter_span)) = callable_ty.parameter_span( + db, + Some(default.parameter_index + source_parameter_index_offset), + ) { + sub.annotate(Annotation::primary(parameter_span)); + } + diag.sub(sub); + } + if let Some(parameter_source) = parameter_source { let (name_span, parameter_span) = parameter_source.parameter_span(db, parameter); @@ -10817,6 +11047,45 @@ impl<'db> BindingError<'db> { } } + Self::InvalidDefaultArgument { + parameter, + expected_ty, + default_ty, + } => { + let range = all_arguments_range(node); + if let Some(builder) = context.report_lint(&INVALID_ARGUMENT_TYPE, range) { + let display_settings = DisplaySettings::from_possibly_ambiguous_types( + db, + env, + [*default_ty, *expected_ty], + ); + let mut diag = builder.into_diagnostic(format_args!( + "Default value of parameter {parameter}{} does not fit this call", + callable_description + .map(|description| format!(" of {description}")) + .unwrap_or_default() + )); + diag.set_primary_annotation_message(format_args!( + "Expected `{}`, found `{}`", + expected_ty.display_with(db, env, display_settings.clone()), + default_ty.display_with(db, env, display_settings), + )); + if let Some(compound_diag) = compound_diag { + compound_diag.add_context(db, env, &mut diag); + } else if let Some((_, parameter_span)) = callable_ty.parameter_span( + db, + Some(parameter.signature_parameter_index + source_parameter_index_offset), + ) { + let mut sub = SubDiagnostic::new( + SubDiagnosticSeverity::Info, + "Parameter declared here", + ); + sub.annotate(Annotation::primary(parameter_span)); + diag.sub(sub); + } + } + } + Self::NoContextArgument { parameter } => { let range = all_arguments_range(node); if let Some(builder) = context.report_lint(&MISSING_CONTEXT_ARGUMENT, range) { diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs index 26842520b2..5ae93afbc9 100644 --- a/crates/ty_python_semantic/src/types/function.rs +++ b/crates/ty_python_semantic/src/types/function.rs @@ -374,6 +374,30 @@ pub struct DataclassTransformerParams<'db> { impl get_size2::GetSize for DataclassTransformerParams<'_> {} /// Whether a function should implicitly be treated as a staticmethod based on its name. +/// Returns `true` if `function` is decorated with `@overload` (the stub declaration, not the +/// implementation) or `@abstractmethod`, so that its body, and the `...` it writes for a +/// parameter's default, stand in for something it does not provide itself. +pub(crate) fn is_overload_or_abstractmethod<'db>( + db: &'db dyn Db, + function: Definition<'db>, + node: &ast::StmtFunctionDef, +) -> bool { + let decorators = infer_definition_types(db, function); + node.decorator_list.iter().any(|decorator| { + match decorators.expression_type(&decorator.expression) { + Type::FunctionLiteral(decorator) => matches!( + decorator.known(db), + Some(KnownFunction::Overload | KnownFunction::AbstractMethod) + ), + // In unreachable code, we infer `Never` for decorators like `typing.overload`. Count + // those too, to avoid false positive `invalid-return-type` lints for `@overload`ed + // functions without a body in unreachable code. + Type::Never | Type::Divergent(_) => true, + _ => false, + } + }) +} + pub(crate) fn is_implicit_staticmethod(function_name: &str) -> bool { matches!(function_name, "__new__") } diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index e3dba52428..57b7f02306 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -116,8 +116,8 @@ use crate::types::exceptions::CallSolution; use crate::types::extensions; use crate::types::format; use crate::types::function::{ - FunctionDecorators, FunctionType, KnownFunction, OverloadLiteral, report_revealed_type, - same_module_uncached_raw_signature, + FunctionDecorators, FunctionType, KnownFunction, OverloadLiteral, + is_overload_or_abstractmethod, report_revealed_type, same_module_uncached_raw_signature, }; use crate::types::generics::{ GenericContext, Specialization, SpecializationBuilder, bind_typevar, enclosing_binding_contexts, @@ -2701,20 +2701,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { self.function_type(self.current_function_definition()?) } - fn function_decorator_types<'a>( - &'a self, - function: &'a ast::StmtFunctionDef, - ) -> impl Iterator> + 'a { - let definition = self.index.expect_single_definition(function); - - let definition_types = infer_definition_types(self.db(), definition); - - function - .decorator_list - .iter() - .map(move |decorator| definition_types.expression_type(&decorator.expression)) - } - /// Returns `true` if the current scope is the function body scope of a function overload (that /// is, the stub declaration decorated with `@overload`, not the implementation), or an /// abstract method (decorated with `@abstractmethod`.) @@ -2722,24 +2708,11 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { let Some(function) = self.current_function_definition() else { return false; }; - - self.function_decorator_types(function) - .any(|decorator_type| { - match decorator_type { - Type::FunctionLiteral(function) => matches!( - function.known(self.db()), - Some(KnownFunction::Overload | KnownFunction::AbstractMethod) - ), - Type::Never => { - // In unreachable code, we infer `Never` for decorators like `typing.overload`. - // Return `true` here to avoid false positive `invalid-return-type` lints for - // `@overload`ed functions without a body in unreachable code. - true - } - Type::Divergent(_) => true, - _ => false, - } - }) + is_overload_or_abstractmethod( + self.db(), + self.index.expect_single_definition(function), + function, + ) } fn infer_body(&mut self, suite: &[ast::Stmt]) { diff --git a/crates/ty_python_semantic/src/types/infer/builder/function.rs b/crates/ty_python_semantic/src/types/infer/builder/function.rs index 95a0dc5606..91770ec965 100644 --- a/crates/ty_python_semantic/src/types/infer/builder/function.rs +++ b/crates/ty_python_semantic/src/types/infer/builder/function.rs @@ -41,7 +41,10 @@ use crate::{ inferred_signature::{can_implicitly_return_none, return_type_from_body}, lifetimes::InheritedBorrow, relation::TypeRelation, - signatures::{ReturnCallableTypeVarScope, function_signature_expression_type}, + signatures::{ + DefaultFit, ReturnCallableTypeVarScope, default_fit, + function_signature_expression_type, relate_default, + }, trailing_lambda::{ BlockCallee, UnbindableParameters, block_callee, trailing_lambda_it_borrow, trailing_lambda_it_type, @@ -1962,17 +1965,12 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { // Avoid duplicate diagnostics: invalid TypedDict literals already emit specific errors. let suppress_invalid_default = is_invalid_typed_dict_literal(db, env, declared_ty, default_expr.into()); + // basedpython: a default that fits only some specializations of the annotation's + // type variables initialises them, and each call that leaves the argument out + // is checked against the specialization it settles on if !default_ty.is_assignable_to(db, env, declared_ty) && !suppress_invalid_default - && !((self.in_stub() - || self.in_function_overload_or_abstractmethod() - || self.is_in_type_checking_block(self.scope(), default_expr) - || self - .class_context_of_current_method() - .is_some_and(|class| class.is_protocol(db))) - && default - .as_ref() - .is_some_and(|d| d.is_ellipsis_literal_expr())) + && default_fit(db, definition) == DefaultFit::Never { if let Some(builder) = self .context @@ -1988,6 +1986,24 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } } + // basedpython: an override that leaves the default out takes the overridden method's, + // which was written against the base's annotation rather than this one + if default_expr.is_none() + && self.is_basedpython_file() + && let Some(inherited) = self.inherited_parameter_default(parameter) + && relate_default(db, env, definition, declared_ty, inherited) == DefaultFit::Never + && let Some(builder) = self + .context + .report_lint(&INVALID_PARAMETER_DEFAULT, parameter_with_default) + { + builder.into_diagnostic(format_args!( + "Inherited default value of type `{}` is not assignable \ + to annotated parameter type `{}`", + inherited.display(db, env), + declared_ty.display(db, env) + )); + } + self.add_declaration_with_binding( parameter.into(), definition, @@ -2079,6 +2095,19 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { ty.is_inferred_parameter_hole(self.db()).then_some(ty) } + /// basedpython: the default the enclosing override takes for `parameter` from the method it + /// overrides, when it writes none of its own + fn inherited_parameter_default(&self, parameter: &ast::Parameter) -> Option> { + let db = self.db(); + let enclosing = nearest_enclosing_function(db, self.index, self.scope())?; + enclosing + .last_definition_raw_signature(db, ReturnCallableTypeVarScope::Public) + .parameters() + .iter() + .find(|candidate| candidate.name() == Some(¶meter.name.id))? + .eager_default_type() + } + /// The type the enclosing function's signature gives `parameter`, when that type was not /// written as an annotation. /// diff --git a/crates/ty_python_semantic/src/types/infer/builder/post_inference/overloaded_function.rs b/crates/ty_python_semantic/src/types/infer/builder/post_inference/overloaded_function.rs index ce9e15fc15..8ce08d272d 100644 --- a/crates/ty_python_semantic/src/types/infer/builder/post_inference/overloaded_function.rs +++ b/crates/ty_python_semantic/src/types/infer/builder/post_inference/overloaded_function.rs @@ -232,6 +232,7 @@ fn check_overload_set<'db>( implementation, &implementation_callables, ); + check_overload_initialising_defaults(context, overloads, implementation); } // Check that the overloaded function has at least two overloads @@ -569,6 +570,81 @@ fn check_non_generic_overload_implementation_consistency<'db>( } } +/// basedpython: check that the implementation runs with a default each overload's initialising +/// default describes +/// +/// a call that matches `@overload def f[T](t: T = 1) -> T` and leaves `t` out is solved from the +/// overload's `1`, but it is the implementation that runs, with its own default. that default +/// has to be a value the overload's describes, or the call's type says nothing about its result. +/// the overload consistency check above leaves generic overloads alone, and an initialising +/// default only occurs on a generic one, so this is checked here for every overload +fn check_overload_initialising_defaults<'db>( + context: &InferContext<'db, '_>, + overloads: &'db [OverloadLiteral<'db>], + implementation: OverloadLiteral<'db>, +) { + let db = context.db(); + let env = context.program_environment(); + let implementation_signature = implementation.signature(db); + let implementation_parameters = implementation_signature.parameters(); + for overload in overloads { + let signature = overload.signature(db); + let mut position = 0; + for (index, parameter) in signature.parameters().iter().enumerate() { + let counterpart = if parameter.is_positional() { + position += 1; + implementation_parameters.positional().nth(position - 1) + } else { + None + } + .or_else(|| { + implementation_parameters + .keyword_by_name(parameter.keyword_name()?) + .map(|(_, counterpart)| counterpart) + }); + let Some(promised) = parameter.initialising_default_type(db) else { + continue; + }; + let Some(counterpart) = counterpart else { + continue; + }; + let runs_with = counterpart.default_type(db); + if runs_with.is_some_and(|runs_with| runs_with.is_assignable_to(db, env, promised)) { + continue; + } + let function_node = overload.node(db, context.file(), context.module()); + let Some(builder) = context.report_lint(&INVALID_OVERLOAD, &function_node.name) else { + continue; + }; + let name = parameter + .name() + .map(ToString::to_string) + .unwrap_or_default(); + let mut diagnostic = match runs_with { + Some(runs_with) => builder.into_diagnostic(format_args!( + "Implementation's default for parameter `{name}` does not fit this overload's \ + default: expected `{}`, found `{}`", + promised.display(db, env), + runs_with.display(db, env), + )), + None => builder.into_diagnostic(format_args!( + "Implementation has no default for parameter `{name}`, which this overload \ + lets a call leave out" + )), + }; + let (_, parameter_span) = overload.parameter_span(db, Some(index)); + diagnostic.annotate( + Annotation::secondary(parameter_span).message("Overload's default declared here"), + ); + diagnostic.annotate( + context + .secondary(implementation.focus_range(db, context.module())) + .message(format_args!("Implementation defined here")), + ); + } + } +} + /// A decorator that is applied inconsistently across an overload set. struct BindingDecoratorInconsistency<'db, 'a> { /// The user-facing name of the decorator, without the leading `@`. diff --git a/crates/ty_python_semantic/src/types/infer/tests.rs b/crates/ty_python_semantic/src/types/infer/tests.rs index 44254adbf8..bca5a4e2ab 100644 --- a/crates/ty_python_semantic/src/types/infer/tests.rs +++ b/crates/ty_python_semantic/src/types/infer/tests.rs @@ -1094,6 +1094,46 @@ fn lazy_parameter_defaults() -> anyhow::Result<()> { Ok(()) } +/// basedpython: a call reads whether a default initialises a type variable of its parameter, which +/// means inferring a default whose annotation names one. that answer is a query of its own, so +/// editing such a default leaves the call alone as long as the answer stays the same +#[test] +fn lazy_generic_parameter_defaults() -> anyhow::Result<()> { + let mut db = setup_db(); + db.write_files([ + ( + "/src/defaults.py", + "def f[T](x: T | None = None) -> list[T]: return []", + ), + ("/src/main.py", "from defaults import f\nresult = f()"), + ])?; + let main = system_path_to_file(&db, "/src/main.py")?; + let result = global_symbol(&db, main, "result").place.expect_type(); + assert_eq!( + result.display(&db, &db.program_environment()).to_string(), + "list[Unknown]" + ); + + db.write_file( + "/src/defaults.py", + "def f[T](x: T | None = (None)) -> list[T]: return []", + )?; + db.clear_salsa_events(); + let result = global_symbol(&db, main, "result").place.expect_type(); + assert_eq!( + result.display(&db, &db.program_environment()).to_string(), + "list[Unknown]" + ); + let events = db.take_salsa_events(); + assert_function_query_was_not_run( + &db, + infer_definition_types, + first_public_binding(&db, main, "result"), + &events, + ); + Ok(()) +} + #[test] fn parameter_default_presence_invalidates_caller() -> anyhow::Result<()> { let mut db = setup_db(); diff --git a/crates/ty_python_semantic/src/types/signatures.rs b/crates/ty_python_semantic/src/types/signatures.rs index 72038fdcb0..61bce9b464 100644 --- a/crates/ty_python_semantic/src/types/signatures.rs +++ b/crates/ty_python_semantic/src/types/signatures.rs @@ -23,19 +23,22 @@ use smallvec::{SmallVec, smallvec_inline}; use super::{DynamicType, Type, TypeVarVariance, UnionType, semantic_index}; use crate::types::UnpackedKwargs; -use crate::types::callable::CallableTypeKind; +use crate::types::callable::{CallableTypeKind, walk_callable_type}; use crate::types::constraints::{ ConstraintSet, ConstraintSetBuilder, IteratorConstraintsExtension, OwnedConstraintSet, PathBounds, Solutions, }; use crate::types::cyclic::ActiveRecursionDetector; -use crate::types::function::{OverloadLiteral, deferred_assertion_guards}; +use crate::types::function::{ + FunctionType, OverloadLiteral, deferred_assertion_guards, is_overload_or_abstractmethod, + walk_function_type, +}; use crate::types::generics::{ ApplySpecialization, GenericContext, Specialization, SpecializationBuilder, TypeVarInference, walk_generic_context, }; use crate::types::infer::{ - TypeExpressionFlags, infer_deferred_types, infer_function_default_types, + TypeExpressionFlags, infer_deferred_types, infer_function_default_types, original_class_type, }; use crate::types::inferred_signature::inferred_parameter_type; use crate::types::instance::ProtocolInstanceType; @@ -49,6 +52,7 @@ use crate::types::typevar::{ MAX_TYPEVAR_FRESHNESS_DELTA, TypeVarInstance, TypeVarKind, TypeVarSet, max_typevar_freshness_matching_generic_context, }; +use crate::types::visitor::{TypeCollector, TypeVisitor, walk_type_with_recursion_guard}; use crate::types::{ ApplyTypeMappingVisitor, BindingContext, BoundTypeVarIdentity, BoundTypeVarInstance, CallableType, ErrorContext, ErrorContextTree, FindLegacyTypeVarsVisitor, KnownClass, @@ -60,6 +64,7 @@ use crate::{Db, FxOrderSet}; use ruff_db::parsed::parsed_module; use ruff_python_ast::helpers::ReturnGuardForm; use ruff_python_ast::{self as ast, ParameterBorrow, name::Name}; +use ruff_text_size::Ranged; use std::borrow::Cow; use ty_python_core::definition::{Definition, DefinitionKind, ParameterDefinitionNodeKind}; @@ -1713,7 +1718,19 @@ impl<'db> Signature<'db> { if inherited.display_default_value(db, env).is_none() { continue; } - parameter.set_default_type(inherited); + // the base wrote the default against its own annotation, so whether it initialises a + // type variable is a question about the override's, `def f[T](self, a: T)`. one that + // fits no specialization of the override's is reported where the override is written + let fit = parameter.definition.map(|definition| { + let declared = parameter.annotated_type.erase_overlapping(db, env); + relate_default(db, env, definition, declared, inherited) + }); + parameter.set_default_type(match fit { + Some(DefaultFit::Initialises(inherited)) => { + ParameterDefault::Initialising(inherited) + } + _ => ParameterDefault::Inferred(inherited), + }); } } @@ -3378,6 +3395,9 @@ impl<'c, 'db> TypeRelationChecker<'_, 'c, 'db> { .and(db, self.constraints, || { checker.check_signature_pair_inner(db, source, target) }) + .and(db, self.constraints, || { + checker.check_initialising_defaults(db, source, target) + }) }) }); @@ -3462,6 +3482,83 @@ impl<'c, 'db> TypeRelationChecker<'_, 'c, 'db> { } } + /// basedpython: relates the defaults of `source` to `target` wherever a call to `target` can + /// leave the parameter out, so that the call falls back on the default `source` runs with + /// + /// a parameter every call to `target` supplies never falls back on its default. otherwise: + /// + /// - when `target` declares the very same default, a call through `target` already checks it + /// against the specialization that call settles on, so it asks nothing more of `source` + /// - when `target`'s own default initialises a type variable, a caller of `target` solved the + /// call from that default, so the default `source` runs with has to be a value the target's + /// default describes: an override of `def m[T](self, t: T = 1) -> T` can't default to `"a"` + /// - when `source`'s default initialises a type variable of `source`, it constrains that type + /// variable as an argument would: `def f[T](t: T = 1) -> T` is a `() -> int` but not a + /// `() -> str` + /// + /// this runs once the parameter lists are known to line up, which is what lets a target + /// parameter be matched to its source parameter by position or by name alone. a gradual + /// target takes any call, and a `ParamSpec` target carries the source's parameters, defaults + /// included, on to the calls made through it, so only a standard target is checked + fn check_initialising_defaults( + &self, + db: &'db dyn Db, + source: &Signature<'db>, + target: &Signature<'db>, + ) -> ConstraintSet<'db, 'c> { + if !matches!(target.parameters().kind(), ParametersKind::Standard) { + return self.always(); + } + let target_parameters = target.parameters.expand_starred_variadic_annotations(db); + let target_positional = target_parameters + .iter() + .filter(|parameter| parameter.is_positional()) + .collect::; 4]>>(); + + let mut result = self.always(); + let mut position = 0; + for parameter in source.parameters() { + let counterpart = if parameter.is_positional() { + position += 1; + target_positional.get(position - 1).copied() + } else { + None + } + .or_else(|| { + let name = parameter.keyword_name()?; + target_parameters + .iter() + .find(|candidate| candidate.keyword_name() == Some(name)) + }); + let Some(default) = parameter.default() else { + continue; + }; + let constraint = match counterpart { + Some(counterpart) if !counterpart.has_default() => continue, + Some(counterpart) if counterpart.default() == Some(default) => continue, + counterpart => { + if let Some(promised) = counterpart + .and_then(|counterpart| counterpart.initialising_default_type(db)) + { + // a placeholder says nothing about the value the source runs with + if parameter.is_placeholder_default(db) { + continue; + } + (default.ty(db), promised) + } else if let Some(initialising) = parameter.initialising_default_type(db) { + (initialising, parameter.annotated_type()) + } else { + continue; + } + } + }; + result = result.and(db, self.constraints, || { + self.check_type_pair(db, constraint.0, constraint.1) + }); + } + result + } + fn check_signature_pair_inner( &self, db: &'db dyn Db, @@ -6777,7 +6874,7 @@ impl<'db> Parameter<'db> { } pub(crate) fn with_default_type(mut self, default: Type<'db>) -> Self { - self.set_default_type(default); + self.set_default_type(ParameterDefault::Inferred(default)); self } @@ -6785,12 +6882,12 @@ impl<'db> Parameter<'db> { /// /// A variadic parameter stands for a run of arguments rather than one, so there is nothing /// for a caller to leave out and nothing a default would mean. - fn set_default_type(&mut self, default: Type<'db>) { + fn set_default_type(&mut self, default: ParameterDefault<'db>) { match &mut self.kind { ParameterKind::PositionalOnly { default_type, .. } | ParameterKind::PositionalOrKeyword { default_type, .. } | ParameterKind::KeywordOnly { default_type, .. } => { - *default_type = Some(ParameterDefault::Inferred(default)); + *default_type = Some(default); } ParameterKind::Variadic { .. } | ParameterKind::KeywordVariadic { .. } => { panic!("cannot set default value for variadic parameter") @@ -6919,7 +7016,9 @@ impl<'db> Parameter<'db> { ParameterKind::PositionalOnly { default_type, .. } | ParameterKind::PositionalOrKeyword { default_type, .. } | ParameterKind::KeywordOnly { default_type, .. } => { - if let Some(ParameterDefault::Inferred(ty)) = default_type { + if let Some(ParameterDefault::Inferred(ty) | ParameterDefault::Initialising(ty)) = + default_type + { *ty = normalize_type(*ty)?; } } @@ -7213,6 +7312,26 @@ impl<'db> Parameter<'db> { self.default().and_then(ParameterDefault::eager_type) } + /// basedpython: the type of this parameter's default when a call that leaves the argument out + /// has to be specialized to accept it. see [`initialising_default_type`] + pub(crate) fn initialising_default_type(&self, db: &'db dyn Db) -> Option> { + match self.default()? { + ParameterDefault::Deferred(parameter) => initialising_default_type(db, parameter), + ParameterDefault::Initialising(ty) => Some(ty), + // a synthesized signature's default has no written parameter it was checked against + ParameterDefault::Inferred(_) => None, + } + } + + /// basedpython: whether this parameter's default is a placeholder `...` rather than a value + /// see [`is_placeholder_default`] + fn is_placeholder_default(&self, db: &'db dyn Db) -> bool { + matches!( + self.default(), + Some(ParameterDefault::Deferred(parameter)) if is_placeholder_default(db, parameter) + ) + } + /// Rewrites a positional-or-keyword parameter as keyword-only while preserving its metadata. fn positional_or_keyword_to_keyword_only(&self) -> Self { let mut result = self.clone(); @@ -7234,6 +7353,10 @@ impl<'db> Parameter<'db> { pub enum ParameterDefault<'db> { /// An already inferred default. Inferred(Type<'db>), + /// basedpython: an already inferred default that initialises a type variable of the + /// parameter's annotation — one an override inherits, where the override's annotation is not + /// the one the default was written against. see `initialising_default_type` + Initialising(Type<'db>), /// A source parameter whose default is inferred on demand. Deferred(Definition<'db>), } @@ -7241,14 +7364,14 @@ pub enum ParameterDefault<'db> { impl<'db> ParameterDefault<'db> { fn ty(self, db: &'db dyn Db) -> Type<'db> { match self { - Self::Inferred(ty) => ty, + Self::Inferred(ty) | Self::Initialising(ty) => ty, Self::Deferred(parameter) => parameter_default_type(db, parameter), } } fn eager_type(self) -> Option> { match self { - Self::Inferred(ty) => Some(ty), + Self::Inferred(ty) | Self::Initialising(ty) => Some(ty), Self::Deferred(_) => None, } } @@ -7256,6 +7379,7 @@ impl<'db> ParameterDefault<'db> { fn map_type(self, f: impl FnOnce(Type<'db>) -> Type<'db>) -> Self { match self { Self::Inferred(ty) => Self::Inferred(f(ty)), + Self::Initialising(ty) => Self::Initialising(f(ty)), // A source default is a runtime value, not part of the callable's type parameters. // Specializing or otherwise transforming the signature must not evaluate it. Self::Deferred(_) => self, @@ -7293,6 +7417,351 @@ fn parameter_default_type<'db>(db: &'db dyn Db, parameter: Definition<'db>) -> T .replace_parameter_defaults(db, &ProgramEnvironment::from_definition(function)) } +/// whether `parameter`'s default is the `...` a declaration writes in place of a value it does +/// not have: in a stub, on an `@overload` or `@abstractmethod`, on a protocol method, or under +/// `if TYPE_CHECKING:` +/// +/// such a default is not checked against the annotation, and it says nothing about the value a +/// call that leaves the argument out receives +#[salsa::tracked(returns(copy), heap_size=ruff_memory_usage::heap_size)] +pub(crate) fn is_placeholder_default<'db>(db: &'db dyn Db, parameter: Definition<'db>) -> bool { + let DefinitionKind::Parameter(ParameterDefinitionNodeKind::Parameter(node)) = + parameter.kind(db) + else { + return false; + }; + let scope = parameter.scope(db); + let Some(function) = scope.node(db).as_function() else { + return false; + }; + let program_file = parameter.program_file(db); + let module = parsed_module(db, program_file.python_file(db)).load(db); + let Some(default) = node.node(&module).default() else { + return false; + }; + if !default.is_ellipsis_literal_expr() { + return false; + } + if parameter.file(db).is_stub(db) { + return true; + } + let index = semantic_index(db, program_file); + let body_scope = scope.file_scope_id(db); + index.is_in_type_checking_block(body_scope, default.range()) + || is_overload_or_abstractmethod( + db, + index.expect_single_definition(function), + function.node(&module), + ) + || index + .class_definition_of_method(body_scope) + .and_then(|class| original_class_type(db, class)) + .is_some_and(|class| class.is_protocol(db)) +} + +/// basedpython: how a parameter's default relates to the specializations of its annotation +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, get_size2::GetSize, salsa::SalsaValue)] +pub(crate) enum DefaultFit<'db> { + /// the default fits whatever the annotation's type variables are, or there is no default or + /// annotation to relate + Every, + /// the `...` a declaration writes in place of a value. see [`is_placeholder_default`] + Placeholder, + /// the default fits some specializations of the annotation but not all of them, so a call + /// that leaves the argument out has to pick one it fits. see [`default_initialises`] + Initialises(Type<'db>), + /// no specialization a call can pick fits the default + Never, +} + +/// basedpython: how a default of type `default` relates to the annotation `declared` of +/// `parameter`, which it belongs to either as written or, for an override, as inherited +pub(crate) fn relate_default<'db>( + db: &'db dyn Db, + env: &ProgramEnvironment<'db>, + parameter: Definition<'db>, + declared: Type<'db>, + default: Type<'db>, +) -> DefaultFit<'db> { + if default.is_assignable_to(db, env, declared) { + DefaultFit::Every + } else if default_initialises(db, env, parameter, declared, default) { + DefaultFit::Initialises(default) + } else { + DefaultFit::Never + } +} + +/// basedpython: whether a default of type `default` initialises a type variable of the annotation +/// `declared` on `parameter`: it fits some specialization of the type variables that a call +/// solves or is given. [`relate_default`] asks this only of a default that does not fit every +/// one +/// +/// `1` initialises `T` in `def f[T](t: T = 1)`, which `f()` solves to `Literal[1]`. the type +/// variables a call supplies are those of the function itself and, for a method, of its class — +/// a method's call is given its class's by the receiver, as in `A[int]().m()`. a type variable of +/// an enclosing function is fixed in the body the function is defined in, so no call can +/// specialize it to fit +/// +/// a default's type is the type of the value made where the `def` runs, which every call shares, +/// so a type that names one of those type variables does not describe it: `[1]` against `list[T]` +/// is inferred as `list[T | int]`, but no call can make the list that is actually there hold its +/// `T`. such a default initialises nothing +fn default_initialises<'db>( + db: &'db dyn Db, + env: &ProgramEnvironment<'db>, + parameter: Definition<'db>, + declared: Type<'db>, + default: Type<'db>, +) -> bool { + let scope = parameter.scope(db); + let index = semantic_index(db, parameter.program_file(db)); + let Some(function) = scope.node(db).as_function() else { + return false; + }; + let function = BindingContext::Definition(index.expect_single_definition(function)); + let class = index + .class_definition_of_method(scope.file_scope_id(db)) + .map(BindingContext::Definition); + let is_supplied = |typevar: BoundTypeVarInstance<'db>| { + let context = typevar.binding_context(db); + context == function || Some(context) == class + }; + if names_free_typevar(db, env, default, &is_supplied) { + return false; + } + let supplied = std::cell::RefCell::new(FxOrderSet::default()); + any_over_type(db, env, declared, false, |ty: Type<'db>| { + if let Type::TypeVar(typevar) = ty + && is_supplied(typevar) + { + // `Callable[P, str]` names `P` through `P.args` and `P.kwargs`, but it is `P` a + // call solves + let typevar = if typevar.paramspec_attr(db).is_some() { + typevar.without_paramspec_attr(db) + } else { + typevar + }; + supplied.borrow_mut().insert(typevar); + } + false + }); + let supplied = supplied.into_inner(); + if supplied.is_empty() { + return false; + } + // solved the way a call that leaves the argument out and passes nothing else is solved, so + // that a default this accepts is one such a call accepts too + let constraints = ConstraintSetBuilder::new(); + let mut builder = SpecializationBuilder::new( + db, + env, + &constraints, + GenericContext::from_typevar_instances(db, env, supplied), + ); + if builder.infer(declared, default).is_err() { + return false; + } + let Ok(inference) = builder.build_inference_with(|_, _| None) else { + return false; + }; + let specialization = inference.merged_specialization(db); + default.is_assignable_to(db, env, declared.apply_specialization(db, specialization)) +} + +/// whether `ty` names a type variable `is_supplied` accepts where that type variable is free +/// +/// a generic function or callable inside `ty` binds its own type variables, so the default +/// `lambda: f` — whose type names `f`'s own generic signature — does not name `f`'s `T` at all +fn names_free_typevar<'db>( + db: &'db dyn Db, + env: &ProgramEnvironment<'db>, + ty: Type<'db>, + is_supplied: &dyn Fn(BoundTypeVarInstance<'db>) -> bool, +) -> bool { + struct FreeTypeVarVisitor<'db, 'a> { + env: &'a ProgramEnvironment<'db>, + is_supplied: &'a dyn Fn(BoundTypeVarInstance<'db>) -> bool, + bound: std::cell::RefCell>>, + recursion_guard: TypeCollector<'db>, + found: std::cell::Cell, + } + + impl<'db> FreeTypeVarVisitor<'db, '_> { + /// walks a generic signature with the type variables its own contexts bind set aside + fn visit_binding( + &self, + contexts: impl IntoIterator>, + walk: impl FnOnce(), + ) { + let depth = self.bound.borrow().len(); + self.bound.borrow_mut().extend(contexts); + walk(); + self.bound.borrow_mut().truncate(depth); + } + } + + impl<'db> TypeVisitor<'db> for FreeTypeVarVisitor<'db, '_> { + fn program_environment(&self) -> &ProgramEnvironment<'db> { + self.env + } + + fn should_visit_lazy_type_attributes(&self) -> bool { + false + } + + fn visit_type(&self, db: &'db dyn Db, ty: Type<'db>) { + if self.found.get() { + return; + } + if let Type::TypeVar(typevar) = ty + && (self.is_supplied)(typevar) + && !self + .bound + .borrow() + .iter() + .any(|context| context.contains(db, typevar.identity(db))) + { + self.found.set(true); + return; + } + walk_type_with_recursion_guard(db, ty, self, &self.recursion_guard); + } + + fn visit_function_type(&self, db: &'db dyn Db, function: FunctionType<'db>) { + let contexts = function + .signature(db) + .overloads + .iter() + .filter_map(|signature| signature.generic_context) + .collect::; 1]>>(); + self.visit_binding(contexts, || walk_function_type(db, function, self)); + } + + fn visit_callable_type(&self, db: &'db dyn Db, callable: CallableType<'db>) { + let contexts = callable + .signatures(db) + .iter() + .filter_map(|signature| signature.generic_context) + .collect::; 1]>>(); + self.visit_binding(contexts, || walk_callable_type(db, callable, self)); + } + } + + let visitor = FreeTypeVarVisitor { + env, + is_supplied, + bound: std::cell::RefCell::default(), + recursion_guard: TypeCollector::default(), + found: std::cell::Cell::new(false), + }; + visitor.visit_type(db, ty); + visitor.found.get() +} + +/// the annotation of a written parameter, as its default is related to it +fn parameter_declared_type<'db>( + db: &'db dyn Db, + parameter: Definition<'db>, +) -> Option<(Definition<'db>, Type<'db>)> { + let DefinitionKind::Parameter(ParameterDefinitionNodeKind::Parameter(node)) = + parameter.kind(db) + else { + return None; + }; + let function = parameter.scope(db).node(db).as_function()?; + let program_file = parameter.program_file(db); + let function = semantic_index(db, program_file).expect_single_definition(function); + let module = parsed_module(db, program_file.python_file(db)).load(db); + let node = node.node(&module); + node.default()?; + let annotation = node.parameter.annotation()?; + let env = ProgramEnvironment::from_definition(function); + Some(( + function, + function_signature_expression_type(db, function, annotation).erase_overlapping(db, &env), + )) +} + +/// basedpython: how `parameter`'s written default relates to its written annotation. this is the +/// one decision both the definition, which reports a default no specialization fits, and every +/// call that leaves the argument out, which passes an initialising default in its place, read +#[salsa::tracked( + returns(copy), + cycle_initial=|_, id, _| DefaultFit::Initialises(Type::divergent(id)), + cycle_fn=|db, cycle, previous: &DefaultFit<'db>, value: DefaultFit<'db>, parameter: Definition<'db>| { + // a default can name the function it belongs to, `def g[T](x: T = lambda: g())`, and + // then each round of solving `T` from it nests the previous round's answer + match (*previous, value) { + (DefaultFit::Initialises(previous), DefaultFit::Initialises(value)) => { + DefaultFit::Initialises(value.cycle_normalized( + db, + &ProgramEnvironment::from_definition(parameter), + previous, + cycle, + )) + } + _ => value, + } + }, + heap_size=ruff_memory_usage::heap_size +)] +pub(crate) fn default_fit<'db>(db: &'db dyn Db, parameter: Definition<'db>) -> DefaultFit<'db> { + let Some((function, declared)) = parameter_declared_type(db, parameter) else { + return DefaultFit::Every; + }; + if is_placeholder_default(db, parameter) { + return DefaultFit::Placeholder; + } + let env = ProgramEnvironment::from_definition(function); + relate_default( + db, + &env, + parameter, + declared, + parameter_default_type(db, parameter), + ) +} + +/// basedpython: the type of `parameter`'s default, when that default initialises a type variable +/// of the parameter's annotation — a call that leaves such an argument out is solved and checked +/// as if the default had been passed in its place. see [`default_fit`] +/// +/// this is what a call reads, and it is tracked so that the call depends on the answer rather +/// than on the source it is read from: editing a default that initialises nothing leaves every +/// call to its function alone +#[salsa::tracked( + returns(copy), + cycle_initial=|_, id, _| Some(Type::divergent(id)), + cycle_fn=|db, cycle, previous: &Option>, value: Option>, parameter: Definition<'db>| { + match (*previous, value) { + (Some(previous), Some(value)) => Some(value.cycle_normalized( + db, + &ProgramEnvironment::from_definition(parameter), + previous, + cycle, + )), + _ => value, + } + }, + heap_size=ruff_memory_usage::heap_size +)] +pub(crate) fn initialising_default_type<'db>( + db: &'db dyn Db, + parameter: Definition<'db>, +) -> Option> { + let (function, declared) = parameter_declared_type(db, parameter)?; + // an annotation without a type variable fits a default for every specialization or none, so + // the default need not be inferred at all — which is what most calls come down to + if !declared.has_typevar(db, &ProgramEnvironment::from_definition(function)) { + return None; + } + match default_fit(db, parameter) { + DefaultFit::Initialises(default) => Some(default), + DefaultFit::Every | DefaultFit::Placeholder | DefaultFit::Never => None, + } +} + #[derive(Clone, Debug, PartialEq, Eq, Hash, get_size2::GetSize, salsa::SalsaValue)] pub enum ParameterKind<'db> { /// Positional-only parameter, e.g. `def f(x, /): ...` diff --git a/crates/ty_server/src/db.rs b/crates/ty_server/src/db.rs index 9ddc746cf1..233f130377 100644 --- a/crates/ty_server/src/db.rs +++ b/crates/ty_server/src/db.rs @@ -1,6 +1,7 @@ use crate::NotebookDocument; use crate::session::index::Document; use crate::system::LSPSystem; +use lsp_types::Uri; use ruff_db::Db as _; use ruff_db::files::{File, FilePath}; use ty_project::{Db as ProjectDb, ProjectDatabase}; @@ -11,6 +12,10 @@ pub(crate) trait Db: ProjectDb { /// `None` if the file isn't open in the editor. fn document(&self, file: File) -> Option<&Document>; + /// Returns the LSP [`Document`] the client opened at `uri`, a notebook cell included, or + /// `None` if the client has not opened it. + fn document_at_uri(&self, uri: &Uri) -> Option<&Document>; + /// Returns the LSP [`NotebookDocument`] corresponding to `File` or /// `None` if the file isn't open in the editor or if it isn't a notebook. fn notebook_document(&self, file: File) -> Option<&NotebookDocument> { @@ -30,4 +35,11 @@ impl Db for ProjectDatabase { FilePath::Vendored(_) => None, }) } + + fn document_at_uri(&self, uri: &Uri) -> Option<&Document> { + self.system() + .as_any() + .downcast_ref::() + .and_then(|system| system.uri_to_document(uri)) + } } diff --git a/crates/ty_server/src/document.rs b/crates/ty_server/src/document.rs index 37583d26d2..c55d29f1f4 100644 --- a/crates/ty_server/src/document.rs +++ b/crates/ty_server/src/document.rs @@ -1,5 +1,6 @@ //! Types and utilities for working with text, modifying source files, and `ty <-> LSP` type conversion. +mod asked_text; mod location; mod notebook; mod range; @@ -9,6 +10,7 @@ use lsp_types::{PositionEncodingKind, Uri}; use ruff_db::system::{SystemPathBuf, SystemVirtualPath, SystemVirtualPathBuf}; use crate::system::AnySystemPath; +pub(crate) use asked_text::TextHash; pub(crate) use location::ToLink; pub use notebook::NotebookDocument; pub(crate) use range::{ diff --git a/crates/ty_server/src/document/asked_text.rs b/crates/ty_server/src/document/asked_text.rs new file mode 100644 index 0000000000..dbe34d1c64 --- /dev/null +++ b/crates/ty_server/src/document/asked_text.rs @@ -0,0 +1,153 @@ +//! Which text a document request is about, when the client says. +//! +//! A document request names a document by its URI, and the server answers about whatever it holds +//! for that URI when the request runs: the buffer the client last synchronised, or nothing at all +//! when the client has not opened it, which is refused. That is enough for a client that only asks +//! about documents it has opened and that sends every edit before the request that depends on it. +//! +//! It is not enough for a client that keeps an answer against a revision of its own. The server +//! cannot tell such a client which text an answer was for, so the client has to assume it was the +//! text it had when it asked, and there are two moments when that is not true: before the client's +//! `didOpen` for a document has arrived, and — for a document the client has not opened — while the +//! file on disk and the client's copy of it disagree. An answer about the wrong text, kept against +//! the client's revision, is wrong for as long as that revision lasts. +//! +//! So a client may name the text it means, by adding `textHash` to any document request's params: +//! the [`TextHash`] of the text the request is about. A request that names its text is answered +//! about that text and no other — +//! +//! - at once, if the text the server has for the document is that text: the open buffer, or, for +//! a handler that can answer a document that is not open, the file as the server reads it; +//! - otherwise once the server has it, which is the next time the session changes to hold it: a +//! `didOpen`, a `didChange`, or the file system reporting the file written. Until then the +//! request waits, and it is answered with `ServerCancelled` if it waits longer than +//! `HOLD_LIMIT` — a client that stops waiting sooner cancels it, as any request is cancelled. +//! +//! A request that names no text is answered as it always was. + +use std::fmt; + +/// The params field a client names its text in. +const TEXT_HASH_FIELD: &str = "textHash"; + +/// A hash of a document's text, as client and server both compute it. +/// +/// FNV-1a, 64 bits, over the text's UTF-16 code units, with each line ending counted as a single +/// `\n`, sent as sixteen lowercase hex digits. UTF-16 because that is what an editor's text is +/// likely to be made of already, so a client hashes what it holds without converting it. Line +/// endings are counted alike because they are the one way a file on disk and an editor's copy of it +/// routinely differ while meaning the same positions: an LSP position counts lines and the +/// characters within one, and `\r\n`, `\r` and `\n` each end a line. +/// +/// A byte order mark is *not* skipped. An editor drops it from its copy, and a server reading the +/// file keeps it, and the mark is a character on the first line — so the two do not name the same +/// positions, and should not hash alike. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct TextHash(u64); + +impl TextHash { + const OFFSET_BASIS: u64 = 0xcbf2_9ce4_8422_2325; + const PRIME: u64 = 0x0000_0100_0000_01b3; + + /// The hash of `text`. + pub(crate) fn of(text: &str) -> Self { + let mut hash = Self::OFFSET_BASIS; + let mut after_carriage_return = false; + for unit in text.encode_utf16() { + let unit = match unit { + // the `\n` of a `\r\n` has been counted already, as the `\r` + 0x0A if after_carriage_return => { + after_carriage_return = false; + continue; + } + 0x0D => { + after_carriage_return = true; + 0x0A + } + unit => { + after_carriage_return = false; + unit + } + }; + for byte in unit.to_le_bytes() { + hash ^= u64::from(byte); + hash = hash.wrapping_mul(Self::PRIME); + } + } + Self(hash) + } + + /// Reads the hash a client wrote: sixteen hex digits. + fn parse(value: &str) -> Option { + if value.len() != 16 { + return None; + } + u64::from_str_radix(value, 16).ok().map(Self) + } + + /// Takes the named text out of a request's params, so that the handler reads the params it + /// has always read. + /// + /// # Errors + /// + /// When the field is there but holds no hash — a client that meant to name its text and + /// cannot be answered as though it had not. + pub(crate) fn take_from(params: &mut serde_json::Value) -> anyhow::Result> { + let Some(value) = params + .as_object_mut() + .and_then(|params| params.remove(TEXT_HASH_FIELD)) + else { + return Ok(None); + }; + value + .as_str() + .and_then(Self::parse) + .map(Some) + .ok_or_else(|| anyhow::anyhow!("`{TEXT_HASH_FIELD}` is not a text hash: {value}")) + } +} + +impl fmt::Display for TextHash { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:016x}", self.0) + } +} + +#[cfg(test)] +mod tests { + use super::TextHash; + + #[test] + fn line_endings_hash_alike() { + let unix = TextHash::of("a\nb\n"); + assert_eq!(TextHash::of("a\r\nb\r\n"), unix); + assert_eq!(TextHash::of("a\rb\r"), unix); + assert_ne!(TextHash::of("a\n\nb\n"), unix); + // `\r\n\n` is two line endings, not one + assert_eq!(TextHash::of("a\r\n\nb"), TextHash::of("a\n\nb")); + } + + #[test] + fn a_byte_order_mark_is_part_of_the_text() { + assert_ne!(TextHash::of("\u{feff}x = 1\n"), TextHash::of("x = 1\n")); + } + + /// The values a client's implementation is checked against, so that the two are held to the + /// same function rather than to each other's bugs. + #[test] + fn known_values() { + assert_eq!(TextHash::of("").to_string(), "cbf29ce484222325"); + assert_eq!(TextHash::of("a").to_string(), "089be207b544f1e4"); + // a character outside the basic plane is its two surrogates, as an editor holds it + assert_eq!(TextHash::of("é𝄞\n").to_string(), "3a3901ff4b1faf17"); + assert_eq!(TextHash::of("é𝄞\r\n").to_string(), "3a3901ff4b1faf17"); + } + + #[test] + fn a_hash_reads_back() { + let hash = TextHash::of("def f():\n pass\n"); + assert_eq!(TextHash::parse(&hash.to_string()), Some(hash)); + assert_eq!(TextHash::parse("xyz"), None); + assert_eq!(TextHash::parse("0123456789abcdef0"), None); + } +} diff --git a/crates/ty_server/src/document/text_document.rs b/crates/ty_server/src/document/text_document.rs index 7bf65a229e..0d027b8e99 100644 --- a/crates/ty_server/src/document/text_document.rs +++ b/crates/ty_server/src/document/text_document.rs @@ -73,13 +73,17 @@ impl LanguageId { "python" | "by" | "basedpython" => Self::Python, "django-html" | "django-txt" | "htmldjango" | "django" => Self::DjangoTemplate, "jinja" | "jinja-html" | "jinja2" => Self::Other, - _ => match path.as_system() { - Some(path) if ty_ide::is_django_template_path(path) => Self::DjangoTemplate, - Some(path) if PySourceType::try_from_path(path.as_std_path()).is_some() => { - Self::Python - } - _ => Self::Other, - }, + _ => Self::of_path(path), + } + } + + /// The language of a file by its path alone: what a document the client has given no id + /// for — one it has not opened — is written in. + pub(crate) fn of_path(path: &AnySystemPath) -> Self { + match path.as_system() { + Some(path) if ty_ide::is_django_template_path(path) => Self::DjangoTemplate, + Some(path) if PySourceType::try_from_path(path.as_std_path()).is_some() => Self::Python, + _ => Self::Other, } } diff --git a/crates/ty_server/src/lib.rs b/crates/ty_server/src/lib.rs index 25ae32cfc1..8f8f856d70 100644 --- a/crates/ty_server/src/lib.rs +++ b/crates/ty_server/src/lib.rs @@ -67,6 +67,7 @@ pub fn run_server() -> anyhow::Result<()> { fallback_system, false, project_server_directory, + true, ) .context("Failed to start server")? .run(); diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index ea50ac3fe5..1544eb82db 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -56,6 +56,7 @@ impl Server { native_system: Arc, in_test: bool, project_server_directory: Option, + watch_file_system: bool, ) -> crate::Result { let (id, init_value) = connection.initialize_start()?; @@ -194,6 +195,7 @@ impl Server { native_system, ClientName::from(client_info), in_test, + watch_file_system, )?, }) } diff --git a/crates/ty_server/src/server/api.rs b/crates/ty_server/src/server/api.rs index b18c537a8c..af51453fae 100644 --- a/crates/ty_server/src/server/api.rs +++ b/crates/ty_server/src/server/api.rs @@ -1,11 +1,15 @@ +use crate::Db as _; +use crate::document::TextHash; use crate::server::schedule::Task; -use crate::session::Session; +use crate::session::{DocumentSnapshot, Session}; use anyhow::anyhow; use lsp_server as server; use lsp_server::{ErrorCode, RequestId}; use lsp_types::{LspNotificationMethod, Notification}; use lsp_types::{LspRequestMethod, Request}; +use ruff_db::source::source_text; use std::panic::{AssertUnwindSafe, UnwindSafe}; +use ty_project::ProjectDatabase; pub(super) mod changes; mod diagnostics; @@ -384,13 +388,17 @@ where } fn background_document_request_task( - req: server::Request, + mut req: server::Request, schedule: BackgroundSchedule, ) -> Result where <::RequestType as Request>::Params: UnwindSafe, { - let retry = R::RETRY_ON_CANCELLATION.then(|| req.clone()); + // Asked again as it came, the named text included, whether after a salsa cancellation or + // after being held for its text. + let as_sent = req.clone(); + let retry = R::RETRY_ON_CANCELLATION.then(|| as_sent.clone()); + let asked = TextHash::take_from(&mut req.params).with_failure_code(ErrorCode::InvalidParams)?; let (id, params) = cast_request::(req)?; Ok(Task::background(schedule, move |session: &Session| { @@ -400,25 +408,40 @@ where .cancellation_token(&id) .expect("request should have been tested for cancellation before scheduling"); - let uri = R::document_uri(¶ms); + let uri = R::document_uri(¶ms).into_owned(); + let revision = session.revision(); - let Ok(document) = session.snapshot_document(&uri) else { - let reason = format!("Document {uri} is not open in the session"); - tracing::warn!( - "Ignoring request id={id} method={} because {reason}", - R::METHOD - ); - return Box::new(|client| { - respond_silent_error( - id, - client, - lsp_server::ResponseError { - code: lsp_server::ErrorCode::InvalidParams as i32, - message: reason, - data: None, - }, + let document = match session.snapshot_document(&uri) { + Ok(document) => document, + Err(_) if asked.is_some() && R::ANSWERS_CLOSED_DOCUMENTS => { + match session.snapshot_closed_document(&uri) { + Some(document) => document, + // no file to read: the text can only come from the client opening it + None => return Box::new(move |client| client.hold(as_sent, revision)), + } + } + // the client has said what text it means, and opening the document is how it arrives + Err(_) if asked.is_some() => { + return Box::new(move |client| client.hold(as_sent, revision)); + } + Err(_) => { + let reason = format!("Document {uri} is not open in the session"); + tracing::warn!( + "Ignoring request id={id} method={} because {reason}", + R::METHOD ); - }); + return Box::new(|client| { + respond_silent_error( + id, + client, + lsp_server::ResponseError { + code: lsp_server::ErrorCode::InvalidParams as i32, + message: reason, + data: None, + }, + ); + }); + } }; let path = document.notebook_or_file_path(); @@ -443,6 +466,16 @@ where if let Err(error) = ruff_db::panic::catch_unwind(|| { salsa::attach(&db, || { + if let Some(asked) = asked + && !holds_text(&db, &document, asked) + { + tracing::debug!( + "Request id={id} method={} asks about text {asked}, which {uri} does not hold yet", + R::METHOD + ); + client.hold(as_sent, revision); + return; + } R::handle_request(&id, &db, document, client, params); }); }) { @@ -452,6 +485,23 @@ where })) } +/// Whether the text `document` names is the text a client asked about. +/// +/// For a document the client has opened, that is the text the client sent, and for a notebook +/// cell it is the cell's own text rather than the notebook's cells joined together. For one it +/// has not opened, it is the file as the server reads it. The hash is taken fresh: a text asked +/// about is usually the one the client just sent, and nothing is cached against it yet. +fn holds_text(db: &ProjectDatabase, document: &DocumentSnapshot, asked: TextHash) -> bool { + match db.document_at_uri(document.uri()) { + Some(open) => open + .as_text() + .is_some_and(|text| TextHash::of(text.contents()) == asked), + None => document + .to_notebook_or_file(db) + .is_some_and(|file| TextHash::of(source_text(db, file).as_str()) == asked), + } +} + fn panic_response( id: &RequestId, client: &Client, diff --git a/crates/ty_server/src/server/api/changes.rs b/crates/ty_server/src/server/api/changes.rs index b2d2c510d9..7171454f05 100644 --- a/crates/ty_server/src/server/api/changes.rs +++ b/crates/ty_server/src/server/api/changes.rs @@ -35,6 +35,10 @@ pub(crate) fn apply(session: &mut Session, client: &Client, changes: &[ChangeEve publish_settings_diagnostics(session, client, root); } + // a change can move what there is to watch: a new virtual environment, a configuration + // naming other search paths + session.update_file_watcher(client); + if client_capabilities.supports_workspace_diagnostic_refresh() { client.send_request::(session, (), |_, ()| {}); } else { diff --git a/crates/ty_server/src/server/api/requests/detached_transpile.rs b/crates/ty_server/src/server/api/requests/detached_transpile.rs index 914b8d0a15..3bcb5643a7 100644 --- a/crates/ty_server/src/server/api/requests/detached_transpile.rs +++ b/crates/ty_server/src/server/api/requests/detached_transpile.rs @@ -33,11 +33,36 @@ use ruff_db::panic::catch_unwind; /// caught only by the join would still have printed a raw backtrace to the server's /// log on the way out. Caught here it is logged the way the rest of the server logs /// a panicking handler, and the caller is told only that there is nothing to return. +/// +/// ## a cancellation is not a panic +/// +/// Salsa stops a query by unwinding it with a [`salsa::Cancelled`] payload: an edit +/// arriving while `op` reads the project database is a write, and a write waits for +/// every reader to let go of the revision it is replacing. That unwind is routine — it +/// happens whenever a document changes under a request, and in an editor that is +/// whenever someone types and then presses a button — and it is not an answer to +/// report. It is resumed on the calling thread instead, which is inside the request +/// dispatcher's own catch: that is where every other request's cancellation lands, +/// and where it becomes a retry against the new revision or a `ContentModified`, as +/// the handler chose, rather than being reported as a transpiler panic. +/// +/// Every variant goes back, `PropagatedPanic` included, because that is what the +/// dispatcher does with them: a query this one waited on having unwound is as often +/// that query's own cancellation as a panic, and a real panic there is met again, as +/// a panic, by whichever asks next — the retry, or the client asking again. pub(super) fn transpile_detached(op: impl FnOnce() -> R + Send) -> Result { let caught = std::thread::scope(|scope| scope.spawn(|| catch_unwind(AssertUnwindSafe(op))).join()); match caught { Ok(Ok(value)) => Ok(value), + Ok(Err(cancelled)) + if cancelled + .payload + .downcast_ref::() + .is_some() => + { + cancelled.resume_unwind() + } Ok(Err(panic)) => { tracing::error!("the transpiler panicked: {panic}"); Err(()) @@ -50,3 +75,38 @@ pub(super) fn transpile_detached(op: impl FnOnce() -> R + Send) -> Resu } } } + +#[cfg(test)] +mod tests { + use std::panic::{AssertUnwindSafe, catch_unwind, resume_unwind}; + + use super::transpile_detached; + + /// A panic in the transpiler is still a panic: the caller is told there is nothing to return, + /// and says so. + #[test] + fn a_panic_is_reported_as_nothing_to_return() { + let answer = transpile_detached(|| -> u32 { panic!("the transpiler is broken") }); + assert_eq!(answer, Err(())); + } + + /// A cancellation goes back up the calling thread with its payload intact, where the request + /// dispatcher reads it as one. `resume_unwind` of the payload is how salsa itself throws it. + #[test] + fn a_cancellation_goes_back_to_the_caller() { + for cancelled in [ + salsa::Cancelled::PendingWrite, + salsa::Cancelled::Local, + salsa::Cancelled::PropagatedPanic, + ] { + let unwound = catch_unwind(AssertUnwindSafe(|| { + transpile_detached(|| -> u32 { resume_unwind(Box::new(cancelled)) }) + })); + let payload = unwound.expect_err("a cancellation was answered as a result"); + assert!( + payload.downcast_ref::().is_some(), + "the unwind that came back is not salsa's cancellation" + ); + } + } +} diff --git a/crates/ty_server/src/server/api/requests/document_symbols.rs b/crates/ty_server/src/server/api/requests/document_symbols.rs index aff278821a..03a3776b93 100644 --- a/crates/ty_server/src/server/api/requests/document_symbols.rs +++ b/crates/ty_server/src/server/api/requests/document_symbols.rs @@ -25,6 +25,9 @@ impl RequestHandler for DocumentSymbolRequestHandler { } impl BackgroundDocumentRequestHandler for DocumentSymbolRequestHandler { + // a parse and what is read off it: the same for a file read from disk as for a buffer + const ANSWERS_CLOSED_DOCUMENTS: bool = true; + fn document_uri(params: &DocumentSymbolParams) -> Cow<'_, Uri> { Cow::Borrowed(¶ms.text_document.uri) } diff --git a/crates/ty_server/src/server/api/requests/injections.rs b/crates/ty_server/src/server/api/requests/injections.rs index b8474ac7fc..98b17399da 100644 --- a/crates/ty_server/src/server/api/requests/injections.rs +++ b/crates/ty_server/src/server/api/requests/injections.rs @@ -77,6 +77,9 @@ impl RequestHandler for InjectionsRequestHandler { } impl BackgroundDocumentRequestHandler for InjectionsRequestHandler { + // a parse and what is read off it: the same for a file read from disk as for a buffer + const ANSWERS_CLOSED_DOCUMENTS: bool = true; + fn document_uri(params: &InjectionsParams) -> Cow<'_, Uri> { Cow::Borrowed(¶ms.text_document.uri) } diff --git a/crates/ty_server/src/server/api/requests/semantic_tokens.rs b/crates/ty_server/src/server/api/requests/semantic_tokens.rs index 062e3cae95..857b266715 100644 --- a/crates/ty_server/src/server/api/requests/semantic_tokens.rs +++ b/crates/ty_server/src/server/api/requests/semantic_tokens.rs @@ -20,6 +20,9 @@ impl RequestHandler for SemanticTokensRequestHandler { } impl BackgroundDocumentRequestHandler for SemanticTokensRequestHandler { + // a parse and what is read off it: the same for a file read from disk as for a buffer + const ANSWERS_CLOSED_DOCUMENTS: bool = true; + fn document_uri(params: &SemanticTokensParams) -> Cow<'_, Uri> { Cow::Borrowed(¶ms.text_document.uri) } diff --git a/crates/ty_server/src/server/api/requests/syntax_outline.rs b/crates/ty_server/src/server/api/requests/syntax_outline.rs index 4aa0cedde8..a198736cf8 100644 --- a/crates/ty_server/src/server/api/requests/syntax_outline.rs +++ b/crates/ty_server/src/server/api/requests/syntax_outline.rs @@ -145,6 +145,9 @@ impl RequestHandler for SyntaxOutlineRequestHandler { } impl BackgroundDocumentRequestHandler for SyntaxOutlineRequestHandler { + // a parse and what is read off it: the same for a file read from disk as for a buffer + const ANSWERS_CLOSED_DOCUMENTS: bool = true; + fn document_uri(params: &SyntaxOutlineParams) -> Cow<'_, Uri> { Cow::Borrowed(¶ms.text_document.uri) } diff --git a/crates/ty_server/src/server/api/requests/transpile_for_build.rs b/crates/ty_server/src/server/api/requests/transpile_for_build.rs index 9567eacd0e..d007895ca1 100644 --- a/crates/ty_server/src/server/api/requests/transpile_for_build.rs +++ b/crates/ty_server/src/server/api/requests/transpile_for_build.rs @@ -136,7 +136,9 @@ impl BackgroundRequestHandler for TranspileForBuildRequestHandler { // A panic rather than a failure is reported as a refusal for the reason an error is: // a client has one shape to read, and a debugger that got no answer at all would - // leave the tree it is about to write into in an unknown state. + // leave the tree it is about to write into in an unknown state. Only a panic: an edit + // cancelling the transpile never gets here, but unwinds on up to the dispatcher, which + // asks again — see the `RetriableRequestHandler` below. let Ok(restaged) = restaged else { return Ok(Restage::refuse( "the transpiler panicked while re-staging these files", @@ -181,4 +183,14 @@ fn project_of<'a>( } } -impl RetriableRequestHandler for TranspileForBuildRequestHandler {} +/// Retried rather than answered `ContentModified` when an edit lands mid-request. +/// +/// A reload is a one-shot gesture the user is waiting on, like a rename, and the edit that cancels +/// it is routinely the user's own: typing and then pressing Reload sends the keystrokes' `didChange` +/// after the request, and it arrives while the transpile is reading the database the change is +/// written to. `ContentModified` would hand that race to every client to re-ask, and the re-ask is +/// this same request against the same new revision — the text the editor now holds, which is what +/// the request is documented to transpile. So it is asked again here. +impl RetriableRequestHandler for TranspileForBuildRequestHandler { + const RETRY_ON_CANCELLATION: bool = true; +} diff --git a/crates/ty_server/src/server/api/traits.rs b/crates/ty_server/src/server/api/traits.rs index 35ae7f674b..dec76020ac 100644 --- a/crates/ty_server/src/server/api/traits.rs +++ b/crates/ty_server/src/server/api/traits.rs @@ -84,6 +84,18 @@ pub(super) trait RetriableRequestHandler: RequestHandler { /// /// This handler is specific to requests that operate on a single document. pub(super) trait BackgroundDocumentRequestHandler: RetriableRequestHandler { + /// Whether a document the client has not opened is answered from the file as the server + /// reads it, for a request that says which text it is about. + /// + /// A request that does not say is refused for a closed document, whatever this says: the + /// server cannot know that the client's copy is the file on disk. One that says is held until + /// the server has that text, and this decides whether the file on disk may be it, or only a + /// buffer the client opens. See [`crate::document::TextHash`]. + /// + /// The snapshot of a closed document carries no version the client gave it, so a handler that + /// reads the version cannot say yes. + const ANSWERS_CLOSED_DOCUMENTS: bool = false; + /// Returns the URI of the document that this request handler operates on. fn document_uri( params: &<::RequestType as Request>::Params, diff --git a/crates/ty_server/src/server/main_loop.rs b/crates/ty_server/src/server/main_loop.rs index f6001634a3..3154f0293a 100644 --- a/crates/ty_server/src/server/main_loop.rs +++ b/crates/ty_server/src/server/main_loop.rs @@ -183,6 +183,14 @@ impl Server { ); } + Action::ApplyFileSystemChanges(changes) => { + api::changes::apply(&mut self.session, &client, &changes); + } + + Action::HoldRequest { request, revision } => { + self.session.hold_request(request, revision, &client); + } + Action::SuspendWorkspaceDiagnostics(suspended_request) => { self.session.set_suspended_workspace_diagnostics_request( *suspended_request, @@ -205,6 +213,7 @@ impl Server { Event::PollUvEnvironments { project_root } => { self.session.poll_uv_sync(&client, &project_root); } + Event::HeldRequestDue => {} } } @@ -215,6 +224,14 @@ impl Server { /// /// Returns `Ok(None)` if the client connection is closed. fn next_event(&mut self) -> Result, crossbeam::channel::RecvError> { + // Here rather than after each event, because every event ends up here next — including + // the ones that end early — and a request held for a text the last event supplied + // should not wait for the event after it. + self.session.release_held_requests(&Client::new( + self.main_loop_sender.clone(), + self.connection.sender.clone(), + )); + // We can't queue those into the main loop because that could result in reordering if // the `select` below picks a client message first. if let Some(deferred) = self.session.take_deferred_messages() { @@ -234,7 +251,11 @@ impl Server { } let uv_sync = UvSyncWakeups(self.session.uv_sync_wakeups()); - uv_sync.select(&self.connection.receiver, &self.main_loop_receiver) + uv_sync.select( + &self.connection.receiver, + &self.main_loop_receiver, + self.session.held_requests_deadline(), + ) } /// Schedules a `by` command line's request against a snapshot of this session. @@ -349,9 +370,19 @@ pub(crate) enum Action { SuspendWorkspaceDiagnostics(Box), + /// Keep a document request until the session holds the text it asks about. + HoldRequest { + request: lsp_server::Request, + /// The session's revision when the request found its text missing. + revision: u64, + }, + /// Re-read the file system, after the server itself changed something in it. RescanProjects, + /// Apply what the session's own file system watcher saw. + ApplyFileSystemChanges(Vec), + /// Initialize the workspace after the server received /// the options from the client. InitializeWorkspaces(Vec<(Uri, ClientOptions)>), @@ -370,6 +401,9 @@ pub(crate) enum Event { PollUvEnvironments { project_root: SystemPathBuf, }, + + /// A held request has waited as long as it may. Answered on the way to the next event. + HeldRequestDue, } pub(crate) struct SendRequest { @@ -391,11 +425,13 @@ impl std::fmt::Debug for SendRequest { struct UvSyncWakeups(Vec<(SystemPathBuf, crossbeam::channel::Receiver<()>)>); impl UvSyncWakeups { - /// Waits for a project wakeup, client message, or main-loop action. + /// Waits for a project wakeup, client message, or main-loop action — or, when a request is + /// held, for its `deadline`. fn select( &self, connection: &crossbeam::channel::Receiver, main_loop: &MainLoopReceiver, + deadline: Option, ) -> Result, crossbeam::channel::RecvError> { let mut select = crossbeam::channel::Select::new_biased(); for (_, receiver) in &self.0 { @@ -403,7 +439,15 @@ impl UvSyncWakeups { } let connection_index = select.recv(connection); let main_loop_index = select.recv(main_loop); - let operation = select.select(); + let operation = match deadline { + None => select.select(), + Some(deadline) => match select.select_deadline(deadline) { + Ok(operation) => operation, + Err(crossbeam::channel::SelectTimeoutError) => { + return Ok(Some(Event::HeldRequestDue)); + } + }, + }; let index = operation.index(); if let Some((project_root, receiver)) = self.0.get(index) { diff --git a/crates/ty_server/src/session.rs b/crates/ty_server/src/session.rs index 663cf211ce..8dc309d207 100644 --- a/crates/ty_server/src/session.rs +++ b/crates/ty_server/src/session.rs @@ -24,7 +24,7 @@ use ruff_db::system::{System, SystemPath, SystemPathBuf}; use ruff_python_ast::PySourceType; use ty_combine::Combine; use ty_project::metadata::Options; -use ty_project::watch::ChangeEvent; +use ty_project::watch::{ChangeEvent, ProjectWatcher}; use ty_project::{ ChangeResult, Db as _, ProjectDatabase, ProjectMetadata, ProjectReloadResult, ScriptEnvironmentAvailability, UseUv, UvSyncChanges, @@ -116,12 +116,28 @@ pub(crate) struct Session { /// to see if there are now changes and, if so, respond to the client. suspended_workspace_diagnostics_request: Option, + /// Document requests about a text the session does not hold yet, each waiting for the session + /// to change. See [`HeldRequest`]. + held_requests: Vec, + /// Registrations is a set of LSP methods that have been dynamically registered with the /// client. registrations: HashSet, /// The name of the client (editor) that connected to this server. client_name: ClientName, + + /// Whether this session learns of file system changes by watching the file system itself. + /// + /// Cleared when the watcher cannot be started, which leaves the client's watcher as the + /// only source of changes. + watch_file_system: bool, + + /// The session's own file system watcher, over every project's paths. + /// + /// `None` until the first projects exist, and for good when [`Self::watch_file_system`] is + /// off. + file_watcher: Option, } /// LSP State for a Project @@ -151,6 +167,10 @@ pub(crate) struct ProjectState { } impl Session { + #[expect( + clippy::too_many_arguments, + reason = "one caller, which has each of these to hand" + )] pub(crate) fn new( resolved_client_capabilities: ResolvedClientCapabilities, position_encoding: PositionEncoding, @@ -159,6 +179,7 @@ impl Session { native_system: Arc, client_name: ClientName, in_test: bool, + watch_file_system: bool, ) -> crate::Result { let index = Arc::new(Index::new()); @@ -187,9 +208,12 @@ impl Session { shutdown_requested: false, in_test, suspended_workspace_diagnostics_request: None, + held_requests: Vec::new(), revision: 0, registrations: HashSet::new(), client_name, + watch_file_system, + file_watcher: None, }) } @@ -209,6 +233,11 @@ impl Session { &self.initialization_options } + /// The session's revision: how many times it has changed in a way an answer could see. + pub(crate) fn revision(&self) -> u64 { + self.revision + } + pub(crate) fn is_shutdown_requested(&self) -> bool { self.shutdown_requested } @@ -260,6 +289,82 @@ impl Session { }); } + /// Keeps `request` until the session changes, because what it asks about is a text the session + /// did not hold at `revision`. + /// + /// Asked again at once if the session has changed since then: the change may be the one it + /// waits for, and nothing would wake it otherwise. + pub(crate) fn hold_request( + &mut self, + request: lsp_server::Request, + revision: u64, + client: &Client, + ) { + if !self.request_queue.incoming().is_pending(&request.id) { + tracing::debug!("Not holding request {}: it was cancelled", request.id); + return; + } + if revision == self.revision { + tracing::debug!( + "Holding request id={} method={} until the session holds the text it asks about", + request.id, + request.method + ); + self.held_requests.push(HeldRequest { request, revision }); + } else { + client.retry(request); + } + } + + /// Asks every held request again whose session has changed since it was held, and answers + /// those that have waited longer than [`HOLD_LIMIT`]. Forgets the ones the client cancelled. + pub(crate) fn release_held_requests(&mut self, client: &Client) { + if self.held_requests.is_empty() { + return; + } + let revision = self.revision; + let incoming = self.request_queue.incoming(); + let mut expired = Vec::new(); + self.held_requests.retain_mut(|held| { + let Some(started) = incoming.started(&held.request.id) else { + // cancelled, and answered as cancelled already + return false; + }; + if held.revision != revision { + client.retry(held.request.clone()); + false + } else if started.elapsed() >= HOLD_LIMIT { + expired.push(held.request.id.clone()); + false + } else { + true + } + }); + for id in expired { + tracing::debug!( + "Request {id} waited {HOLD_LIMIT:?} for a text the session never held; giving up" + ); + client.respond_err( + id, + lsp_server::ResponseError { + code: lsp_server::ErrorCode::ServerCancelled as i32, + message: "the document never held the text the request asked about".to_string(), + data: None, + }, + ); + } + } + + /// When the longest-held request is due an answer anyway, if any request is held. + pub(crate) fn held_requests_deadline(&self) -> Option { + let incoming = self.request_queue.incoming(); + self.held_requests + .iter() + .filter_map(|held| incoming.started(&held.request.id)) + .min() + .map(|started| started + HOLD_LIMIT) + } + /// Returns each project's background uv synchronization wakeups. pub(crate) fn uv_sync_wakeups(&self) -> Vec<(SystemPathBuf, crossbeam::channel::Receiver<()>)> { self.projects @@ -309,6 +414,8 @@ impl Session { if changes.project.is_some() { publish_settings_diagnostics(self, client, project_root.to_path_buf()); + // a synchronized environment is new search paths to watch + self.update_file_watcher(client); } self.bump_revision(); @@ -687,9 +794,83 @@ impl Session { } } + self.watch_projects(client); self.register_capabilities(client); } + /// Brings the session's own file system watcher up to date with the projects, and asks + /// the client to watch instead wherever it cannot. + /// + /// Called after anything that can change what there is to watch. Cheap when nothing did. + pub(crate) fn update_file_watcher(&mut self, client: &Client) { + let client_was_needed = self.needs_client_file_watcher(); + self.watch_projects(client); + if client_was_needed != self.needs_client_file_watcher() + && self.workspaces.all_initialized() + { + self.register_capabilities(client); + } + } + + /// Watches what every project needs watched: its root, its included paths, the module + /// search paths outside it, and its configuration files. + /// + /// ## why the server watches, when the client offers to + /// + /// A client's `workspace/didChangeWatchedFiles` says a file changed, and the only thing + /// the server can do with that is read the file. That is only right if the change is + /// on disk by the time the notification is read, and the protocol does not say it has to + /// be. PyCharm's client sends the notification when its virtual file system takes a save, + /// while the bytes can still be queued to be written: the server then reads the old text, + /// nothing tells it again once the write lands, and every open file depending on the saved + /// one keeps its diagnostics, hover, and types from before the save. + /// + /// The operating system reports a write after it happens, so a change reaches the session + /// no earlier than the bytes do. + /// + /// The client is not asked to watch as well while this one works; see + /// [`Self::needs_client_file_watcher`]. + fn watch_projects(&mut self, client: &Client) { + if !self.watch_file_system { + return; + } + if self.projects.is_empty() { + // the last workspace folder was removed: nothing left to watch + self.file_watcher = None; + return; + } + + let dbs = self.projects.values().map(|project| &project.db); + if let Some(watcher) = &mut self.file_watcher { + watcher.update_projects(dbs); + return; + } + + match ty_project::watch::directory_watcher(client.file_system_changes()) { + Ok(watcher) => self.file_watcher = Some(ProjectWatcher::for_projects(watcher, dbs)), + Err(error) => { + tracing::warn!( + "Failed to start watching the file system, asking the client to report changes instead: {error}" + ); + self.watch_file_system = false; + } + } + } + + /// Whether the client has to report file changes, because the session's own watcher is not + /// running or could not watch every path. + /// + /// Not both. A client that reports a change before it is on disk makes the session read the + /// old text, which this watcher then corrects when the write lands: a second pass over the + /// same change, and a second refresh of everything the editor shows, for every save. A caller + /// that has to be answered from the new text at once cannot rely on either watcher, and has + /// to say so itself: write the file, then send `workspace/didChangeWatchedFiles`, then ask. + fn needs_client_file_watcher(&self) -> bool { + self.file_watcher + .as_ref() + .is_none_or(ProjectWatcher::has_errored_paths) + } + /// Initializes a single workspace folder with the given URI /// and options. /// @@ -1037,6 +1218,7 @@ impl Session { self.clear_diagnostics_if_needed(&doc, client); } + self.update_file_watcher(client); self.bump_revision(); Ok(()) @@ -1143,16 +1325,18 @@ impl Session { } } - if let Some(register_options) = self.file_watcher_registration_options() { - if self - .registrations - .contains(DidChangeWatchedFilesNotification::METHOD.as_str()) - { - unregistrations.push(Unregistration { - id: FILE_WATCHER_REGISTRATION_ID.into(), - method: DidChangeWatchedFilesNotification::METHOD.into(), - }); - } + let file_watcher_options = self.file_watcher_registration_options(); + if self + .registrations + .contains(DidChangeWatchedFilesNotification::METHOD.as_str()) + { + // withdrawn as well as replaced: the session may have taken over watching + unregistrations.push(Unregistration { + id: FILE_WATCHER_REGISTRATION_ID.into(), + method: DidChangeWatchedFilesNotification::METHOD.into(), + }); + } + if let Some(register_options) = file_watcher_options { registrations.push(Registration { id: FILE_WATCHER_REGISTRATION_ID.into(), method: DidChangeWatchedFilesNotification::METHOD.into(), @@ -1249,6 +1433,11 @@ impl Session { } } + if !self.needs_client_file_watcher() { + tracing::debug!("Not asking the client to watch files: the server watches them itself"); + return None; + } + if !self.client_capabilities().supports_file_watcher() { tracing::warn!( "Your LSP client doesn't support file watching: \ @@ -1317,6 +1506,32 @@ impl Session { }) } + /// A snapshot of a document the client has not opened, as the file it names reads now. + /// + /// Only for a request that has said which text it is about — see + /// [`crate::document::TextHash`] — and only for a file on disk: a document with no path is + /// nothing until the client opens it. The handle carries version `0`: the client never gave + /// the document a version, and the handlers that answer a closed document read none. + pub(crate) fn snapshot_closed_document(&self, uri: &Uri) -> Option { + let path = DocumentKey::from_uri(uri).file_path()?.clone(); + let path = AnySystemPath::System(path); + Some(DocumentSnapshot { + resolved_client_capabilities: self.resolved_client_capabilities, + global_settings: self.global_settings.clone(), + workspace_settings: self + .workspace_settings_for_document(&path) + .unwrap_or_else(|| Arc::new(WorkspaceSettings::default())), + position_encoding: self.position_encoding, + document: DocumentHandle::Text { + uri: uri.clone(), + language_id: LanguageId::of_path(&path), + path, + version: 0, + }, + client_name: self.client_name, + }) + } + fn workspace_settings_for_document( &self, path: &AnySystemPath, @@ -1966,6 +2181,27 @@ impl SuspendedWorkspaceDiagnosticRequest { } } +/// How long a request may wait for its text before it is answered with `ServerCancelled`. +/// +/// A backstop, not a timing anyone should meet. What a request waits for is a notification the +/// client has already decided to send — a `didOpen` behind the edit that made the request, or the +/// file system reporting a write — and it arrives in milliseconds. A client that gives up sooner +/// cancels the request, as it cancels any other. +const HOLD_LIMIT: std::time::Duration = std::time::Duration::from_secs(10); + +/// A document request that named a text the session did not hold, waiting for it. +/// +/// Asked again, whole, each time the session changes, rather than told what changed: the check +/// that holds it is the same one that lets it through, and it runs against a fresh snapshot. +#[derive(Debug)] +pub(crate) struct HeldRequest { + /// The request as the client sent it, the named text included. + request: lsp_server::Request, + + /// The session's revision when the request found its text missing. + revision: u64, +} + /// A handle to a document stored within [`Index`]. /// /// Allows identifying the document within the index but it also carries the URI used by the diff --git a/crates/ty_server/src/session/client.rs b/crates/ty_server/src/session/client.rs index bad3e3e57c..204fce8313 100644 --- a/crates/ty_server/src/session/client.rs +++ b/crates/ty_server/src/session/client.rs @@ -74,6 +74,24 @@ impl Client { .unwrap(); } + /// Where the session's own file system watcher delivers what it saw: to the main loop, to be + /// applied as a client's watched-files notification is. + pub(crate) fn file_system_changes( + &self, + ) -> impl Fn(Vec) + Send + 'static { + let sender = self.main_loop_sender.clone(); + move |changes| { + // the main loop is gone only once the server is shutting down, and a change it + // would have applied no longer matters + if sender + .send(Event::Action(Action::ApplyFileSystemChanges(changes))) + .is_err() + { + tracing::debug!("Dropped file system changes because the main loop is closed"); + } + } + } + /// Asks the main loop to re-read the file system for every project. /// /// This is for a background task that changed something outside the editor — @@ -249,6 +267,12 @@ impl Client { .unwrap(); } + /// Keeps this request until the session changes, because the text it asks about is not the + /// text the session had at `revision`. See [`crate::document::TextHash`]. + pub(crate) fn hold(&self, request: lsp_server::Request, revision: u64) { + self.queue_action(Action::HoldRequest { request, revision }); + } + pub(crate) fn queue_action(&self, action: Action) { self.main_loop_sender.send(Event::Action(action)).unwrap(); } diff --git a/crates/ty_server/src/session/request_queue.rs b/crates/ty_server/src/session/request_queue.rs index 60c601ae92..1e1fb0c14d 100644 --- a/crates/ty_server/src/session/request_queue.rs +++ b/crates/ty_server/src/session/request_queue.rs @@ -80,6 +80,13 @@ impl Incoming { self.pending.contains_key(request_id) } + /// When the request with the given id was registered, if it is still pending. + pub(crate) fn started(&self, request_id: &RequestId) -> Option { + self.pending + .get(request_id) + .map(|pending| pending.start_time) + } + /// Returns the cancellation token for the given request id if the request is still pending. pub(crate) fn cancellation_token( &self, diff --git a/crates/ty_server/src/system.rs b/crates/ty_server/src/system.rs index b5fd544d30..03e1126947 100644 --- a/crates/ty_server/src/system.rs +++ b/crates/ty_server/src/system.rs @@ -164,6 +164,10 @@ impl LSPSystem { let any_path = AnySystemPath::SystemVirtual(path.to_path_buf()); self.document(any_path) } + + pub(crate) fn uri_to_document(&self, uri: &Uri) -> Option<&Document> { + self.index().document(&DocumentKey::from_uri(uri)).ok() + } } impl System for LSPSystem { diff --git a/crates/ty_server/tests/e2e/file_watching.rs b/crates/ty_server/tests/e2e/file_watching.rs new file mode 100644 index 0000000000..5a74ce6b3a --- /dev/null +++ b/crates/ty_server/tests/e2e/file_watching.rs @@ -0,0 +1,179 @@ +//! The server watching the file system itself, as `by server` does. +//! +//! These tests read real file system events from the operating system. Like the command line's +//! file-watching tests, they do not work inside a sandbox that withholds those events. + +use anyhow::Result; +use lsp_types::{DocumentDiagnosticReport, Message, RegistrationRequest}; +use ruff_db::system::SystemPath; + +use crate::{TestServer, TestServerBuilder}; + +const LIB_INT: &str = "def make() -> int:\n return 1\n"; +const LIB_STR: &str = "def make() -> str:\n return ''\n"; +const USER: &str = "from lib import make\n\nx: bytes = make()\n"; + +/// The messages of the diagnostics the server has for `path` now. +fn diagnostic_messages(server: &mut TestServer, path: &SystemPath) -> Vec { + match server.document_diagnostic_request(path, None) { + DocumentDiagnosticReport::RelatedFullDocumentDiagnosticReport(report) => report + .full_document_diagnostic_report + .items + .into_iter() + .map(|diagnostic| { + let Message::String(message) = diagnostic.message else { + panic!( + "a diagnostic message should be a string, and was {:#?}", + diagnostic.message + ) + }; + message + }) + .collect(), + DocumentDiagnosticReport::RelatedUnchangedDocumentDiagnosticReport(_) => { + panic!("asked without a previous result id") + } + } +} + +/// Waits for the server to say something changed, then asks again, until `user`'s diagnostics +/// satisfy `done`. Nothing but the file system tells the server about the change. +fn diagnostics_after_change( + server: &mut TestServer, + user: &SystemPath, + done: impl Fn(&[String]) -> bool, +) -> Vec { + loop { + server.await_diagnostic_refresh(); + let messages = diagnostic_messages(server, user); + if done(&messages) { + return messages; + } + } +} + +/// A file that is not open, rewritten on disk, reaches the diagnostics of an open file that +/// depends on it, with no `workspace/didChangeWatchedFiles` from the client. The server hears of +/// the write from the operating system, which reports it only once it has happened. +#[test] +fn a_write_reaches_an_open_dependent_without_the_client() -> Result<()> { + let lib = SystemPath::new("src/lib.py"); + let user = SystemPath::new("src/user.py"); + + let mut server = TestServerBuilder::new()? + .with_workspace(SystemPath::new("src"), None)? + .with_file(lib, LIB_INT)? + .with_file(user, USER)? + .enable_pull_diagnostics(true) + .enable_workspace_diagnostic_refresh(true) + .watch_file_system() + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(user, USER, 1); + let before = diagnostic_messages(&mut server, user); + assert!( + before.iter().any(|message| message.contains("`int`")), + "{before:#?}" + ); + + server.write_file(lib, LIB_STR)?; + + let after = diagnostics_after_change(&mut server, user, |messages| { + messages.iter().any(|message| message.contains("`str`")) + }); + assert!( + after.iter().all(|message| !message.contains("`int`")), + "{after:#?}" + ); + + Ok(()) +} + +/// The same through a workspace folder the client names by a symbolic link. The operating system +/// reports a change at the path it resolved to; the server knows the file by the path it was +/// given. +#[cfg(unix)] +#[test] +fn a_write_reaches_an_open_dependent_through_a_linked_workspace() -> Result<()> { + let lib = SystemPath::new("linked/lib.py"); + let user = SystemPath::new("linked/user.py"); + + let builder = TestServerBuilder::new()? + .with_file(SystemPath::new("real/lib.py"), LIB_INT)? + .with_file(SystemPath::new("real/user.py"), USER)?; + std::os::unix::fs::symlink( + builder.file_path("real").as_std_path(), + builder.file_path("linked").as_std_path(), + )?; + let mut server = builder + .with_workspace(SystemPath::new("linked"), None)? + .enable_pull_diagnostics(true) + .enable_workspace_diagnostic_refresh(true) + .watch_file_system() + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(user, USER, 1); + let before = diagnostic_messages(&mut server, user); + assert!( + before.iter().any(|message| message.contains("`int`")), + "{before:#?}" + ); + + server.write_file(lib, LIB_STR)?; + + diagnostics_after_change(&mut server, user, |messages| { + messages.iter().any(|message| message.contains("`str`")) + }); + + Ok(()) +} + +/// A client that offers to watch files is not asked to while the server watches them itself. +/// Its reports would be a second pass over every change, and a client can report a change before +/// the change is on disk. +#[test] +fn the_client_is_not_asked_to_watch_what_the_server_watches() -> Result<()> { + let mut server = TestServerBuilder::new()? + .with_workspace(SystemPath::new("src"), None)? + .enable_did_change_watched_files(true) + .enable_diagnostic_dynamic_registration(true) + .watch_file_system() + .build() + .wait_until_workspaces_are_initialized(); + + let (_, params) = server.await_request::(); + let methods: Vec<_> = params + .registrations + .iter() + .map(|registration| registration.method.as_str()) + .collect(); + assert_eq!(methods, ["textDocument/diagnostic"]); + + Ok(()) +} + +/// A server that does not watch for itself asks the client to. +#[test] +fn the_client_is_asked_to_watch_when_the_server_does_not() -> Result<()> { + let mut server = TestServerBuilder::new()? + .with_workspace(SystemPath::new("src"), None)? + .enable_did_change_watched_files(true) + .enable_diagnostic_dynamic_registration(true) + .build() + .wait_until_workspaces_are_initialized(); + + let (_, params) = server.await_request::(); + let methods: Vec<_> = params + .registrations + .iter() + .map(|registration| registration.method.as_str()) + .collect(); + assert_eq!( + methods, + ["textDocument/diagnostic", "workspace/didChangeWatchedFiles"] + ); + + Ok(()) +} diff --git a/crates/ty_server/tests/e2e/main.rs b/crates/ty_server/tests/e2e/main.rs index 59207ca930..a4cd5c184c 100644 --- a/crates/ty_server/tests/e2e/main.rs +++ b/crates/ty_server/tests/e2e/main.rs @@ -36,6 +36,7 @@ mod completions; mod configuration; mod data_flow; mod django_templates; +mod file_watching; mod folding_range; mod goto_definition; mod hover; @@ -43,6 +44,7 @@ mod implementation; mod initialize; mod injections; mod inlay_hints; +mod named_text; mod notebook; mod program_model; mod project_server; @@ -233,6 +235,7 @@ impl TestServer { initialization_options: Option, env_vars: Vec<(String, Option)>, project_server_directory: Option, + watch_file_system: bool, ) -> Self { setup_tracing(); @@ -265,6 +268,7 @@ impl TestServer { test_system, true, project_server_directory, + watch_file_system, ) { Ok(server) => { if let Err(err) = server.run() { @@ -423,6 +427,20 @@ impl TestServer { id } + /// Send a request by method name alone, with params written as json, for a test that asks + /// the same thing of several methods. + pub(crate) fn send_request_named( + &mut self, + method: &str, + params: serde_json::Value, + ) -> RequestId { + let id = self.next_request_id(); + tracing::debug!("Client sends request `{method}` with ID {id}"); + let request = lsp_server::Request::new(id.clone(), method.to_string(), params); + self.send(Message::Request(request)); + id + } + /// Send a notification to the server. pub(crate) fn send_notification(&mut self, params: N::Params) where @@ -1293,6 +1311,7 @@ pub(crate) struct TestServerBuilder { client_capabilities: ClientCapabilities, env_vars: Vec<(String, Option)>, project_server_directory: Option, + watch_file_system: bool, } impl TestServerBuilder { @@ -1338,6 +1357,7 @@ impl TestServerBuilder { .map(|name| ((*name).to_string(), None)) .collect(), project_server_directory: None, + watch_file_system: false, }) } @@ -1501,13 +1521,19 @@ impl TestServerBuilder { } /// Enable or disable file watching capability - #[expect(dead_code)] + /// + /// Enabled means the client offers to watch files for the server, which it does through + /// dynamic registration: a client that watches but cannot be told what to watch is, to the + /// server, a client that does not watch. pub(crate) fn enable_did_change_watched_files(mut self, enabled: bool) -> Self { self.client_capabilities .workspace .get_or_insert_default() .did_change_watched_files = if enabled { - Some(DidChangeWatchedFilesClientCapabilities::default()) + Some(DidChangeWatchedFilesClientCapabilities { + dynamic_registration: Some(true), + ..DidChangeWatchedFilesClientCapabilities::default() + }) } else { None }; @@ -1656,6 +1682,15 @@ impl TestServerBuilder { self } + /// Start the server watching the file system itself, as `by server` does. + /// + /// Off by default: a server that watches the test's directory is told about every file the + /// test writes, and answers each with refresh requests that a test would then have to claim. + pub(crate) fn watch_file_system(mut self) -> Self { + self.watch_file_system = true; + self + } + /// Build the test server pub(crate) fn build(self) -> TestServer { TestServer::new( @@ -1665,6 +1700,7 @@ impl TestServerBuilder { self.initialization_options, self.env_vars, self.project_server_directory, + self.watch_file_system, ) } } diff --git a/crates/ty_server/tests/e2e/named_text.rs b/crates/ty_server/tests/e2e/named_text.rs new file mode 100644 index 0000000000..c84b5daf27 --- /dev/null +++ b/crates/ty_server/tests/e2e/named_text.rs @@ -0,0 +1,377 @@ +//! A document request that names the text it is about, over the wire. +//! +//! A client that keeps an answer against its own revision of a document needs the answer to be +//! about that revision. It says which text it means with `textHash`, and the server answers about +//! that text: at once when it holds it — the open buffer, or the file on disk for a document the +//! client has not opened — and otherwise once it does. + +use std::time::Duration; + +use anyhow::Result; +use lsp_server::ErrorCode; +use lsp_types::{ + LspRequestMethod, MessageDirection, Request, TextDocumentContentChangeEvent, + TextDocumentContentChangeWholeDocument, +}; +use ruff_db::system::SystemPath; +use ty_server::ClientOptions; + +use crate::notebook::NotebookBuilder; +use crate::{AwaitResponseError, TestServer, TestServerBuilder}; + +/// `by/syntaxOutline`, in json throughout, as a client sends it. +enum SyntaxOutline {} + +impl Request for SyntaxOutline { + type Params = serde_json::Value; + type Result = Option; + const METHOD: LspRequestMethod<'static> = LspRequestMethod::Custom("by/syntaxOutline"); + const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer; +} + +/// `textDocument/semanticTokens/full`, in json, so that the named text can ride along. +enum SemanticTokens {} + +impl Request for SemanticTokens { + type Params = serde_json::Value; + type Result = Option; + const METHOD: LspRequestMethod<'static> = + LspRequestMethod::Custom("textDocument/semanticTokens/full"); + const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer; +} + +/// `textDocument/hover`, in json: a request whose handler answers only an open document. +enum Hover {} + +impl Request for Hover { + type Params = serde_json::Value; + type Result = Option; + const METHOD: LspRequestMethod<'static> = LspRequestMethod::Custom("textDocument/hover"); + const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer; +} + +/// The text hash as a client computes it, written out again here rather than borrowed from the +/// server, so that the two are held to the definition rather than to each other: FNV-1a, 64 +/// bits, over UTF-16 code units, every line ending counted as one `\n`. +fn text_hash(text: &str) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + let normalised = text.replace("\r\n", "\n").replace('\r', "\n"); + for unit in normalised.encode_utf16() { + for byte in unit.to_le_bytes() { + hash ^= u64::from(byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + } + format!("{hash:016x}") +} + +const MAIN: &str = "src/main.by"; + +/// On disk: one statement. +const SAVED: &str = "x = 1\n"; + +/// In the client's editor, not yet sent: a line above it, so every position moves. +const EDITED: &str = "y = \"s\"\nx = 1\n"; + +fn server() -> Result { + server_with(SAVED) +} + +/// A server whose `MAIN` holds `saved` on disk, and which the client has not opened. +fn server_with(saved: &str) -> Result { + Ok(TestServerBuilder::new()? + .with_initialization_options(&ClientOptions::default()) + .with_workspace(SystemPath::new("src"), None)? + .with_file(SystemPath::new(MAIN), saved)? + .build() + .wait_until_workspaces_are_initialized()) +} + +fn outline_params(server: &TestServer, hash: Option<&str>) -> serde_json::Value { + let mut params = serde_json::json!({ + "textDocument": { "uri": server.file_uri(MAIN) }, + }); + if let Some(hash) = hash { + params["textHash"] = serde_json::json!(hash); + } + params +} + +/// The line each top-level statement starts on. +fn statement_lines(outline: &serde_json::Value) -> Vec { + outline["statements"] + .as_array() + .expect("statements") + .iter() + .map(|statement| { + statement["range"]["start"]["line"] + .as_u64() + .expect("a line") + }) + .collect() +} + +/// A short wait, for a response that should not come. +const NOT_YET: Option = Some(Duration::from_millis(500)); + +#[test] +fn a_closed_document_is_answered_about_the_file_when_that_is_the_text_named() -> Result<()> { + let mut server = server()?; + let params = outline_params(&server, Some(&text_hash(SAVED))); + let outline = server + .send_request_await::(params) + .expect("an outline of the file on disk"); + assert_eq!(statement_lines(&outline), vec![0]); + Ok(()) +} + +#[test] +fn a_file_with_windows_line_endings_is_the_text_an_editor_holds_with_unix_ones() -> Result<()> { + let mut server = server_with("y = 1\r\nx = 1\r\n")?; + let params = outline_params(&server, Some(&text_hash("y = 1\nx = 1\n"))); + let outline = server + .send_request_await::(params) + .expect("the same positions, so the same text"); + assert_eq!(statement_lines(&outline), vec![0, 1]); + Ok(()) +} + +#[test] +fn a_closed_document_is_still_refused_when_no_text_is_named() -> Result<()> { + let mut server = server()?; + let id = server.send_request::(outline_params(&server, None)); + match server.try_await_response::(&id, None) { + Err(AwaitResponseError::RequestFailed(error)) => { + assert_eq!(error.code, ErrorCode::InvalidParams as i32); + assert!(error.message.contains("is not open"), "{}", error.message); + } + other => panic!("expected the refusal a closed document always had, got {other:?}"), + } + Ok(()) +} + +/// The case the named text exists for: an edit to a file no editor shows, asked about before the +/// client's `didOpen` for it has arrived. Answering from disk would be answering about the wrong +/// text — every line one out — and a client keeping that against its revision would keep it. +#[test] +fn a_request_for_unsent_text_waits_for_the_did_open_that_brings_it() -> Result<()> { + let mut server = server()?; + let id = + server.send_request::(outline_params(&server, Some(&text_hash(EDITED)))); + assert!( + matches!( + server.try_await_response::(&id, NOT_YET), + Err(AwaitResponseError::Timeout) + ), + "the file on disk is not the text asked about" + ); + + server.open_text_document(MAIN, EDITED, 0); + let outline = server + .await_response::(&id) + .expect("an outline of the text asked about"); + assert_eq!(statement_lines(&outline), vec![0, 1]); + Ok(()) +} + +#[test] +fn a_request_for_an_edit_waits_for_the_did_change_that_brings_it() -> Result<()> { + let mut server = server()?; + server.open_text_document(MAIN, SAVED, 0); + let id = + server.send_request::(outline_params(&server, Some(&text_hash(EDITED)))); + assert!(matches!( + server.try_await_response::(&id, NOT_YET), + Err(AwaitResponseError::Timeout) + )); + + server.change_text_document( + MAIN, + vec![ + TextDocumentContentChangeEvent::TextDocumentContentChangeWholeDocument( + TextDocumentContentChangeWholeDocument { + text: EDITED.to_string(), + }, + ), + ], + 1, + ); + let outline = server + .await_response::(&id) + .expect("an outline of the edit"); + assert_eq!(statement_lines(&outline), vec![0, 1]); + Ok(()) +} + +/// A request for a text that has come and gone is no better answered than one for a text that +/// never came: the client moved on, and cancels. +#[test] +fn a_held_request_the_client_cancels_is_answered_once_as_cancelled() -> Result<()> { + let mut server = server()?; + let id = + server.send_request::(outline_params(&server, Some(&text_hash(EDITED)))); + assert!(matches!( + server.try_await_response::(&id, NOT_YET), + Err(AwaitResponseError::Timeout) + )); + server.cancel(&id); + match server.try_await_response::(&id, None) { + Err(AwaitResponseError::RequestFailed(error)) => { + assert_eq!(error.code, ErrorCode::RequestCanceled as i32); + } + other => panic!("expected the cancellation, got {other:?}"), + } + + // the text arriving later does not bring a second answer + server.open_text_document(MAIN, EDITED, 0); + assert!(matches!( + server.try_await_response::(&id, NOT_YET), + Err(AwaitResponseError::Timeout) + )); + Ok(()) +} + +#[test] +fn a_standard_request_names_its_text_the_same_way() -> Result<()> { + let mut server = server()?; + let params = serde_json::json!({ + "textDocument": { "uri": server.file_uri(MAIN) }, + "textHash": text_hash(SAVED), + }); + let tokens = server + .send_request_await::(params) + .expect("semantic tokens for the file on disk"); + assert!( + !tokens["data"].as_array().expect("token data").is_empty(), + "{tokens:#?}" + ); + Ok(()) +} + +/// A notebook cell is a document of its own, so the text a request about a cell names is the +/// cell's, not the notebook's cells joined together. +#[test] +fn a_request_about_a_cell_names_the_cell_text() -> Result<()> { + let mut server = TestServerBuilder::new()? + .enable_pull_diagnostics(true) + .build() + .wait_until_workspaces_are_initialized(); + let mut notebook = NotebookBuilder::virtual_file("test.ipynb"); + notebook.add_python_cell("x = 1\n"); + let cell = notebook.add_python_cell("y = x\n"); + notebook.open(&mut server); + server.collect_publish_diagnostic_notifications(2); + + let id = server.send_request::(serde_json::json!({ + "textDocument": { "uri": cell }, + "textHash": text_hash("y = x\n"), + })); + let tokens = server + .try_await_response::(&id, Some(Duration::from_secs(5))) + .unwrap_or_else(|error| panic!("the cell was not answered: {error}")) + .expect("semantic tokens for the cell"); + assert!( + !tokens["data"].as_array().expect("token data").is_empty(), + "{tokens:#?}" + ); + Ok(()) +} + +#[test] +fn each_method_that_reads_a_closed_document_answers_one() -> Result<()> { + let mut server = server()?; + for method in [ + "by/syntaxOutline", + "by/injections", + "textDocument/semanticTokens/full", + "textDocument/documentSymbol", + ] { + let params = serde_json::json!({ + "textDocument": { "uri": server.file_uri(MAIN) }, + "textHash": text_hash(SAVED), + }); + let id = server.send_request_named(method, params); + let answer = server.await_response::(&id); + assert!(answer.is_some(), "{method} answered nothing"); + } + Ok(()) +} + +/// The file on disk moving ahead of the server: the client has read the new text and asks about +/// it before the server's own watcher has reported the write. The request waits for the watcher +/// rather than being answered about the text the server still has. +#[test] +fn a_request_for_a_written_file_waits_for_the_server_to_see_the_write() -> Result<()> { + let mut server = TestServerBuilder::new()? + .with_initialization_options(&ClientOptions::default()) + .with_workspace(SystemPath::new("src"), None)? + .with_file(SystemPath::new(MAIN), SAVED)? + .watch_file_system() + .build() + .wait_until_workspaces_are_initialized(); + + // the server has read the file as it was + let before = server + .send_request_await::(outline_params(&server, Some(&text_hash(SAVED)))) + .expect("an outline of the file as it was"); + assert_eq!(statement_lines(&before), vec![0]); + + let id = + server.send_request::(outline_params(&server, Some(&text_hash(EDITED)))); + server.write_file(MAIN, EDITED)?; + let after = server + .await_response::(&id) + .expect("an outline of the file as written"); + assert_eq!(statement_lines(&after), vec![0, 1]); + Ok(()) +} + +/// A handler that reads only open documents does not start reading the file on disk because the +/// client named its text; the request waits for the client to open it. +#[test] +fn a_handler_that_answers_only_open_documents_waits_for_the_open() -> Result<()> { + let mut server = server()?; + let params = serde_json::json!({ + "textDocument": { "uri": server.file_uri(MAIN) }, + "position": { "line": 0, "character": 0 }, + "textHash": text_hash(SAVED), + }); + let id = server.send_request::(params); + assert!(matches!( + server.try_await_response::(&id, NOT_YET), + Err(AwaitResponseError::Timeout) + )); + server.open_text_document(MAIN, SAVED, 0); + let hover = server.await_response::(&id).expect("a hover on `x`"); + assert!(hover.to_string().contains("int"), "{hover}"); + Ok(()) +} + +#[test] +fn a_text_hash_that_is_not_one_is_refused() -> Result<()> { + let mut server = server()?; + let id = server.send_request::(outline_params(&server, Some("not a hash"))); + match server.try_await_response::(&id, None) { + Err(AwaitResponseError::RequestFailed(error)) => { + assert_eq!(error.code, ErrorCode::InvalidParams as i32); + } + other => panic!("expected invalid params, got {other:?}"), + } + Ok(()) +} + +/// Nothing a client can do makes the server keep a request forever: one whose text never comes +/// is answered once it has waited as long as it may. +#[test] +fn a_request_whose_text_never_comes_is_answered_in_the_end() -> Result<()> { + let mut server = server()?; + let id = + server.send_request::(outline_params(&server, Some(&text_hash(EDITED)))); + match server.try_await_response::(&id, Some(Duration::from_secs(30))) { + Err(AwaitResponseError::RequestFailed(error)) => { + assert_eq!(error.code, ErrorCode::ServerCancelled as i32); + } + other => panic!("expected the server to give up, got {other:?}"), + } + Ok(()) +} diff --git a/crates/ty_server/tests/e2e/transpile.rs b/crates/ty_server/tests/e2e/transpile.rs index f45e9f92ec..2c5a28ff19 100644 --- a/crates/ty_server/tests/e2e/transpile.rs +++ b/crates/ty_server/tests/e2e/transpile.rs @@ -373,6 +373,111 @@ fn a_file_never_opened_that_does_not_check_refuses_the_set() -> Result<()> { Ok(()) } +/// **An edit landing while a reload is answered is not a panic.** Typing and then pressing Reload +/// sends the request first and the keystrokes' `didChange` after it, and the change arrives while +/// the transpile is still reading the database it writes to. Salsa cancels the reader by unwinding +/// it; the request is asked again against the edit, and answered about the text the editor now +/// holds rather than refused as a transpiler panic. +/// +/// The race is made certain rather than hoped for: the tree's `_by_sourcemap.py` is a pipe, so the +/// server — which reads it after taking its snapshot and before transpiling anything — waits on it +/// until the test has sent the edit. +#[cfg(unix)] +#[test] +fn an_edit_arriving_while_a_reload_is_answered_is_answered_against_the_edit() -> Result<()> { + use std::io::Write as _; + use std::time::Duration; + + use lsp_types::{TextDocumentContentChangeEvent, TextDocumentContentChangeWholeDocument}; + + let before = "def go() -> int:\n return 42\n"; + let after = "def go() -> int:\n return 43\n"; + let mut server = TestServerBuilder::new()? + .with_workspace(SystemPath::new("project"), None)? + .with_file("project/main.by", before)? + .with_file("project/other.by", "def other() -> int:\n return 6\n")? + .build() + .wait_until_workspaces_are_initialized(); + server.open_text_document(SystemPath::new("project/main.by"), before, 1); + let build = stale_build(&server, SystemPath::new("build"))?; + + let pipe = build.join("_by_sourcemap.py").into_std_path_buf(); + let map = std::fs::read_to_string(&pipe)?; + std::fs::remove_file(&pipe)?; + let made = std::process::Command::new("mkfifo").arg(&pipe).status()?; + assert!(made.success(), "mkfifo failed: {made}"); + + let id = server.send_request::(TranspileForBuildParams { + text_documents: vec![TextDocumentIdentifier { + uri: server.file_uri("project/main.by"), + }], + build_directory: build.as_std_path().to_path_buf(), + }); + server.change_text_document( + SystemPath::new("project/main.by"), + vec![ + TextDocumentContentChangeEvent::TextDocumentContentChangeWholeDocument( + TextDocumentContentChangeWholeDocument { + text: after.to_string(), + }, + ), + ], + 2, + ); + // Long enough for the server to take the change in and begin writing it, which it cannot + // finish while the request holds the revision it replaces. The server has nothing to show that + // it has begun, so this is a wait; if it is ever too short, the answer is about `before` and + // the assertion below says so rather than passing. + std::thread::sleep(Duration::from_millis(500)); + + // Opening a pipe to write waits for its reader, so once this returns the server is holding the + // pipe open. The path then becomes an ordinary file holding the same map, before the pipe is + // written — so the server's read is fed exactly once, and whatever reads the map after it, + // which is only ever an attempt made after the edit, reads the file. + let fed = { + let (pipe, staged) = ( + pipe.clone(), + build.join("_by_sourcemap.py.next").into_std_path_buf(), + ); + std::fs::write(&staged, &map)?; + let (sender, receiver) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let fed = std::fs::OpenOptions::new() + .write(true) + .open(&pipe) + .and_then(|mut writer| { + std::fs::rename(&staged, &pipe)?; + writer.write_all(map.as_bytes()) + }); + let _ = sender.send(fed); + }); + receiver.recv_timeout(Duration::from_secs(60)) + }; + let Ok(fed) = fed else { + // let the feeder go, which is waiting for a reader that never came + let _ = std::fs::read(&pipe); + panic!("the server never read the tree's map"); + }; + fed?; + + let answer = server + .try_await_response::(&id, Some(Duration::from_secs(60))) + .unwrap_or_else(|error| panic!("the reload was not answered: {error}")); + let files = answer["files"] + .as_array() + .unwrap_or_else(|| panic!("the reload was refused: {answer}")); + let content = files[0]["content"].as_str().unwrap(); + // The request's first attempt holds the revision from before the edit, and nothing can show it + // the edit — the change's write waits for that attempt to let go. So an answer about the edit is + // an answer from a second attempt, made against the new revision. + assert!( + content.contains("return 43"), + "the answer is about the text before the edit, so the edit never reached the \ + request while it was running:\n{content}" + ); + Ok(()) +} + /// A tree nothing says is a build is refused for the whole set, not file by file. #[test] fn a_directory_that_is_not_a_build_refuses_the_set() -> Result<()> { diff --git a/docs/basedpython/features/editor.md b/docs/basedpython/features/editor.md index 34354728cd..fa5c16de26 100644 --- a/docs/basedpython/features/editor.md +++ b/docs/basedpython/features/editor.md @@ -406,6 +406,23 @@ much indentation a triple-quoted string has stripped and `*_test.by` — because a `.by` transpiles to the `.py` of the same stem, and that is the file pytest sees +### asking about the text you have + +a client that keeps an answer against its own revision of a document can say +which text a document request is about, by adding `textHash` to the request's +params: FNV-1a, 64 bits, over the text's UTF-16 code units with every line ending +counted as one `\n`, written as sixteen lowercase hex digits. for a notebook cell +that is the cell's own text. the request is then answered about that text and no +other. when the server already holds it — the +open buffer, or for `by/syntaxOutline`, `by/injections`, +`textDocument/semanticTokens/full` and `textDocument/documentSymbol` the file on +disk of a document the client has not opened — it answers at once; otherwise the +request waits for the `didOpen`, `didChange` or file write that brings the text, +and is answered with `ServerCancelled` if nothing has after ten seconds. so a +client need not know whether its `didOpen` has gone out before it asks, and an +answer never describes text it did not ask about. a request without `textHash` is +answered as before, and refused for a document that is not open + ## debugger facts while a program is stopped, an editor knows something no checker does: what the diff --git a/docs/basedpython/features/index.md b/docs/basedpython/features/index.md index 887aca9fd0..13742d5252 100644 --- a/docs/basedpython/features/index.md +++ b/docs/basedpython/features/index.md @@ -135,6 +135,7 @@ type parameters — their bounds, their variance, and what survives to runtime - [safe variance](safe-variance.md) - [overlapping](overlapping.md) - [explicit generic call sites](generic-calls.md) +- [defaults that initialise type parameters](parameter-defaults.md) - [reified type parameters](reified-generics.md) - [reified class type parameters](reified-class-generics.md) - [type reification](type-reification.md) diff --git a/docs/basedpython/features/parameter-defaults.md b/docs/basedpython/features/parameter-defaults.md new file mode 100644 index 0000000000..0d0c353e52 --- /dev/null +++ b/docs/basedpython/features/parameter-defaults.md @@ -0,0 +1,175 @@ +# defaults that initialise type parameters + +a parameter's default can decide a type parameter: when a call leaves the argument out, the type +parameter is solved from the default, the same as if the default had been passed + +```by +def f[T](t: T = 1) -> T: + return t + +one = f() # `1` +a = f("a") # `"a"` +``` + +the default only has to fit *some* specialization of the annotation, not every one. `1` is not a +valid `T` for every `T`, but it is for the `T` that a call leaving `t` out solves + +this is purely a checker rule: the program transpiles and runs unchanged + +## the default is solved with the other arguments + +a default the call falls back on is one more value for the type parameter, alongside the arguments +that were passed + +```by +def pair[T](first: T, second: T = 1) -> T: + return first + +either = pair("a") # `"a" | 1` +``` + +when an argument pins the type parameter to something the default doesn't fit, the call is an +`invalid-argument-type` error, with a note naming the default that took part + +```by +def first[T](items: list[T], fallback: T = 0) -> T: + return items[0] if items else fallback + +def _(names: list[str]): + name = first(names, "") # fine + name = first(names) # error: `list[T]` makes `T` a `str`, and `0` isn't one +``` + +## unpacked arguments + +an unpacked argument that may be too short to reach the parameter may or may not supply it, so the +default takes part alongside what it unpacks + +```by +def f[T](t: T = 1) -> T: + return t + +def _(maybe: tuple[()] | tuple[str]): + x = f(*maybe) # `str | 1`: `f(*())` is `f()` +``` + +## class type parameters + +a constructor's defaults initialise the class's type parameters + +```by +class Box[T]: + def __init__(self, item: T = 1): + self.item = item + + def replace(self, item: T = 1) -> T: + return item + +numbers = Box() # `Box[int]` +words = Box("a") # `Box[str]` +``` + +a method's call is given the class's type parameters by the receiver, so its default is checked +against them + +```by +def _(box: Box[str]): + word = box.replace() # error: the default `1` is not a `str` + +empty = Box[str]() # error: the default `1` is not a `str` +``` + +## `some` parameters + +a [`some`](sound-types.md) parameter opens an anonymous type parameter, and its default initialises +it like any other. another parameter annotated with that type contributes to it too + +```by +def f(n: some int = 1, m: n = 2) -> n: + return n + +n = f(m=5) # `5 | 1` +``` + +## inherited defaults + +an override that [inherits a default](inherited-defaults.md) solves it against its own annotation, +not the base's + +```by +class A: + def f(self, a: int = 1) -> int: + return a + +class B(A): + override def f[T](self, a: T) -> T: + return a + +one = B().f() # `1` +``` + +## overrides, protocols and overloads keep the default + +a caller that holds an `A` and calls `a.m()` solves `T` from the default `A.m` declares, whatever +subclass it really holds. so an override, a protocol implementation, or a function passed where a +callable protocol is expected has to run with a default that the declared one describes + +```by +class A: + def m[T](self, t: T = 1) -> T: + return t + +class B(A): + override def m[T](self, t: T = "a") -> T: # error: invalid-method-override + return t +``` + +an overload's default makes the same promise about its implementation, which is what runs. an +implementation whose default the overload's doesn't describe is an `invalid-overload` error + +## passing the function on + +a function whose default initialises a type parameter can stand in for a callable that leaves the +argument out, and a `ParamSpec` that forwards its parameters keeps the default + +```by +from collections.abc import Callable + +def g[T](t: T = 1) -> T: + return t + +def call[R](f: Callable[[], R]) -> R: + return f() + +one = call(g) # `1` +``` + +a `functools.partial` that leaves the parameter to the later call solves the default together with +the arguments it binds, since the later call may fall back on it + +## what is still an error + +a default no specialization fits is an `invalid-parameter-default` error where it is written. that +includes a default for a type parameter of an enclosing function, which no call can change, and a +default whose own type names the type parameter: a default is typed as the one value made where the +`def` runs, which no single call's type parameter describes + +```by +def bounded[T: str](t: T = 1): ... # error: `1` is never a `str` + +def outer[T](value: T): + def inner(t: T = 1): ... # error: `T` is fixed by `outer` + +def first_or[T](value: T, fallback: list[T] = [1]): ... # error: `[1]` is a `list[T | int]` +``` + +an [inherited default](inherited-defaults.md) that fits no specialization of the override's +annotation is reported on the override + +the `...` that a stub, an `@overload`, an `@abstractmethod`, a protocol member or a declaration under +`if TYPE_CHECKING:` writes in place of a default is not a value, so it solves nothing + +## limitations + +assigning such a function to a declared callable type, `c: Callable[[], str] = g`, doesn't take the +default into account yet, so that assignment is accepted diff --git a/ty.schema.json b/ty.schema.json index e3e52a2d8a..da4e523aef 100644 --- a/ty.schema.json +++ b/ty.schema.json @@ -1513,7 +1513,7 @@ }, "invalid-overload": { "title": "detects invalid `@overload` usages", - "description": "## What it does\n\nChecks for various invalid `@overload` usages.\n\n## Why is this bad?\n\nThe `@overload` decorator is used to define functions and methods that accepts different\ncombinations of arguments and return different types based on the arguments passed. This is mainly\nbeneficial for type checkers. But, if the `@overload` usage is invalid, the type checker may not be\nable to provide correct type information.\n\n## Examples\n\n### Single overload\n\n```py\nfrom typing import overload\n\n\n@overload\ndef foo(x: int) -> int: ... # error\ndef foo(x: int | None) -> int | None:\n return x\n```\n\n### Missing implementation\n\n```py\nfrom typing import overload\n\n\n@overload\ndef foo() -> None: ... # error\n@overload\ndef foo(x: int) -> int: ...\n```\n\n## References\n\n- [Python documentation: `@overload`](https://docs.python.org/3/library/typing.html#typing.overload)", + "description": "## What it does\n\nChecks for various invalid `@overload` usages.\n\n## Why is this bad?\n\nThe `@overload` decorator is used to define functions and methods that accepts different\ncombinations of arguments and return different types based on the arguments passed. This is mainly\nbeneficial for type checkers. But, if the `@overload` usage is invalid, the type checker may not be\nable to provide correct type information.\n\n## Examples\n\n### Single overload\n\n```py\nfrom typing import overload\n\n\n@overload\ndef foo(x: int) -> int: ... # error\ndef foo(x: int | None) -> int | None:\n return x\n```\n\n### Missing implementation\n\n```py\nfrom typing import overload\n\n\n@overload\ndef foo() -> None: ... # error\n@overload\ndef foo(x: int) -> int: ...\n```\n\n### Implementation default an overload's default doesn't describe\n\nA call matching an overload that leaves an argument out is solved from the overload's default, but\nthe implementation runs with its own.\n\n```py\nfrom typing import TypeVar, overload\n\nT = TypeVar(\"T\")\n\n\n@overload\ndef foo(x: T = 1) -> T: ... # error\n@overload\ndef foo(x: int, y: int) -> int: ...\ndef foo(x: object = \"a\", y: int = 0) -> object:\n return x\n```\n\n## References\n\n- [Python documentation: `@overload`](https://docs.python.org/3/library/typing.html#typing.overload)", "default": "error", "oneOf": [ { @@ -1533,7 +1533,7 @@ }, "invalid-parameter-default": { "title": "detects default values that can't be assigned to the parameter's annotated type", - "description": "## What it does\n\nChecks for default values that can't be assigned to the parameter's annotated type.\n\n## Why is this bad?\n\nThis breaks the rules of the type system and weakens a type checker's ability to accurately reason\nabout your code.\n\n## Examples\n\n```python\ndef f(a: int = \"\"): ... # error\n```", + "description": "## What it does\n\nChecks for default values that can't be assigned to the parameter's annotated type.\n\nA default only needs to fit *some* specialization of the type variables the annotation names: a call\nthat leaves the argument out is solved as if the default had been passed. The default is reported\nwhen no such specialization exists. Only the type variables of the function itself (and, for a\nmethod, of its class) can be chosen that way; one of an enclosing function is fixed for every call.\nA default whose own type names one of those type variables, such as `[1]` inferred against `list[T]`\nas `list[T | int]`, is reported too: it is a single value shared by every call, so no call can make\nit fit.\n\nIn basedpython, a default an override inherits from the method it overrides is checked against the\noverride's annotation in the same way.\n\n## Why is this bad?\n\nThis breaks the rules of the type system and weakens a type checker's ability to accurately reason\nabout your code.\n\n## Examples\n\n```python\nfrom typing import TypeVar\n\nT = TypeVar(\"T\")\nS = TypeVar(\"S\", bound=str)\n\n\ndef f(a: int = \"\"): ... # error\n\n\n# fine: `g()` solves `T` from the default\ndef g(a: T = 1) -> T:\n return a\n\n\n# no `S` bounded by `str` holds `1`\ndef h(a: S = 1) -> S: # error\n return a\n```", "default": "error", "oneOf": [ { diff --git a/zensical.toml b/zensical.toml index 423102f51e..4d52ff6ea5 100644 --- a/zensical.toml +++ b/zensical.toml @@ -151,6 +151,7 @@ features = [ { "safe variance" = "features/safe-variance.md" }, { "overlapping" = "features/overlapping.md" }, { "explicit generic call sites" = "features/generic-calls.md" }, + { "defaults that initialise type parameters" = "features/parameter-defaults.md" }, { "reified type parameters" = "features/reified-generics.md" }, { "reified class type parameters" = "features/reified-class-generics.md" }, { "type reification" = "features/type-reification.md" },