-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Improve disambiguation of class names in diagnostics #20603
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
- src/bokeh/model/model.py:496:16: error[invalid-return-type] Return type does not match returned value: expected `set[Model]`, found `set[Model]`
+ src/bokeh/model/model.py:496:16: error[invalid-return-type] Return type does not match returned value: expected `set[src.bokeh.model.model.Model]`, found `set[src.bokeh.model.model.Model]`obviously not related to your PR, but it looks like something is wrong here? |
sharkdp
left a comment
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.
Thank you very much. Slightly unfortunate that we gain .clones in multiple places now, but this is probably not performance-critical.
Yes, definitely (multiple things, in fact...) -- but at least we now know that this specific case isn't solved by printing the fully qualified name in the diagnostics 😆 |
Yes, I didn't love this, but couldn't see an easy way to get around it. I think |
#20629) This PR ensures that we always put `./src` before `.` in our list of first-party search paths. This better emulates the fact that at runtime, the module name of a file `src/foo.py` would almost certainly be `foo` rather than `src.foo`. I wondered if fixing this might fix #20603 (comment). It seems like that's not the case, but it also seems like it leads to better diagnostics because we report much more intuitive module names to the user in our error messages -- so, it's probably a good change anyway.
* 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) ...
Summary
Currently we'll use the fully qualified name of a class in some cases when reporting instances where one class was expected but another one was found and the two classes have the same name. But there are still many cases where we won't do that -- e.g. in #20368, this is one of the new diagnostics:
This PR switches the
DisplaySettings::from_possibly_ambiguous_type_pair()function to use aTypeVisitorimplementation, so that we can be confident it will deeply recurse into a pair of types and identify possibly ambiguous type names wherever they might be hiding.Test Plan
Mdtests