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

Suggest considering casting fn item as fn pointer in more cases #133382

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

mu001999
Copy link
Contributor

Fixes #132648

@rustbot
Copy link
Collaborator

rustbot commented Nov 23, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Nov 23, 2024
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 the contribution, but I think this logic is a bit too ad-hoc to land.

As an alternative approach, could you find the logic that ends up producing this note:

for that trait implementation, expected fn() found fn() {foo}

And instead try to add a note there? That logic should be responsible for peeling off all the types until the type mismatch, which should make it much simpler and not require us to reimplement a new kind of method to peel off ADTs to discover fn type mismatches.

@mu001999
Copy link
Contributor Author

@compiler-errors Just checking the found ty and expected ty at fulfillment_errors.rs will work, but it only handles the cases that we know both tys are fns.

I want to suggest in more cases, just like the second statement in the test, e.g., let _: Vec<fn()> = Vec::from([foo]);. We got expected Vec<fn()>, and found Vec<fn() {foo}>, so I add the logic to help do this.

Do you think just adding a help for the simple case is enough? If so I can open another PR which will do less ;)

@compiler-errors
Copy link
Member

@mu001999: What I'm saying is that this logic largely replicates, and I don't think I can see why this needs to be reimplemented in a particularly verbose way.

When we have a type mismatch like Vec<fn()> and Vec<fn() {foo}>, we return something called TypeError::Sorts that should contain the fn pointers themselves.

I think that working backwards to understand the function and trying to generalize the existing suggest_function_pointers function and also make sure it has enough information to understand situations like this is better than adding more logic to the compiler which will likely need to eventually be deduplicated in the future.

@mu001999
Copy link
Contributor Author

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned fee1-dead Nov 24, 2024
@compiler-errors
Copy link
Member

Sorry, don't have the context to get back into this PR.

r? compiler

@rustbot rustbot assigned lcnr and unassigned compiler-errors Jan 23, 2025
@lcnr
Copy link
Contributor

lcnr commented Jan 29, 2025

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 29, 2025

📌 Commit 4203627 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 Jan 29, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 29, 2025
Suggest considering casting fn item as fn pointer in more cases

Fixes rust-lang#132648
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#133382 (Suggest considering casting fn item as fn pointer in more cases)
 - rust-lang#136092 (Test pipes also when not running on Windows and Linux simultaneously)
 - rust-lang#136190 (Remove duplicated code in RISC-V asm bad-reg test)
 - rust-lang#136192 (ci: remove unused windows runner)
 - rust-lang#136205 (Properly check that array length is valid type during built-in unsizing in index)
 - rust-lang#136211 (Update mdbook to 0.4.44)
 - rust-lang#136212 (Tweak `&mut self` suggestion span)
 - rust-lang#136214 (Make crate AST mutation accessible for driver callback)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f8d103d into rust-lang:master Jan 29, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 29, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2025
Rollup merge of rust-lang#133382 - mu001999-contrib:diag/fnitem, r=lcnr

Suggest considering casting fn item as fn pointer in more cases

Fixes rust-lang#132648
@mu001999 mu001999 deleted the diag/fnitem branch February 7, 2025 15:10
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.

'Expected <function pointer>, found <function item>' diagnostic could explicitly suggest casting
6 participants