-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Propagate deref coercion into block #83850
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This is no my area of expertise, and also a language change. |
Wouldn't this lead to inference errors? I'm surprised we don't have tests that trip. But maybe it's special-cased enough to work. I'll defer to @nikomatsakis either way. |
Sorry @ldm0 ! I'll try to give useful feedback ASAP |
Is this something that we can figure out with a crater run? |
I would like to see this change made. There are two cases I know where deref coercion propagation is very useful. I have a set of macros that use variables from their scope internally. There is a set of storage structs that need to be taken by
Inside the macro the variables are bound like
And everything else will just work. Without the propagation the latter two bindings and the assignment to Of course, since the main storage and reference types I use are naturally unsized you might think I wouldn't complain, but there are other sets of sized structs that would greatly benefit from propagation in the general case.
Given what is already possible with unsized types, there shouldn't be any compiler side complications correct? I presume that proc-macro code output is compiled as if it were pasted into the edition? The main problem is just the number of regressions (or I'm not sure what is an acceptable amount of breakage, how bad is 128 root regressions of which many are minor versions of the same library), so I think a edition dependent change is something we could do. I glanced through some of the breakages and saw one related to a macro, that looked like an external binding problem. In the vast majority of cases when using older macros in a new edition context, external changes can fix the compilation, and there should be few internal problems since good practice labels types explicitly inside a macro. |
The crater run seems suspicious:
At the time the crater runs, there are some merge conflicts, I guess the error occurs because I haven't rebase it? @pnkfelix Could you start another crater run to ensure the real immediate breakage this PR will introduce? |
I'm fine to requeue a crater run to get an idea of how much of the breakage is spurious, but I'm not sure that those results will change the overall feeling against doing this now. @bors try Let's go ahead and @fcp close It might be worth revisiting in the future when we can model this better formally, but for now it's probably not a good idea. |
⌛ Trying commit 25845ff with merge f7637e1d7fb16748989c9113c7a0d47087e454f8... |
Whoops @rustbot fcp close |
@rfcbot fcp close |
@rfcbot close |
Team member @jackh726 has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
☔ The latest upstream changes (presumably #103310) made this pull request unmergeable. Please resolve the merge conflicts. |
Add tests for autoderef on block tail ref: rust-lang#83850 (comment)
The final comment period, with a disposition to close, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
Above code doesn't compiles because we apply
check_expr_with_expectation()
on{ Box::new(1i32) }
withExpectHasType(i32)
. Which is a hard constraint and won't success without deref coercion.This PR taks the same path as #20083 (remove the hard constraint on
check_expr_with_expectation
when meets a unsized type). When the compiler see aAddrOf(Foo(...))
(e.g.&{ Box::new(1i32) }
), removes the hard constraint that thisFoo
have some type.Fixes #26978
Fixes #57749
Fixes #83783