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

Defaulted unit types no longer error out (regression?) #51125

Open
Manishearth opened this issue May 28, 2018 · 3 comments
Open

Defaulted unit types no longer error out (regression?) #51125

Manishearth opened this issue May 28, 2018 · 3 comments
Labels
A-inference Area: Type inference C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

This currently compiles (on stable and nightly). Till 1.25, it would trigger a lint because it has inference default to () instead of throwing an error.

fn main() {}

struct Err;
fn load<T: Default>() -> Result<T, Err> {
    Ok(T::default())
}
fn foo() -> Result<(), Err> {
    let val = load()?; // defaults to ()
    Ok(())
}

(playpen)

That lint indicates that it would become a hard error in the future, but it's not erroring. It seems like a bunch of this was changed when we stabilized !.

That issue says

Type inference will now default unconstrained type variables to ! instead of (). The resolve_trait_on_defaulted_unit lint has been retired. An example of where this comes up is if you have something like:

Though this doesn't really make sense, this looks like a safe way to produce nevers, which should, in short, never happen. It seems like this is related to #40801 -- but that was closed as it seems to be a more drastic change.

Also, if you print val, it's clear that the compiler thought it was a unit type. This seems like one of those cases where attempting to observe the situation changes it.

We should have a hard error here, this looks like a footgun, and as @SimonSapin mentioned has broken some unsafe code already. We had an upgrade lint for the hard error and we'll need to reintroduce it for a cycle or two since we've had a release without it. AFAICT this is a less drastic change than #40801, and we seem to have intended for this to be a hard error before so it probably is minor enough that it's fine.

cc @nikomatsakis @eddyb

h/t @spacekookie for finding this

@Manishearth Manishearth added the A-inference Area: Type inference label May 28, 2018
@XAMPPRocky XAMPPRocky added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Oct 2, 2018
@Enselic Enselic added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Nov 18, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 18, 2023
@Enselic
Copy link
Member

Enselic commented Nov 18, 2023

Triage: Even though the issue is old, it seems serious enough to be formally marked as a regression. So I added a regression label.

@apiraino
Copy link
Contributor

Well, yes this is technically a regression (albeit very old) but it is tracked in #39216 and recently added to a 2024 edition milestone. So if I understand the context this issue is not "forgotten" but doesn't require immediate attention.

@rustbot label -I-prioritize

@rustbot rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 20, 2023
@WaffleLapkin
Copy link
Member

WaffleLapkin commented Mar 12, 2024

So the issue is how desugating of ? interacts with ! (ahaha).

The code in the issue description gets desugared to something like this (heavily simplified) (I've renamed types/variables for clarity):

struct E;

fn load<T: Default>() -> Result<T, E> { ... }

fn foo() -> Result<(), E> {
    let v = match load() {
        Ok(val) => val,
        Err(err) => return Err(err),
    };
    Ok(())
}

In this desugared form it's clear that val's type if affected by ?/return. The match requires ?M <: ?V, ?M <: ?D where ?M type of the match/v, ?V is the type of val/load's generic, ?D is a diverging inference variable that comes from never-to-any coercion. Then we fill ?M = ?V = ?D = () because of the spontaneous never decay / diverging fallback.

? should not do that, this code should cause an error like "can't inference blah blah blah". I'll try to change the desugaring to something like this:

fn foo() -> Result<(), E> {
    let v = {
        let ret;
        match load() {
            Ok(val) => ret = val,
            Err(err) => return Err(err),
        };
        ret
    };
    Ok(())
}

To eliminate the return influnce on val's type.

Upd: this desugaring does not work, because it changes the way ? interacts with temporaries :(
Upd: I found a couple desugarings that actually work and opened #122412 with the least intrusive one

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 3, 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-ci/rust that referenced this issue Apr 4, 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-ci/rust that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inference Area: Type inference C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants