[red-knot] Don't use separate ID types for each alist#16415
Conversation
dcreager
added
internal
dcreager
requested review from
AlexWaygood,
MichaReiser,
carljm and
sharkdp
as code owners
crates/ruff_index/src/list.rs
Outdated
| impl<K, V> ListStorage<K, V> { | ||
| /// Iterates through the entries in a list _in reverse order by key_. | ||
| pub fn iter_reverse(&self, list: Option<I>) -> ListReverseIterator<'_, I, K, V> { | ||
| #[allow(clippy::needless_pass_by_value)] |
There was a problem hiding this comment.
List is Copy, so I'm not sure why clippy want us to always pass this around by value like we were before. I guess the extra phantom fields increase the perceived complexity?
|
There was a problem hiding this comment.
It's not entirely clear to me what the benefit of removing the Index type is. Didn't it give us some added type safety that prevented mixing different list types?
Edit: It's very likely that I simply don't understand the data structure enough to see the benefit of it. In that case, feel free to disregard this comment.
crates/ruff_index/src/list.rs
Outdated
| #[derive(Debug, Eq, PartialEq)] | ||
| pub struct ListStorage<I, K, V = ()> { | ||
| cells: IndexVec<I, ListCell<I, K, V>>, | ||
| pub struct ListStorage<K, V = ()> { |
There was a problem hiding this comment.
Removing the I makes this type less a clear fit for the ruff_index crate because it's no longer an indexed type.
What's the benefit of removing the I type other than just removing the need to define an index newtype wrapper type? Didn't it protect that you could pass a list for another ListStorage with the same K, V to a ListStorage for another index?
There was a problem hiding this comment.
What's the benefit of removing the
Itype other than just removing the need to define an index newtype wrapper type? Didn't it protect that you could pass a list for anotherListStoragewith the sameK,Vto aListStoragefor another index?
We weren't (and as far as I know, aren't planning on) creating any lists with the same K, V but different I types. So I was viewing this as removing a redundancy.
Removing the
Imakes this type less a clear fit for theruff_indexcrate because it's no longer an indexed type.
That's fair — would you want me to move this into a different crate if we keep the change? Is there an existing crate that's a better fit, or would it deserve its own?
There was a problem hiding this comment.
We weren't (and as far as I know, aren't planning on) creating any lists with the same K, V but different I types. So I was viewing this as removing a redundancy.
If there's value in having the separate index type than I think I'd prefer to keep it and instead have type aliases in red_knot_python_semantic or to move the types into red_knot_python_semantic if we don't intent to use it elsewhere.
That's fair — would you want me to move this into a different crate if we keep the change? Is there an existing crate that's a better fit, or would it deserve its own?
That's a tough question. I don't think there's a fitting crate for it but creating its own crate also feels somewhat overkill. I'm okay leaving it here or I'd move it into red_knot_python_semantic, considering it's the only place where we use the type.
* main: [red-knot] unify LoopState and saved_break_states (#16406) [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378) [FURB156] Do not consider docstring(s) (#16391) Use `is_none_or` in `stdlib-module-shadowing` (#16402) [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398) [red-knot] Fix file watching for new non-project files (#16395) document MSRV policy (#16384) [red-knot] fix non-callable reporting for unions (#16387) bump MSRV to 1.83 (#16294) Avoid unnecessary info at non-trace server log level (#16389) Expand `ruff.configuration` to allow inline config (#16296) Start detecting version-related syntax errors in the parser (#16090)
|
Discussed this briefly with @dcreager in our 1:1. I don't think this really loses any type safety that we had previously. If you want to create two list arenas with the same The most likely mistake in our use of these lists is that we would mistakenly use a list from one scope's list arena with the arena from a different scope. But these arenas are all the same type (and use the same newtype-index today), so we already don't have protection from this. (It's no different than any of our per-scope IndexVec newtype indices, which are invalid if used across scopes, and we don't have compiler-level protection from this.) |
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks fine to me. No opposition to moving it into red_knot_python_semantic, at least until an external use case shows up.
* main: [red-knot] Switch to a handwritten parser for mdtest error assertions (#16422) [red-knot] Disallow more invalid type expressions (#16427) Bump version to Ruff 0.9.9 (#16434) Check `LinterSettings::preview` for version-related syntax errors (#16429) Avoid caching files with unsupported syntax errors (#16425) Prioritize "bug" label for changelog sections (#16433) [`flake8-copyright`] Add links to applicable options (`CPY001`) (#16421) Fix string-length limit in documentation for PYI054 (#16432) Show version-related syntax errors in the playground (#16419) Allow passing `ParseOptions` to inline tests (#16357) Bump version to 0.9.8 (#16414) [red-knot] Ignore surrounding whitespace when looking for `<!-- snapshot-diagnostics -->` directives in mdtests (#16380) Notify users for invalid client settings (#16361) Avoid indexing the project if `configurationPreference` is `editorOnly` (#16381)
Done. That required removing some methods that aren't currently being used (union, list iterator), but we can always resurrect those from the git history if needed. |
3 participants
Regardless of whether #16408 and #16311 pan out, this part is worth pulling out as a separate PR.
Before, you had to define a new
IndexVecindex type for each type of association list you wanted to create. Now there's a single index type that's internal to the alist implementation, and you useList<K, V>to store a handle to a particular list.This also adds some property tests for the alist implementation.