[ty] Support variable-length tuples in unpacking assignments #18948
Conversation
|
dcreager
force-pushed
the
dcreager/unpack-tuple
branch
from
76757de to
05af0b3
Compare
* main: [ty] Add regression-benchmark for attribute-assignment hang (#18957) [ty] Format conflicting types as an enumeration (#18956) [ty] Prevent union builder construction for just one declaration (#18954) [ty] Infer nonlocal types as unions of all reachable bindings (#18750) [`pyflakes`] Mark `F504`/`F522`/`F523` autofix as unsafe if there's a call with side effect (#18839) [`playground`] Add ruff logo docs link to Header.tsx (#18947) [ty] Reduce the overwhelming complexity of `TypeInferenceBuilder::infer_call_expression` (#18943) [ty] Add subdiagnostic about empty bodies in more cases (#18942) [ty] Move search path resolution to `Options::to_program_settings` (#18937) [`flake8-errmsg`] Extend `EM101` to support byte strings (#18867) Move big rule implementations (#18931) [`pylint`] Allow fix with comments and document performance implications (`PLW3301`) (#18936)
| | Expected 3 or more | ||
| | Expected at least 3 |
There was a problem hiding this comment.
These rewordings aren't because I had a strong opinion about the word choice; it's just that we're now using the same display machinery that we were using for e.g. the index-out-of-bounds diagnostics when indexing into a tuple.
| #[allow(unsafe_code)] | ||
| unsafe impl<T> salsa::Update for FixedLengthTuple<T> | ||
| where | ||
| T: salsa::Update, | ||
| { | ||
| unsafe fn maybe_update(old_pointer: *mut Self, new_value: Self) -> bool { | ||
| unsafe { | ||
| let old_value = &mut *old_pointer; | ||
| Vec::maybe_update(&mut old_value.0, new_value.0) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't love this, but it was necessary to implement salsa::Update only if T also implements it. UnionBuilder does not, and it was looking very invasive to add it.
dcreager
changed the title
|
I've spot-checked the new ecosystem results, and they all seem to be coming from unpacking variable-length tuples. For example, there are many new messages involving a, b, c, d, e, f, g, h, i, j = symbols('a, b, c, d, e, f, g, h, i, j', seq=True)
k, l, m, n, o, p, q, r, s, t = symbols('k, l, m, n, o, p, q, r, s, t', seq=True)
u, v, w, x, y, z = symbols('u, v, w, x, y, z', seq=True)( |
There was a problem hiding this comment.
Nice!
It feels like a fair amount of complexity was introduced to our tuple representation in order to support it also representing a tuple of union-builders. I think this is probably still simpler / less duplicative than having a separate unpack-targets implementation, but it seems like some of this complexity currently leaks out to external users of tuple types. It seems like that can probably be cleaned up with some additional methods specific to tuples of Type?
| .all_elements() | ||
| .map(TupleElement::into_inner) | ||
| .copied() |
There was a problem hiding this comment.
We seem to do this a lot -- should it be a method?
There was a problem hiding this comment.
Done — made separate all_elements vs all_elements_with_kind. All of the existing callers still use all_elements, don't have to call into_inner (but do still have to add a .copied()) The new code that needs to distinguish between the different tuple element positions now calls all_elements_with_kind and has to match on the result.
* main: [ty] Add builtins to completions derived from scope (#18982) [ty] Don't add incorrect subdiagnostic for unresolved reference (#18487) [ty] Simplify `KnownClass::check_call()` and `KnownFunction::check_call()` (#18981) [ty] Add micro-benchmark for #711 (#18979) [`flake8-annotations`] Make `ANN401` example error out-of-the-box (#18974) [`flake8-async`] Make `ASYNC110` example error out-of-the-box (#18975) [pandas]: Fix issue on `non pandas` dataframe `in-place` usage (PD002) (#18963) [`pylint`] Fix `PLC0415` example (#18970) [ty] Add environment variable to dump Salsa memory usage stats (#18928) [`pylint`] Fix `PLW0108` autofix introducing a syntax error when the lambda's body contains an assignment expression (#18678) Bump 0.12.1 (#18969) [`FastAPI`] Add fix safety section to `FAST002` (#18940) [ty] Add regression test for leading tab mis-alignment in diagnostic rendering (#18965) [ty] Resolve python environment in `Options::to_program_settings` (#18960) [`ruff`] Fix false positives and negatives in `RUF010` (#18690) [ty] Fix rendering of long lines that are indented with tabs [ty] Add regression test for diagnostic rendering panic [ty] Move venv and conda env discovery to `SearchPath::from_settings` (#18938)
There was a problem hiding this comment.
It feels like a fair amount of complexity was introduced to our tuple representation in order to support it also representing a tuple of union-builders. I think this is probably still simpler / less duplicative than having a separate unpack-targets implementation, but it seems like some of this complexity currently leaks out to external users of tuple types. It seems like that can probably be cleaned up with some additional methods specific to tuples of
Type?
I tweaked some things to make this less impactful on the existing Type-tuple callers. The main difference now is just that the iterators return &Type instead of Type, so there are some additional copies.
I also separated out the unpack_tuple logic into two steps: resizing the rhs tuple of types to have the same length as the lhs tuple of targets, and then pairwise adding each type to the target UnionBuilder. I think the new resize method will also be helpful in the forthcoming PR to splat arguments into parameters.
| .all_elements() | ||
| .map(TupleElement::into_inner) | ||
| .copied() |
There was a problem hiding this comment.
Done — made separate all_elements vs all_elements_with_kind. All of the existing callers still use all_elements, don't have to call into_inner (but do still have to add a .copied()) The new code that needs to distinguish between the different tuple element positions now calls all_elements_with_kind and has to match on the result.
| /// Resizes this tuple to a different length, if possible. If this tuple cannot satisfy the | ||
| /// desired minimum or maximum length, we return an error. If we return an `Ok` result, the | ||
| /// [`len`][Self::len] of the resulting tuple is guaranteed to be equal to `new_length`. | ||
| pub(crate) fn resize( |
There was a problem hiding this comment.
Eventually it would be nice to make this work for any Tuple<T>, but then you'd have to pass in additional parameters to specify how to combine elements. For now I've just defined it only for Tuple<Type<'db>> and hard-coded combining the types by unioning them.
CodSpeed WallTime Performance ReportMerging #18948 will improve performances by 4.49%Comparing Summary
Benchmarks breakdown
|
| /// unpack the values from a rhs tuple into those targets. If the rhs is a union, call | ||
| /// `unpack_tuple` separately for each element of the union. We will automatically wrap the types | ||
| /// assigned to the starred target in `list`. | ||
| pub(crate) struct TupleUnpacker<'db> { |
| pub(crate) enum TupleLength { | ||
| Fixed(usize), | ||
| Variable(usize, usize), | ||
| } |
There was a problem hiding this comment.
nit: it might be useful to document what these usizes are, especially the ones in Variable variant or just a reference to VariableLengthTuple to indicate that these are the length of prefix and suffix respectively
This PR updates our unpacking assignment logic to use the new tuple machinery. As a result, we can now unpack variable-length tuples correctly.
As part of this, the
TupleSpecclasses have been renamed toTuple, and can now contain any element (Rust) type, not justType<'db>. The unpacker uses a tuple ofUnionBuilders to maintain the types that will be assigned to each target, as we iterate through potentially many union elements on the rhs. We also add a new consuming iterator for tuples, and update theall_elementsmethods to wrap the result in an enum (similar toitertools::Position) letting you know which part of the tuple each element appears in. I also added a newUnionBuilder::try_build, which lets you specify a different fallback type if the union contains no elements.