mirror of
https://github.com/charliermarsh/ruff
synced 2025-10-05 23:52:47 +02:00
## Summary We use the `System` abstraction in ty to abstract away the host/system on which ty runs. This has a few benefits: * Tests can run in full isolation using a memory system (that uses an in-memory file system) * The LSP has a custom implementation where `read_to_string` returns the content as seen by the editor (e.g. unsaved changes) instead of always returning the content as it is stored on disk * We don't require any file system polyfills for wasm in the browser However, it does require extra care that we don't accidentally use `std::fs` or `std::env` (etc.) methods in ty's code base (which is very easy). This PR sets up Clippy and disallows the most common methods, instead pointing users towards the corresponding `System` methods. The setup is a bit awkward because clippy doesn't support inheriting configurations. That means, a crate can only override the entire workspace configuration or not at all. The approach taken in this PR is: * Configure the disallowed methods at the workspace level * Allow `disallowed_methods` at the workspace level * Enable the lint at the crate level using the warn attribute (in code) The obvious downside is that it won't work if we ever want to disallow other methods, but we can figure that out once we reach that point. What about false positives: Just add an `allow` and move on with your life :) This isn't something that we have to enforce strictly; the goal is to catch accidental misuse. ## Test Plan Clippy found a place where we incorrectly used `std::fs::read_to_string`
231 lines
7.0 KiB
Rust
231 lines
7.0 KiB
Rust
#![warn(
|
|
clippy::disallowed_methods,
|
|
reason = "Prefer System trait methods over std methods in ty crates"
|
|
)]
|
|
|
|
use std::{collections::HashMap, hash::BuildHasher};
|
|
|
|
use ordermap::OrderMap;
|
|
use ruff_db::system::SystemPathBuf;
|
|
use ruff_python_ast::PythonVersion;
|
|
use ty_python_semantic::PythonPlatform;
|
|
|
|
/// Combine two values, preferring the values in `self`.
|
|
///
|
|
/// The logic should follow that of Cargo's `config.toml`:
|
|
///
|
|
/// > If a key is specified in multiple config files, the values will get merged together.
|
|
/// > Numbers, strings, and booleans will use the value in the deeper config directory taking
|
|
/// > precedence over ancestor directories, where the home directory is the lowest priority.
|
|
/// > Arrays will be joined together with higher precedence items being placed later in the
|
|
/// > merged array.
|
|
///
|
|
/// ## uv Compatibility
|
|
///
|
|
/// The merging behavior differs from uv in that values with higher precedence in arrays
|
|
/// are placed later in the merged array. This is because we want to support overriding
|
|
/// earlier values and values from other configurations, including unsetting them.
|
|
/// For example: patterns coming last in file inclusion and exclusion patterns
|
|
/// allow overriding earlier patterns, matching the `gitignore` behavior.
|
|
/// Generally speaking, it feels more intuitive if later values override earlier values
|
|
/// than the other way around: `ty --exclude png --exclude "!important.png"`.
|
|
///
|
|
/// The main downside of this approach is that the ordering can be surprising in cases
|
|
/// where the option has a "first match" semantic and not a "last match" wins.
|
|
/// One such example is `extra-paths` where the semantics is given by Python:
|
|
/// the module on the first matching search path wins.
|
|
///
|
|
/// ```toml
|
|
/// [environment]
|
|
/// extra-paths = ["b", "c"]
|
|
/// ```
|
|
///
|
|
/// ```bash
|
|
/// ty --extra-paths a
|
|
/// ```
|
|
///
|
|
/// That's why a user might expect that this configuration results in `["a", "b", "c"]`,
|
|
/// because the CLI has higher precedence. However, the current implementation results in a
|
|
/// resolved extra search path of `["b", "c", "a"]`, which means `a` will be tried last.
|
|
///
|
|
/// There's an argument here that the user should be able to specify the order of the paths,
|
|
/// because only then is the user in full control of where to insert the path when specifying `extra-paths`
|
|
/// in multiple sources.
|
|
///
|
|
/// ## Macro
|
|
/// You can automatically derive `Combine` for structs with named fields by using `derive(ruff_macros::Combine)`.
|
|
pub trait Combine {
|
|
#[must_use]
|
|
fn combine(mut self, other: Self) -> Self
|
|
where
|
|
Self: Sized,
|
|
{
|
|
self.combine_with(other);
|
|
self
|
|
}
|
|
|
|
fn combine_with(&mut self, other: Self);
|
|
}
|
|
|
|
impl<T> Combine for Option<T>
|
|
where
|
|
T: Combine,
|
|
{
|
|
fn combine(self, other: Self) -> Self
|
|
where
|
|
Self: Sized,
|
|
{
|
|
match (self, other) {
|
|
(Some(a), Some(b)) => Some(a.combine(b)),
|
|
(None, Some(b)) => Some(b),
|
|
(a, _) => a,
|
|
}
|
|
}
|
|
|
|
fn combine_with(&mut self, other: Self) {
|
|
match (self, other) {
|
|
(Some(a), Some(b)) => {
|
|
a.combine_with(b);
|
|
}
|
|
(a @ None, Some(b)) => {
|
|
*a = Some(b);
|
|
}
|
|
_ => {}
|
|
}
|
|
}
|
|
}
|
|
|
|
impl<T> Combine for Vec<T> {
|
|
fn combine_with(&mut self, mut other: Self) {
|
|
// `self` takes precedence over `other` but values with higher precedence must be placed after.
|
|
// Swap the vectors so that `other` is the one that gets extended, so that the values of `self` come after.
|
|
std::mem::swap(self, &mut other);
|
|
self.extend(other);
|
|
}
|
|
}
|
|
|
|
impl<K, V, S> Combine for HashMap<K, V, S>
|
|
where
|
|
K: Eq + std::hash::Hash,
|
|
S: BuildHasher,
|
|
{
|
|
fn combine_with(&mut self, mut other: Self) {
|
|
// `self` takes precedence over `other` but `extend` overrides existing values.
|
|
// Swap the hash maps so that `self` is the one that gets extended.
|
|
std::mem::swap(self, &mut other);
|
|
self.extend(other);
|
|
}
|
|
}
|
|
|
|
impl<K, V, S> Combine for OrderMap<K, V, S>
|
|
where
|
|
K: Eq + std::hash::Hash,
|
|
S: BuildHasher,
|
|
{
|
|
fn combine_with(&mut self, other: Self) {
|
|
for (k, v) in other {
|
|
self.entry(k).or_insert(v);
|
|
}
|
|
}
|
|
}
|
|
|
|
/// Implements [`Combine`] for a value that always returns `self` when combined with another value.
|
|
macro_rules! impl_noop_combine {
|
|
($name:ident) => {
|
|
impl Combine for $name {
|
|
#[inline(always)]
|
|
fn combine_with(&mut self, _other: Self) {}
|
|
|
|
#[inline(always)]
|
|
fn combine(self, _other: Self) -> Self {
|
|
self
|
|
}
|
|
}
|
|
};
|
|
}
|
|
|
|
impl_noop_combine!(SystemPathBuf);
|
|
impl_noop_combine!(PythonPlatform);
|
|
impl_noop_combine!(PythonVersion);
|
|
|
|
// std types
|
|
impl_noop_combine!(bool);
|
|
impl_noop_combine!(usize);
|
|
impl_noop_combine!(u8);
|
|
impl_noop_combine!(u16);
|
|
impl_noop_combine!(u32);
|
|
impl_noop_combine!(u64);
|
|
impl_noop_combine!(u128);
|
|
impl_noop_combine!(isize);
|
|
impl_noop_combine!(i8);
|
|
impl_noop_combine!(i16);
|
|
impl_noop_combine!(i32);
|
|
impl_noop_combine!(i64);
|
|
impl_noop_combine!(i128);
|
|
impl_noop_combine!(String);
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use ordermap::OrderMap;
|
|
use std::collections::HashMap;
|
|
|
|
use super::Combine;
|
|
|
|
#[test]
|
|
fn combine_option() {
|
|
assert_eq!(Some(1).combine(Some(2)), Some(1));
|
|
assert_eq!(None.combine(Some(2)), Some(2));
|
|
assert_eq!(Some(1).combine(None), Some(1));
|
|
}
|
|
|
|
#[test]
|
|
fn combine_vec() {
|
|
assert_eq!(None.combine(Some(vec![1, 2, 3])), Some(vec![1, 2, 3]));
|
|
assert_eq!(Some(vec![1, 2, 3]).combine(None), Some(vec![1, 2, 3]));
|
|
assert_eq!(
|
|
Some(vec![1, 2, 3]).combine(Some(vec![4, 5, 6])),
|
|
Some(vec![4, 5, 6, 1, 2, 3])
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn combine_map() {
|
|
let a: HashMap<u32, _> = HashMap::from_iter([(1, "a"), (2, "a"), (3, "a")]);
|
|
let b: HashMap<u32, _> = HashMap::from_iter([(0, "b"), (2, "b"), (5, "b")]);
|
|
|
|
assert_eq!(None.combine(Some(b.clone())), Some(b.clone()));
|
|
assert_eq!(Some(a.clone()).combine(None), Some(a.clone()));
|
|
assert_eq!(
|
|
Some(a).combine(Some(b)),
|
|
Some(HashMap::from_iter([
|
|
(0, "b"),
|
|
// The value from `a` takes precedence
|
|
(1, "a"),
|
|
(2, "a"),
|
|
(3, "a"),
|
|
(5, "b")
|
|
]))
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn combine_order_map() {
|
|
let a: OrderMap<u32, _> = OrderMap::from_iter([(1, "a"), (2, "a"), (3, "a")]);
|
|
let b: OrderMap<u32, _> = OrderMap::from_iter([(0, "b"), (2, "b"), (5, "b")]);
|
|
|
|
assert_eq!(None.combine(Some(b.clone())), Some(b.clone()));
|
|
assert_eq!(Some(a.clone()).combine(None), Some(a.clone()));
|
|
assert_eq!(
|
|
Some(a).combine(Some(b)),
|
|
// The value from `a` takes precedence
|
|
Some(OrderMap::from_iter([
|
|
(1, "a"),
|
|
(2, "a"),
|
|
(3, "a"),
|
|
(0, "b"),
|
|
(5, "b")
|
|
]))
|
|
);
|
|
}
|
|
}
|