Skip to content
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.

Sign up for GitHub

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

Jump to bottom

remove the unused :: between trait and type to give user correct diag… #102421

Merged
merged 1 commit into from
Sep 30, 2022
Merged

remove the unused :: between trait and type to give user correct diag… #102421

merged 1 commit into from
Sep 30, 2022

Conversation

lyming2007
Copy link

…nostic information

modified:   compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
new file:   src/test/ui/type/issue-101866.rs
new file:   src/test/ui/type/issue-101866.stderr

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 28, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2022
@lyming2007 lyming2007 mentioned this pull request Sep 28, 2022
Closed
@Rageking8
Copy link
Contributor

Rageking8 commented Sep 28, 2022 •
edited
Loading

@lyming2007 Please link the corresponding issue to close in your PR descriptions (for all your PRs where applicable). Thanks.

https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

lcnr
lcnr reviewed Sep 29, 2022
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit, then r=me

remove the unused :: between trait and type to give user correct diag…
523a76a 
…nostic information

	modified:   compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
	new file:   src/test/ui/type/issue-101866.rs
	new file:   src/test/ui/type/issue-101866.stderr
@lcnr
Copy link
Contributor

lcnr commented Sep 30, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 30, 2022

📌 Commit 523a76a has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2022
lcnr
lcnr reviewed Sep 30, 2022
View reviewed changes
compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

let mut suggestions = vec![(
trait_path_segment.ident.span.shrink_to_lo(),
format!("<{} as ", self.tcx.def_path(impl_def_id).to_string_no_crate_verbose())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sidenote, using self.tcx.def_path(impl_def_id).to_string_no_crate_verbose() here feels wrong, I would expect us to use self.tcx.type_of(impl_def_id) here to print the self type. rn this is causing us to print ::StructA instead of StructA.

@lyming2007 would you be interested in changing this in a followup pr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both <::StructA as TraitA<i32>>::func() and <StructA as TraitA<i32>>::func() will work for the compiler.
Do we really need to change it from ::StructA to StructA?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to do it, but it's a lot more idiomatic to use StructA instead of ::StructA. Using ::StructA is pretty confusing to most people, so it's better to avoid imo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see this PR #102670 @lcnr

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 30, 2022
@matthiaskrgr
Rollup merge of rust-lang#102421 - lyming2007:issue-101866, r=lcnr
71b2030 
remove the unused :: between trait and type to give user correct diag…

…nostic information

	modified:   compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
	new file:   src/test/ui/type/issue-101866.rs
	new file:   src/test/ui/type/issue-101866.stderr
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 30, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 30, 2022
@matthiaskrgr
Rollup merge of rust-lang#102421 - lyming2007:issue-101866, r=lcnr
73c2e99 
remove the unused :: between trait and type to give user correct diag…

…nostic information

	modified:   compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
	new file:   src/test/ui/type/issue-101866.rs
	new file:   src/test/ui/type/issue-101866.stderr
This was referenced Sep 30, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2022
@bors
Auto merge of rust-lang#102520 - matthiaskrgr:rollup-7nreat0, r=matth…
877877a 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#102276 (Added more const_closure functionality)
 - rust-lang#102382 (Manually order `DefId` on 64-bit big-endian)
 - rust-lang#102421 (remove the unused :: between trait and type to give user correct diag…)
 - rust-lang#102495 (Reinstate `hir-stats.rs` test for stage 1.)
 - rust-lang#102505 (rustdoc: remove no-op CSS `h3.variant, .sub-variant h4 { border-bottom: none }`)
 - rust-lang#102506 (Specify `DynKind::Dyn`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f7f253e into rust-lang:master Sep 30, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 30, 2022
@lyming2007 lyming2007 mentioned this pull request Oct 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

@cjgillot cjgillot cjgillot left review comments

Assignees

@lcnr lcnr

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lyming2007 @rust-highfive @Rageking8 @lcnr @bors @cjgillot @rustbot