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.

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

Print note with closure signature on type mismatch #123379

Merged
merged 1 commit into from Apr 21, 2024

Conversation

wutchzone
Copy link
Contributor

Fixes #119266

r? Nilstrieb

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 2, 2024
@@ -266,6 +266,11 @@ impl<'tcx> ClosureArgs<'tcx> {
pub fn print_as_impl_trait(self) -> ty::print::PrintClosureAsImpl<'tcx> {
ty::print::PrintClosureAsImpl { closure: self }
}

/// Prints closure signature in form of `fn(Args) -> Output` with stripped `extern "rust-call`.
pub fn print_signature(self) -> ty::print::PrintClosureSignature<'tcx> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just reuse print_as_impl_trait? Seems like it does basically the same thing, except for being prefixed with impl Fn instead of fn. I don't expect users to particularly care, since the point is the signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe we can reuse that if we do not have anything against prefixed impl Fn, it conveys the same information. However, it may be confusing for the reader of the note because in the first note, we say expected fn pointer fn(args), and possibly in the second one ..has signature: impl Fn(args), won't the reader be confused by fn vs Fn? My reasoning for using the current output of ..has signature: fn(args) is just for consistency with the first note, but if you think it is negligible, I will change it for the impl variant.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this matters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found out that it is also possible to extract the fn-like signature, without the impl from the closure more easily with the signature_unclosure, so this may be even better?

@Nilstrieb
Copy link
Member

r? compiler-errors

@@ -721,6 +721,16 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> {
self
}

#[rustc_lint_diagnostics]
Copy link
Member

Choose a reason for hiding this comment

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

Can you inline this into the one caller that it's used? Also, this could be a Subdiagnostic, perhaps? I'm not sure if highlighted_note handles translatable string parts...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree this should probably be Subdiagnostic, however the highlighted_note is accepting only StringPart and not the impl Into<SubdiagMessage> (which is the translatable string part right?). Should address the highlighted_note so it is accepting the SubdiagMessage in this PR?
Also, we can do it without colorshighlighting for now.

Copy link
Member

Choose a reason for hiding this comment

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

Nah, it's fine.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2024

📌 Commit be564a8 has been approved by compiler-errors

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 Apr 20, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Apr 20, 2024
Print note with closure signature on type mismatch

Fixes rust-lang#119266

r? Nilstrieb
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2024
Rollup of 11 pull requests

Successful merges:

 - rust-lang#123379 (Print note with closure signature on type mismatch)
 - rust-lang#123967 (static_mut_refs: use raw pointers to remove the remaining FIXME)
 - rust-lang#123976 (Use fake libc in core test)
 - rust-lang#123986 (lint-docs: Add redirects for renamed lints.)
 - rust-lang#124053 (coverage: Branch coverage tests for lazy boolean operators)
 - rust-lang#124071 (Add llvm-bitcode-linker to build manifest)
 - rust-lang#124103 (Improve std::fs::Metadata Debug representation)
 - rust-lang#124132 (llvm RustWrapper: explain OpBundlesIndirect argument type)
 - rust-lang#124191 (Give a name to each distinct manipulation of pretty-printer FixupContext)
 - rust-lang#124193 (Miri subtree update)
 - rust-lang#124196 (mir-opt tests: rename unit-test -> test-mir-pass)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2024
Rollup of 10 pull requests

Successful merges:

 - rust-lang#123379 (Print note with closure signature on type mismatch)
 - rust-lang#123967 (static_mut_refs: use raw pointers to remove the remaining FIXME)
 - rust-lang#123976 (Use fake libc in core test)
 - rust-lang#123986 (lint-docs: Add redirects for renamed lints.)
 - rust-lang#124053 (coverage: Branch coverage tests for lazy boolean operators)
 - rust-lang#124071 (Add llvm-bitcode-linker to build manifest)
 - rust-lang#124103 (Improve std::fs::Metadata Debug representation)
 - rust-lang#124132 (llvm RustWrapper: explain OpBundlesIndirect argument type)
 - rust-lang#124191 (Give a name to each distinct manipulation of pretty-printer FixupContext)
 - rust-lang#124196 (mir-opt tests: rename unit-test -> test-mir-pass)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e9e936c into rust-lang:master Apr 21, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 21, 2024
Rollup merge of rust-lang#123379 - wutchzone:119266, r=compiler-errors

Print note with closure signature on type mismatch

Fixes rust-lang#119266

r? Nilstrieb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Closure signature not mentioned on mismatch with fn ptr
5 participants