-
Notifications
You must be signed in to change notification settings - Fork 2k
[ty] Improve sys.version_info special casing
#19894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ use crate::types::{ | |
| UnionBuilder, UnionType, cyclic::PairVisitor, | ||
| }; | ||
| use crate::util::subscript::{Nth, OutOfBoundsError, PyIndex, PySlice, StepSizeZeroError}; | ||
| use crate::{Db, FxOrderSet}; | ||
| use crate::{Db, FxOrderSet, Program}; | ||
|
|
||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub(crate) enum TupleLength { | ||
|
|
@@ -1162,6 +1162,34 @@ impl<'db> Tuple<Type<'db>> { | |
| Tuple::Variable(_) => false, | ||
| } | ||
| } | ||
|
|
||
| /// Return the `TupleSpec` for the singleton `sys.version_info` | ||
| pub(crate) fn version_info_spec(db: &'db dyn Db) -> TupleSpec<'db> { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fairly easy to add caching to this method if we revert d76fd10, but I still haven't been able to find any evidence yet that this would provide a significant speedup. I also haven't tried doing that as an isolated change on the codspeed runners, though; I've only experimented by running benchmarks locally. |
||
| let python_version = Program::get(db).python_version(db); | ||
| let int_instance_ty = KnownClass::Int.to_instance(db); | ||
|
|
||
| // TODO: just grab this type from typeshed (it's a `sys._ReleaseLevel` type alias there) | ||
| let release_level_ty = { | ||
| let elements: Box<[Type<'db>]> = ["alpha", "beta", "candidate", "final"] | ||
| .iter() | ||
| .map(|level| Type::string_literal(db, level)) | ||
| .collect(); | ||
|
|
||
| // For most unions, it's better to go via `UnionType::from_elements` or use `UnionBuilder`; | ||
| // those techniques ensure that union elements are deduplicated and unions are eagerly simplified | ||
| // into other types where necessary. Here, however, we know that there are no duplicates | ||
| // in this union, so it's probably more efficient to use `UnionType::new()` directly. | ||
| Type::Union(UnionType::new(db, elements)) | ||
| }; | ||
|
|
||
| TupleSpec::from_elements([ | ||
| Type::IntLiteral(python_version.major.into()), | ||
| Type::IntLiteral(python_version.minor.into()), | ||
| int_instance_ty, | ||
| release_level_ty, | ||
| int_instance_ty, | ||
| ]) | ||
| } | ||
| } | ||
|
|
||
| impl<T> From<FixedLengthTuple<T>> for Tuple<T> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just something to be aware of. This has a risk of lock congestion if
explict_basesis very hot because we keep interning the sameTupleType. I don't thinkexplict_basesis that hot that this is an issue here but I thought it's worth noting (and it's also something that we might just need to fix in Salsa)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is likely not an issue here because we only intern when we call
explicit_baseson theVersionInfoclass specifically. And this method is cached so if we do that frequently, all but the first time in a revision wouldn't reach here anyway.