Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 24, 2025

Summary

An increasingly common pattern in our codebase is to do something like this, where we iterate over a sequence of types and apply a fallible mapping operation to each element in turn:

                let mut builder = UnionBuilder::new(db);
                for element in union.elements(db) {
                    builder = builder.add(element.to_instance(db)?);
                }
                Some(builder.build())

While the mapping function continues to return Some(Type<'db>) for each element, we build up a union; but as soon as the mapping operation returns None for any element, we short-circuit and return None.

This PR adds some helper functions to UnionType to make this pattern less verbose: UnionType::try_from_elements() and UnionType::try_map.

Unfortunately there are a couple of places where a mapping function returns a Result:

(Type::Union(union), other) => {
let mut builder = UnionBuilder::new(self.db());
for element in union.elements(self.db()) {
builder =
builder.add(self.infer_binary_type_comparison(*element, op, other, range)?);
}
Ok(builder.build())
}
(other, Type::Union(union)) => {
let mut builder = UnionBuilder::new(self.db());
for element in union.elements(self.db()) {
builder =
builder.add(self.infer_binary_type_comparison(other, op, *element, range)?);
}
Ok(builder.build())
}

But I don't think this approach can be generalized to support mapping operations that return Results as well as mapping operations that return Options until the Try trait is stabilised in Rust.

Test Plan

cargo test -p ty_python_semantic

@AlexWaygood AlexWaygood added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Jun 24, 2025
@AlexWaygood AlexWaygood changed the title Introduce UnionType::try_from_elements and UnionType::try_map [ty] Introduce UnionType::try_from_elements and UnionType::try_map Jun 24, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 24, 2025

mypy_primer results

No ecosystem changes detected ✅

}

/// A fallible version of [`UnionType::map`].
pub(crate) fn try_map(
Copy link
Member

@MichaReiser MichaReiser Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe: try_map and try_map_result or try_map and try_map_ok?

and try_from_elements and try_from_result_elements?

I also think it's important to document what "fallible" means because it wasn't clear to me if it bails if there's any None value or if it skip sover None values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe: try_map and try_map_result or try_map and try_map_ok?

and try_from_elements and try_from_result_elements?

We could, but I don't think it's worth it right now considering that there's only one callsite that uses Results. We can add it later if that pattern becomes more common, I think.

I also think it's important to document what "fallible" means because it wasn't clear to me if it bails if there's any None value or if it skip sover None values.

👍 I'll push an update

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that the Try trait isn't stabilized :(

@AlexWaygood AlexWaygood enabled auto-merge (squash) June 24, 2025 12:05
@AlexWaygood AlexWaygood merged commit 237a582 into main Jun 24, 2025
34 checks passed
@AlexWaygood AlexWaygood deleted the alex/trymap branch June 24, 2025 12:09
dcreager added a commit that referenced this pull request Jun 24, 2025
* main:
  [ty] Fix false positives when subscripting an object inferred as having an `Intersection` type (#18920)
  [`flake8-use-pathlib`] Add autofix for `PTH202` (#18763)
  [ty] Add relative import completion tests
  [ty] Clarify what "cursor" means
  [ty] Add a cursor test builder
  [ty] Enforce sort order of completions (#18917)
  [formatter] Fix missing blank lines before decorated classes in .pyi files (#18888)
  Apply fix availability and applicability when adding to `DiagnosticGuard` and remove `NoqaCode::rule` (#18834)
  py-fuzzer: allow relative executable paths (#18915)
  [ty] Change `environment.root` to accept multiple paths (#18913)
  [ty] Rename `src.root` setting to `environment.root` (#18760)
  Use file path for detecting package root (#18914)
  Consider virtual path for various server actions (#18910)
  [ty] Introduce `UnionType::try_from_elements` and `UnionType::try_map` (#18911)
  [ty] Support narrowing on `isinstance()`/`issubclass()` if the second argument is a dynamic, intersection, union or typevar type (#18900)
  [ty] Add decorator check for implicit attribute assignments (#18587)
  [`ruff`] Trigger `RUF037` for empty string and byte strings (#18862)
  [ty] Avoid duplicate diagnostic in unpacking (#18897)
  [`pyupgrade`] Extend version detection to include `sys.version_info.major` (`UP036`) (#18633)
  [`ruff`] Frozen Dataclass default should be valid (`RUF009`) (#18735)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants