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

Improve fn pointer notes #107287

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Improve fn pointer notes #107287

merged 1 commit into from
Jan 26, 2023

Conversation

mattjperez
Copy link
Contributor

continuation of #105552

r? @compiler-errors

@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2023

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

Please see the contribution instructions for more information.

@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 Jan 25, 2023
@mattjperez mattjperez force-pushed the improve-fn-pointer-notes branch 3 times, most recently from 1371c1c to 78c2035 Compare January 25, 2023 17:05
@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@mattjperez
Copy link
Contributor Author

rebased onto master and ready for review

eggyal
eggyal suggested changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@eggyal eggyal left a comment

Choose a reason for hiding this comment

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

Just the one suggestion from me; otherwise looks good!

compiler/rustc_infer/src/infer/error_reporting/suggest.rs Outdated Show resolved Hide resolved
compiler-errors
compiler-errors reviewed Jan 26, 2023
View reviewed changes
compiler/rustc_infer/src/infer/error_reporting/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/suggest.rs Outdated
}
};
let msg = format!(
"consider casting the expected and found fn items to fn pointers, i.e. `{}` and the following suggestion",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"consider casting the expected and found fn items to fn pointers, i.e. `{}` and the following suggestion",
"consider casting both fn items to fn pointers using `as ....`",

(fill in the .... with just the signature they need to be casted to)

And then perhaps using https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/diagnostic/struct.Diagnostic.html#method.span_suggestion_hidden

Copy link
Member

Choose a reason for hiding this comment

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

Let me explain my thinking here: the phrasing "expected and found fn items" is a bit verbose, and "and the following suggestion" is a bit awkward. Let's avoid both by just explaining the gist of what the user needs to do.

Copy link
Contributor Author

@mattjperez mattjperez Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that explanation is much nicer to read. --verbose is my default and it has been much harder than expected to be concise.

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 applied the suggestions.

@mattjperez mattjperez force-pushed the improve-fn-pointer-notes branch 2 times, most recently from 0a71c1d to 6baec2c Compare January 26, 2023 03:29
compiler-errors
compiler-errors approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this, and for applying the suggestions.

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 26, 2023

📌 Commit 6baec2c91668ae53a7631f10a911b4a1837710cf 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 Jan 26, 2023
@compiler-errors
Copy link
Member

Sorry for the back and forth, just one tiny nit I totally overlooked

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 26, 2023
@mattjperez
improve fn pointer notes
3016f55 
- add note and suggestion for casting both expected and found fn items
  to fn pointers
- add note for casting expected fn item to fn pointer
@compiler-errors
Copy link
Member

ty

@bors r+

@bors
Copy link
Contributor

bors commented Jan 26, 2023

📌 Commit 3016f55 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 26, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 26, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 26, 2023
@bors
Auto merge of rust-lang#107318 - matthiaskrgr:rollup-776kd81, r=matth…
40fda7b 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#97373 (impl DispatchFromDyn for Cell and UnsafeCell)
 - rust-lang#106625 (Remove backwards compat for LLVM 12 coverage format)
 - rust-lang#106779 (Avoid __cxa_thread_atexit_impl on Emscripten)
 - rust-lang#106811 (Append .dwp to the binary filename instead of replacing the existing extension.)
 - rust-lang#106836 (Remove optimistic spinning from `mpsc::SyncSender`)
 - rust-lang#106946 (implement Hash for proc_macro::LineColumn)
 - rust-lang#107074 (remove unnecessary check for opaque types)
 - rust-lang#107287 (Improve fn pointer notes)
 - rust-lang#107304 (Use `can_eq` to compare types for default assoc type error)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f2f1234 into rust-lang:master Jan 26, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

@eggyal eggyal eggyal approved these changes

Assignees

@compiler-errors compiler-errors

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.69.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mattjperez @rustbot @compiler-errors @bors @eggyal