-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
path trimming: ignore type aliases #78052
Conversation
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
I'm not sure what is the logic here besides "this makes |
e86c1e2
to
52c95cd
Compare
Not sure about treating this as special case. If there are other type names colliding with type aliases, the tests don't reveal them. We'll get a better picture by listing all the type aliases in |
☔ The latest upstream changes (presumably #77373) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
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.
Implementation LGTM; I think I like this change, but I'd like to clarify a few things. These are the primary cases that this concerns, right?
- Alias has same name as type that it aliases - alias would be printed using the full path, but (more common) use of type wouldn't (e.g.
Result
). This seems reasonable, improves the common case and I don't think it'd lead to much confusion. - Alias has the same name as an entirely unrelated type (from a different crate, for example), alias would be printed using the full path and if unrelated type were used then it would have its short path printed. Could this lead to a user using alias
Foo
without a path throughout their code, and then having typeFoo
(used in their code asfoo::Foo
) be printed without the full path in diagnostics? That seems like it could be confusing.
@davidtwco can you point to cases where paths to aliases are being printed? AFAIK the type system resolves type aliases so that printed types are fully resolved and are not related to them at all. |
I was just assuming that was the behaviour. If that isn't the case then I think I'm happy with this change. |
@davidtwco do you wish to review this again? Or can we consider "happy with this change" as an approval? (pending the trivial conflicts) |
Apologies, I should have been clearer and followed-up - I'm happy to see this land after a rebase. |
52c95cd
to
247c9c7
Compare
☔ The latest upstream changes (presumably #78343) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
@da-x Ping from triage. I'm sorry but this seems needs a rebase again. |
247c9c7
to
98f43e1
Compare
Apologies again for being so slow to get back to this review - looks good now. @bors r+ |
📌 Commit 98f43e1e7c287eda1aeaed8fffe0254f994ef320 has been approved by |
⌛ Testing commit 98f43e1e7c287eda1aeaed8fffe0254f994ef320 with merge 3aa1bf2bc01ad967a5d3db4e42bbe941f92e2f14... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors r- rollup=iffy failed in #81528 (comment) |
Looks like disabling trimmed paths for that test either didn't work, or there were trimmed paths in the output that weren't undone. |
☔ The latest upstream changes (presumably #80851) made this pull request unmergeable. Please resolve the merge conflicts. |
9dc0d1d
to
0752415
Compare
My mistake so far was not building with Let's see how it runs now. :) |
@bors r=davidtwco |
📌 Commit 0752415f88179d8a828f95c6569ce0a834981161 has been approved by |
☔ The latest upstream changes (presumably #81257) made this pull request unmergeable. Please resolve the merge conflicts. |
0752415
to
6495029
Compare
@bors r=davidtwco |
📌 Commit 6495029 has been approved by |
☀️ Test successful - checks-actions |
Continuation of #73996.