Conversation
sharkdp
requested review from
AlexWaygood,
carljm and
dcreager
as code owners
| # error: [ambiguous-protocol-member] "Consider adding an annotation, e.g. `m: int | str = ...`" | ||
| m = 1 if 1.2 > 3.4 else "a" |
There was a problem hiding this comment.
Yes, unlikely to ever appear in practice. But a nice way to demonstrate how the previous Type::literal_promotion_type was different from Type::promote_literals (this only works with the latter).
There was a problem hiding this comment.
unlikely to ever appear in practice
what do you mean, i write protocols like this all the time
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
| Type::Union(union) => { | ||
| return union | ||
| .elements(db) | ||
| .iter() | ||
| .all(|elem| should_give_hint(db, *elem)); | ||
| } |
There was a problem hiding this comment.
Admittedly more of a showcase for the refactor here. I'm also happy to remove this again (and the test above).
|
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
(All my comments are off-topic, but I guess I couldn't resist musing)
| # error: [ambiguous-protocol-member] "Consider adding an annotation, e.g. `m: int | str = ...`" | ||
| m = 1 if 1.2 > 3.4 else "a" |
There was a problem hiding this comment.
unlikely to ever appear in practice
what do you mean, i write protocols like this all the time
| !matches!( | ||
| class.known(db), | ||
| Some(KnownClass::NoneType | KnownClass::EllipsisType) | ||
| ) | ||
| } |
There was a problem hiding this comment.
I wonder if this could/should be something like
| class.known(db).is_none_or(|known_class| !known_class.is_singleton()) |
instead... but it probably doesn't really make a real-world difference. And off-topic for this PR.
| Type::BooleanLiteral(_) => KnownClass::Bool.to_instance(db), | ||
| Type::IntLiteral(_) => KnownClass::Int.to_instance(db), | ||
| Type::BytesLiteral(_) => KnownClass::Bytes.to_instance(db), | ||
| Type::ModuleLiteral(_) => KnownClass::ModuleType.to_instance(db), |
There was a problem hiding this comment.
I'm not actually sure when promotion of module-literal types is desirable -- we might want to remove it from this method. As you discovered on your "remove | Unknown from module-globals PR (sorry for looking while it was still in draft 🙈), promotion of module-literal types to types.ModuleType can cause serious issues because each module-literal type has a distinct set of attributes available on it. This makes module-literal types somewhat different to all our other literal types
There was a problem hiding this comment.
Was thinking the exact same thing when I changed this code. In fact, I did arrive here in the first place because promotion of module literals was causing the exact problem you are describing. I'll propose that as a separate change.
Edit: I should have read the rest of your comment before writing mine.
* main: (21 commits) [ty] Literal promotion refactor (#20646) [ty] Add tests for nested generic functions (#20631) [`cli`] Add conflict between `--add-noqa` and `--diff` options (#20642) [ty] Ensure first-party search paths always appear in a sensible order (#20629) [ty] Use `typing.Self` for the first parameter of instance methods (#20517) [ty] Remove unnecessary `parsed_module()` calls (#20630) Remove `TextEmitter` (#20595) [ty] Use fully qualified names to distinguish ambiguous protocols in diagnostics (#20627) [ty] Ecosystem analyzer: relax timeout thresholds (#20626) [ty] Apply type mappings to functions eagerly (#20596) [ty] Improve disambiguation of class names in diagnostics (#20603) Add the *The Basics* title back to CONTRIBUTING.md (#20624) [`playground`] Fix quick fixes for empty ranges in playground (#20599) Update dependency ruff to v0.13.2 (#20622) [`ruff`] Fix minor typos in doc comments (#20623) Update dependency PyYAML to v6.0.3 (#20621) Update cargo-bins/cargo-binstall action to v1.15.6 (#20620) Fixed documentation for try_consider_else (#20587) [ty] Use `Top` materializations for `TypeIs` special form (#20591) [ty] Simplify `Any | (Any & T)` to `Any` (#20593) ...
2 participants
Summary
Not sure if this was the original intention, but it looks to me like the previous
Type::literal_promotion_typewas more of an implementation detail for the actual operation of promoting all literals in a possibly-nested position of a type.This is not a pure refactor, as I'm technically changing the behavior for that protocols diagnostic message suggestion.
Test Plan
New Markdown test