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

change message infered type for closure #84476

Conversation

ABouttefeux
Copy link
Contributor

fixes confusing message for inferred type for closure introduced in #84244.


I would prefer to have display something like return type inferred to be Foo<{integer}> here for code like

struct Foo<T>(T);

fn main() {
    || {
        if false {
            return Foo(0);
        }
        Foo(())
    };
}

But I was not able to to so. I would greatly appreciate If someone could help me or commit this change

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2021
@camelid
Copy link
Member

camelid commented May 28, 2021

r? @davidtwco (feel free to reassign)

@rust-highfive rust-highfive assigned davidtwco and unassigned estebank May 28, 2021
@camelid camelid added A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints labels May 28, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in getting to this. With the current changes made by this PR, I don't think it would be worth landing. However, let's see if we can't work out how to improve it in the way you've described in the description.

I would prefer to have display something like return type inferred to be `Foo<{integer}>` here for code like

struct Foo<T>(T);

fn main() {
    || {
        if false {
            return Foo(0);
        }
        Foo(())
    };
}

But I was not able to to so. I would greatly appreciate If someone could help me or commit this change

I haven't had time to look into this thoroughly, but ret_coercion_span is set here:

if self.ret_coercion_span.get().is_none() {
self.ret_coercion_span.set(Some(e.span));
}

Perhaps you could add some debug logging to see if any of the other values of e.span which would have been assigned to ret_coercion_span would have been more suitable, and if there would be a better heuristic than just keeping the first span (maybe the last, or the smallest?).

@davidtwco davidtwco 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2021
@crlf0710
Copy link
Member

crlf0710 commented Jul 4, 2021

@ABouttefeux Ping from triage, any updates on this?

@ABouttefeux
Copy link
Contributor Author

Sorry I can't work on this now, I am in a hospital.

@bstrie bstrie marked this pull request as draft July 21, 2021 19:40
@bstrie
Copy link
Contributor

bstrie commented Jul 21, 2021

@ABouttefeux Hope you get well soon! Since there's still some question as to what to implement here or how to implement it, I'm marking this PR as a draft so that we on the triage team don't keep bothering you until it's ready to review. If you need help, feel free to also ask questions on the #t-compiler/help Zulip channel at https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp . In the meantime if you get this in a state where it's ready for review then please remove the "Draft" designation and leave a comment saying that it's ready for review. On the other hand if you decide not to pursue this then feel free to close this PR. Thanks!

@bors
Copy link
Contributor

bors commented Aug 23, 2021

☔ The latest upstream changes (presumably #87661) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank
Copy link
Contributor

@ABouttefeux hope you are doing well. Do ping us if you need a hand.

@jackh726 jackh726 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 18, 2021
@Dylan-DPC
Copy link
Member

@ABouttefeux thanks for taking the time. I'm closing this due to inactivity, but if you have the time to do this pr, feel free to open a new PR preferably whenever you want :)

@Dylan-DPC Dylan-DPC closed this Feb 21, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. 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.