[ty] Make the default database truly statically infallible#22056
[ty] Make the default database truly statically infallible#22056
Conversation
| /// Generic handling of two possible approaches to an Error: | ||
| /// | ||
| /// * [`FailStrategy`]: The code should simply fail | ||
| /// * [`UseDefaultStrategy`]: The chode should apply default values and never fail | ||
| /// | ||
| /// Any function that wants to be made generic over these approaches should be changed thusly. | ||
| /// | ||
| /// Old: | ||
| /// | ||
| /// ```ignore | ||
| /// fn do_thing() | ||
| /// -> Result<T, E> | ||
| /// { | ||
| /// let x = something_fallible()?; | ||
| /// Ok(x) | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// New: | ||
| /// | ||
| /// ```ignore | ||
| /// fn do_thing<Strategy: MisconfigurationStrategy>(strategy: &Strategy) | ||
| /// -> Result<T, Strategy::Error<E>> | ||
| /// { | ||
| /// let x = strategy.fallback(something_fallible(), |err| { | ||
| /// tracing::debug!("Failed to get value: {err}"); | ||
| /// MyType::default() | ||
| /// })?; | ||
| /// Ok(x) | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// The key trick is instead of returning `Result<T, E>` your function should | ||
| /// return `Result<T, Strategy::Error<E>`. Which simplifies to: | ||
| /// | ||
| /// * [`FailStrategy`]: `Result<T, E>` | ||
| /// * [`UseDefaultStrategy`]: `Result<T, Never>` ~= `T` | ||
| /// | ||
| /// Notably, if your function returns `Result<T, Strategy::Error<E>>` you will | ||
| /// be *statically prevented* from returning an `Err` without going through | ||
| /// [`MisconfigurationStrategy::fallback`][] or [`MisconfigurationStrategy::fallback_opt`][] | ||
| /// which ensure you're handling both approaches (or you wrote an `unwrap` but | ||
| /// those standout far more than adding a new `?` to a function that must be able to Not Fail. | ||
| /// | ||
| /// Also, for any caller that passes in [`UseDefaultStrategy`], they will be able | ||
| /// to write `let Ok(val) = do_thing(&UseDefaultStrategy);` instead of having to | ||
| /// write an `unwrap()`. | ||
| pub trait MisconfigurationStrategy { |
There was a problem hiding this comment.
Here's the heart and soul
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
||
| let db_with_default_settings = ProjectMetadata::from_options( | ||
| let Ok(metadata) = ProjectMetadata::from_options( | ||
| Options::default(), | ||
| root, | ||
| None, | ||
| MisconfigurationMode::UseDefault, | ||
| ) | ||
| .context("Failed to convert default options to metadata") | ||
| .and_then(|metadata| ProjectDatabase::new(metadata, system)) | ||
| .expect("Default configuration to be valid"); | ||
| &UseDefaultStrategy, | ||
| ); | ||
| let Ok(db_with_default_settings) = | ||
| ProjectDatabase::new(metadata, system, &UseDefaultStrategy); | ||
| let default_root = db_with_default_settings |
There was a problem hiding this comment.
Here's the glorious payoff.
|
| impl Default for IncludeExcludeFilter { | ||
| fn default() -> Self { | ||
| let mut includes = IncludeFilterBuilder::new(); | ||
| includes | ||
| .add( | ||
| &PortableGlobPattern::parse("**", PortableGlobKind::Include) | ||
| .unwrap() | ||
| .into_absolute(""), | ||
| ) | ||
| .expect("default include filter to be infallible"); | ||
|
|
||
| let mut excludes = ExcludeFilterBuilder::new(); | ||
|
|
||
| for pattern in DEFAULT_SRC_EXCLUDES { | ||
| PortableGlobPattern::parse(pattern, PortableGlobKind::Exclude) | ||
| .and_then(|exclude| Ok(excludes.add(&exclude.into_absolute(""))?)) | ||
| .unwrap_or_else(|err| { | ||
| panic!( | ||
| "Expected default exclude to be valid glob but adding it failed with: {err}" | ||
| ) | ||
| }); | ||
| } | ||
|
|
||
| Self { | ||
| include: includes | ||
| .build() | ||
| .expect("default include filter to be infallible"), | ||
| exclude: excludes | ||
| .build() | ||
| .expect("default exclude filter to be infallible"), | ||
| } | ||
| } |
There was a problem hiding this comment.
This is as deep as I was willing to propagate fallback logic. These unwraps/panics really should never ever happen and I'm not worried about them.
| { | ||
| if let Some(requires_python) = project.resolve_requires_python_lower_bound()? { | ||
| let requires_python = strategy.fallback_opt( | ||
| project.resolve_requires_python_lower_bound(), | ||
| |err| { | ||
| tracing::debug!("skipping invalid requires_python lower bound: {err}"); | ||
| }, | ||
| )?; | ||
| if let Some(requires_python) = requires_python.flatten() { |
There was a problem hiding this comment.
I was aware of this fallible operation already, I intentionally didn't put handling on it before because the default database doesn't ever reach this branch (but hey, now we Definitely handle it).
| color: colored::control::SHOULD_COLORIZE.should_colorize(), | ||
| })?; | ||
| }); | ||
| let src = strategy.fallback(src, |_| SrcSettings::default())?; | ||
|
|
||
| let overrides = self | ||
| .to_overrides_settings(db, project_root, &mut diagnostics) | ||
| .map_err(|err| ToSettingsError { | ||
| diagnostic: err, | ||
| output_format: terminal.output_format, | ||
| color: colored::control::SHOULD_COLORIZE.should_colorize(), | ||
| })?; | ||
| }); | ||
| let overrides = strategy.fallback(overrides, |_| Vec::new())?; | ||
|
|
There was a problem hiding this comment.
These are totally new to me and potential failures I never looked into (I expect they never could fail with the default database but hey, now we know they won't).
There was a problem hiding this comment.
They can't with the default database because overrides initializes as None but having this enforced is nice
|
geofft
left a comment
There was a problem hiding this comment.
I don't think I'm qualified to actually review this but it seems entirely sensible to me!
| /// Generic handling of two possible approaches to an Error: | ||
| /// | ||
| /// * [`FailStrategy`]: The code should simply fail | ||
| /// * [`UseDefaultStrategy`]: The chode should apply default values and never fail |
There was a problem hiding this comment.
| /// * [`UseDefaultStrategy`]: The chode should apply default values and never fail | |
| /// * [`UseDefaultStrategy`]: The code should apply default values and never fail |
There was a problem hiding this comment.
What about naming these FallibleStrategy and InfallibleStrategy?
Although, I do think I like UseDefaultStrategy more than InfallibleStrategy. I think it's more specific.
MichaReiser
left a comment
There was a problem hiding this comment.
This is great. It might make sense for @BurntSushi to also take a look.
What would be nice is if we could hide this implementation detail from the ProjectDatabase::new callers, e.g. by exposing two methods instead of having them pick a strategy (where the safe could also do the unwrapping)
| }); | ||
|
|
||
| let mut db = ProjectDatabase::new(metadata, system).unwrap(); | ||
| let mut db = ProjectDatabase::new(metadata, system, &FailStrategy).unwrap(); |
There was a problem hiding this comment.
Would it make sense to instead expose two methods like ProjectDatabase::new (falible by default) and ProjectDatabase::safe? Ideally, the unwrap wouldn't be needed here because we, obviously, want' the safe strategy here
There was a problem hiding this comment.
Hm I think the tests/benches do actually want this unwrap() because we want to immediately slam the breaks if they're misconfigured or if someone has broken a common setup. Unless we're trying to emulate the default database experience?
There was a problem hiding this comment.
If we went this route, I'd go with ProjectDatabase::fallible and ProjectDatabase::automatic_defaults or something like that. I'd avoid safe personally, given its confusability with Rust notions of safety.
| color: colored::control::SHOULD_COLORIZE.should_colorize(), | ||
| })?; | ||
| }); | ||
| let src = strategy.fallback(src, |_| SrcSettings::default())?; | ||
|
|
||
| let overrides = self | ||
| .to_overrides_settings(db, project_root, &mut diagnostics) | ||
| .map_err(|err| ToSettingsError { | ||
| diagnostic: err, | ||
| output_format: terminal.output_format, | ||
| color: colored::control::SHOULD_COLORIZE.should_colorize(), | ||
| })?; | ||
| }); | ||
| let overrides = strategy.fallback(overrides, |_| Vec::new())?; | ||
|
|
There was a problem hiding this comment.
They can't with the default database because overrides initializes as None but having this enforced is nice
| /// | ||
| /// In Rust if you have Result<T, Never> the compiler knows `Err` is impossible | ||
| /// and you can just write `let Ok(val) = result;` | ||
| pub enum Never {} |
There was a problem hiding this comment.
Could we make use of this in many places where we use .unwrap today?
There was a problem hiding this comment.
I think you can just use std::convert::Infallible here instead. (I don't know if it really matters. Probably not. But seems nicer to use the same type as std.)
| /// Also, for any caller that passes in [`UseDefaultStrategy`], they will be able | ||
| /// to write `let Ok(val) = do_thing(&UseDefaultStrategy);` instead of having to | ||
| /// write an `unwrap()`. | ||
| pub trait MisconfigurationStrategy { |
There was a problem hiding this comment.
Is this our own Try trait :D
There was a problem hiding this comment.
Since this is mostly just an internal trait (and probably spiritually sealed at least), I might add a std::fmt::Debug constraint here. That way folks can dbg! it in random code.
| /// A [`MisconfigurationStrategy`] that happily fails whenever | ||
| /// an important `Err` is encountered. | ||
| #[derive(Default, Copy, Clone, Debug, PartialEq, Eq)] | ||
| pub struct FailStrategy; |
There was a problem hiding this comment.
Maybe ErrStrategy or ErrorStrategy?
BurntSushi
left a comment
There was a problem hiding this comment.
This seems okay to me. I feel like the GAT is mostly bundled up pretty tight. And the docs on the trait helped me understand the approach. :-)
| /// | ||
| /// In Rust if you have Result<T, Never> the compiler knows `Err` is impossible | ||
| /// and you can just write `let Ok(val) = result;` | ||
| pub enum Never {} |
There was a problem hiding this comment.
I think you can just use std::convert::Infallible here instead. (I don't know if it really matters. Probably not. But seems nicer to use the same type as std.)
| /// Generic handling of two possible approaches to an Error: | ||
| /// | ||
| /// * [`FailStrategy`]: The code should simply fail | ||
| /// * [`UseDefaultStrategy`]: The chode should apply default values and never fail |
There was a problem hiding this comment.
What about naming these FallibleStrategy and InfallibleStrategy?
Although, I do think I like UseDefaultStrategy more than InfallibleStrategy. I think it's more specific.
| /// | ||
| /// Also, for any caller that passes in [`UseDefaultStrategy`], they will be able | ||
| /// to write `let Ok(val) = do_thing(&UseDefaultStrategy);` instead of having to | ||
| /// write an `unwrap()`. |
There was a problem hiding this comment.
Great docs. This really helped me understand the approach here.
I might add here that we previously did this at runtime with a simple enum, but that we kept playing whack-a-mole since every call site needed to check the enum. Hence the justification for more sophisticated type wizardry to statically prevent it to force all callers to deal with the possibility of an automatic fallback.
| pub fn new<S, Strategy: MisconfigurationStrategy>( | ||
| project_metadata: ProjectMetadata, | ||
| system: S, | ||
| strategy: &Strategy, |
There was a problem hiding this comment.
Out of curiosity, how come you ended up using an &Strategy here instead of just a Strategy?
If I were to try and answer my own question, I think it's because this assumes less about MisconfigurationStrategy. I think in order to make just Strategy work, you'd need to add a Copy bound to MisconfigurationStrategy?
I guess it doesn't really matter.
There was a problem hiding this comment.
Yep I just didn't want to add the Copy bound, didn't feel it mattered much.
| /// Also, for any caller that passes in [`UseDefaultStrategy`], they will be able | ||
| /// to write `let Ok(val) = do_thing(&UseDefaultStrategy);` instead of having to | ||
| /// write an `unwrap()`. | ||
| pub trait MisconfigurationStrategy { |
There was a problem hiding this comment.
Since this is mostly just an internal trait (and probably spiritually sealed at least), I might add a std::fmt::Debug constraint here. That way folks can dbg! it in random code.
| }); | ||
|
|
||
| let mut db = ProjectDatabase::new(metadata, system).unwrap(); | ||
| let mut db = ProjectDatabase::new(metadata, system, &FailStrategy).unwrap(); |
There was a problem hiding this comment.
If we went this route, I'd go with ProjectDatabase::fallible and ProjectDatabase::automatic_defaults or something like that. I'd avoid safe personally, given its confusability with Rust notions of safety.
|
This has multiple approving reviews but never landed and has now accumulated lots of conflicts. Do we still want to do this? Assigning to @MichaReiser to make a call on what to do here (non-urgent since it's already quite old.) |
|
I like the changes but don't think it's worth a lot of my time right now. I'll put Claude on it to see if I can get this into a landable state. |
|
I rebased the changes, submitted and merged a new PR |
Summary
I usually hate type wizardry but I decided to have some fun (and also I am legitimately worried that we're going to end up in
Errwhackamole and the previous approach was a huge maintenance hazard).