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

Decision on "must define before use" for opaque types #117866

Open
traviscross opened this issue Nov 13, 2023 · 9 comments
Open

Decision on "must define before use" for opaque types #117866

traviscross opened this issue Nov 13, 2023 · 9 comments
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@traviscross
Copy link
Contributor

traviscross commented Nov 13, 2023

Nominating for T-lang to decide whether the following rule should hold for opaque types:

If the body of an item that may define the hidden type of some opaque does define that hidden type, it must do so syntactically before using the opaque type in a non-defining way.

This restriction is called "must define before use."

Consider:

use core::convert::identity;

struct I;
struct IShow;
impl I { fn show(&self) -> IShow { IShow } }

struct OnIShow;
trait OnI { fn show(&self) -> OnIShow { OnIShow } }
impl OnI for I {}

fn test(n: bool) -> impl OnI {
    let true = n else { loop {} };
    let x = test(!n); //~ NOTE this is the opaque type
    let _: OnIShow = x.show(); //~ NOTE this is a non-defining use
    //~^ ERROR if the body registers a hidden type for the opaque, it
    //         must do so *before* using it opaquely
    let _: IShow = identity::<I>(x).show();
    //~^ NOTE this registers a hidden type for the opaque, but does so
    //        too late
    loop {}
}

fn main() {}

Because the code above works today, this would be a breaking change for RPIT.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 13, 2023
@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` I-lang-nominated Nominated for discussion during a lang team meeting. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 13, 2023
@traviscross
Copy link
Contributor Author

@rustbot labels -I-lang-nominated

In further discussion, @compiler-errors has described that he's not worried about this and that there are likely ways that we could implement the simple forms of passthrough that one might want -- if we wanted them -- without the restriction proposed here. If he's not worried about it, then neither am I. So removing the nomination and closing.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 8, 2024
@traviscross
Copy link
Contributor Author

traviscross commented Feb 7, 2024

@rustbot labels +I-lang-nominated

Reopening as @lcnr has now proposed a similar rule.

(There is some relation here to #120549.)

@traviscross traviscross reopened this Feb 7, 2024
@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 7, 2024
@traviscross traviscross changed the title TAIT decision on "must define before use" Decision on "must define before use" for opaque types Feb 7, 2024
@RalfJung
Copy link
Member

So "before" here is meant in the sense of "syntactically before the other one in a function"? Or "before" in a data-flow sense? E.g., in if ... { a } else { b }, is a "before" b or not?

Rust usually doesn't care much for syntactic "before", for things like which items I can reference and for type inference. So what is the motivation for imposing such a requirement on opaque types?

@lcnr
Copy link
Contributor

lcnr commented Feb 15, 2024

before in a "walk the hir during hir typeck" sense 😁

the reason why this requirement would be desirable is that it allows us to simplify the handling of opaque types, especially wrt the new solver. We did a crater run emulating this change in #120798. This only impacts places in HIR typeck where we need to eagerly make a decision, i.e. cannot defer it by e.g. emitting an obligation instead.

While there are other places which would be impacted here, the main change is its impact on method resultion: expr.method() eagerly selects which method to use, emitting an error if its ambiguous. If the type of expr is an opaque type in its defining scope we currently resolve method by using the "item bounds" of the opaque.

Ideally opaques are handled similar to other aliases (e.g. associated types) and eagerly normalized, changing the type of expr to be an inference variable (and we remember that the opaque normalizes to that inference variable). This breaks the following example and also resulted in multiple failures found in the crater run:

use std::future::Future;
use futures::FutureExt;

fn go(i: usize) -> impl Future<Output = ()> + Send + 'static {
    async move {
        if i != 0 {
            // This returns `impl Future<Output = ()>` in its defining scope,
            // we don't know the concrete type of that opaque at this point.
            // Currently treats the opaque as a known type and succeeds, but
            // from the perspective of "easiest to soundly implement", it would
            // be good for this to be ambiguous.
            go(i - 1).boxed().await;
        }
    }
}

Changing this example to error with ambiguity breaks existing crates and this pattern also seems generally desirable, e.g. https://github.com/kurnevsky/esxtool/blob/88538ca7c8e068b5e38d4b386eb1bf3d5cede3d0/src/mwscript/parser.rs#L186-L199 would require using Parser::parse_stream(minusplus(), i) instead.

Given that 1) this pattern seems generally desirable to support and 2) not supporting it is a breaking change, I believe we should accept the additional complexity required to support this with the new solver and not break this pattern.

@RalfJung
Copy link
Member

Hm... I see. I have to say I find that go example quite magical and wouldn't expect it to typecheck -- there's nothing actually determining the return type here, is there?

But really my main concern is ossifying implementation details of current rustc, and leaking these details into the spec. If we have a notion of "before" that becomes relevant for typeck, we should carefully document/specify how "before" is defined, because it becomes part of our stable language surface.

@lcnr
Copy link
Contributor

lcnr commented Feb 15, 2024

If we have a notion of "before" that becomes relevant for typeck, we should carefully document/specify how "before" is defined, because it becomes part of our stable language surface.

we already have this notion rn:

fn main() {
    let mut x = Default::default();
    x.sort(); // type annotations needed
    drop::<Vec<i32>>(x);
}

there's nothing actually determining the return type here, is there?

there is: it returns an async move block. That blocks awaits a Box<dyn Future>. It returns a coroutine where all types used at await-points are fully known

@traviscross
Copy link
Contributor Author

@rustbot labels -I-lang-nominated

Given that we found other paths forward and given the discussion above, this doesn't seem ripe for lang discussion at the moment, so let's unnominate for now.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 20, 2024
@golddranks
Copy link
Contributor

If we have a notion of "before" that becomes relevant for typeck, we should carefully document/specify how "before" is defined, because it becomes part of our stable language surface.

we already have this notion rn:

fn main() {
    let mut x = Default::default();
    x.sort(); // type annotations needed
    drop::<Vec<i32>>(x);
}

I've been wondering this before. I think this isn't generally well-known limitation of the type inference, people often claim that the inference works both ways and are surprised when I show them this example.

Is this documented somewhere? Does it has a name that would help searching releated issues?

Ref: Just talked about this on Reddit some weeks ago: https://www.reddit.com/r/rust/comments/1eou6re/comment/lielbao/?utm_source=share&utm_medium=mweb3x&utm_name=mweb3xcss&utm_term=1&utm_content=share_button

@lcnr
Copy link
Contributor

lcnr commented Sep 9, 2024

Is this documented somewhere? Does it has a name that would help searching releated issues?

I think not. We don't have a name or a documented list of places during type inference which are eager, big ones are:

  • method resolution
  • coercions
  • closure signature inference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants