Stop skewing inference in ?'s desugaring #122412
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @nnethercote rustbot has assigned @nnethercote. Use r? to explicitly pick a reviewer |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
fmease
added
the
needs-fcp
scottmcm
added
the
I-lang-nominated
nnethercote
reviewed
Mar 13, 2024
| /// This is possible because `!` is uninhabited (has no values), so this function can't actually | ||
| /// be ever called at runtime. | ||
| /// | ||
| /// Even though `!` can be coerced to any type implicitly anyway (and indeed this function |
There was a problem hiding this comment.
"function is implemented"
nnethercote
reviewed
Mar 13, 2024
nnethercote
reviewed
Mar 13, 2024
|
I have skimmed this PR and gave minor comments, but this kind of subtle lang/libs/type system change is the kind of thing I am perfectly not suited to reviewing. The "needs-fcp" and "I-lang-nominated" labels have been added, so this clearly will get some scrutiny via other channels, but I will take a guess at a suitable reviewer and reassign anyway... r? @cuviper |
|
Is |
|
@cuviper I think it could plausibly become stable, at least if/when |
|
OK, well the actual implementation of r? libs-api |
rustbot
added
the
T-libs-api
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 20, 2024
Do not use `?`-induced skewing of type inference in the compiler This prevents breakage from rust-lang#122412 and is generally a good idea. r? `@estebank`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 20, 2024
Rollup merge of rust-lang#122540 - WaffleLapkin:ununexpected, r=estebank Do not use `?`-induced skewing of type inference in the compiler This prevents breakage from rust-lang#122412 and is generally a good idea. r? `@estebank`
|
API-wise this seems fine to add as unstable. I presume the next step is a crater run to evaluate the impact? |
|
FTR, I've commonly used |
|
We were unsure about how to think about this PR in the triage meeting today. Looking at this in particular,
it made me wonder whether it would be feasible to change |
@scottmcm I'm not sure. I don't think it's unfeasible, but it sure is harder than this. The issues are:
I'm not sure if this is a good idea or not. It's kinda weird. |
@Amanieu yes. |
|
@chorman0773 can you explain what you mean by "if this will cause issues in This PR makes all |
WaffleLapkin
added
T-lang
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 5, 2024
…-operator-to-not-screw-with-inference-will-it-obey, r=<try>
Stop skewing inference in ?'s desugaring
**NB**: this is a breaking change (although arguably a bug fix) and as such shouldn't be taken lightly.
This changes `expr?`'s desugaring like so (simplified, see code for more info):
```rust
// old
match expr {
Ok(val) => val,
Err(err) => return Err(err),
}
// new
match expr {
Ok(val) => val,
Err(err) => core::convert::absurd(return Err(err)),
}
// core::convert
pub const fn absurd<T>(x: !) -> T { x }
```
This prevents `!` from the `return` from skewing inference:
```rust
// previously: ok (never type spontaneous decay skews inference, `T = ()`)
// with this pr: can't infer the type for `T`
Err(())?;
```
Fixes rust-lang#51125
Closes rust-lang#39216
bors
added a commit
to rust-lang/cargo
that referenced
this pull request
Apr 5, 2024
Don't depend on `?` affecting type inference in weird ways This is likely to stop working in the future versions of Rust, see rust-lang/rust#122412 (comment).
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Try build successful - checks-actions |
|
@craterbot run start=master#385fa9d845dd326c6bbfd58c22244215e431948a end=try#645bb72776a6a56a1a8f52631a44bd082b2ba509 mode=check-only name=absurd-question-mark-desugar crates=https://crater-reports.s3.amazonaws.com/no-never-type-fallback/retry-regressed-list.txt (using #122955 regressions as the crate list, since this can only break things that #122955 broke) |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
8 tasks
|
r? compiler |
WaffleLapkin
added
S-waiting-on-author
traviscross
added
the
A-maybe-future-edition
|
@rustbot labels -I-lang-nominated We discussed this in the triage meeting today. Some people liked this change, and most liked the spirit of it, but there were some concerns. In particular, the many cases like this: fn result() -> Result<bool, ()> {
- Err(())?;
+ Err::<(), _>(())?;
Ok(true)
}...resulted in much discussion regarding, e.g., the churn and whether the motivation of this proposal makes it worth it. We did not reach consensus here other than that probably no consensus is possible at this time without a revised proposal, so we can unnominate until and unless such a revised proposal is available. |
rustbot
removed
the
I-lang-nominated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NB: this is a breaking change (although arguably a bug fix) and as such shouldn't be taken lightly.
This changes
expr?'s desugaring like so (simplified, see code for more info):This prevents
!from thereturnfrom skewing inference:Fixes #51125
Closes #39216