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

Don't ICE when uncallable functions occur due to unsatisfiable where clauses #21203

Closed
erickt opened this issue Jan 15, 2015 · 19 comments · Fixed by #90304
Closed

Don't ICE when uncallable functions occur due to unsatisfiable where clauses #21203

erickt opened this issue Jan 15, 2015 · 19 comments · Fixed by #90304
Labels
A-typesystem Area: The type system C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@erickt
Copy link
Contributor

erickt commented Jan 15, 2015

UPDATED DESCRIPTION:

We now allow the original problem, but some corner cases are still unhandled. See comments below.

ORIGINAL DESCRIPTION:

Where clauses right now require predicates to contain type parameters, as in:

trait Foo {
    fn foo(&self);
}

struct Bar<T> {
    x: T,
}

impl<T> Foo for Bar<T> where T: Foo {
    fn foo(&self) {
        self.x.foo()
    }
}

But we cannot add constraints on non-generic types:

struct Baz { x: i32 }

impl Foo for Baz where i32: Foo {
    fn foo(&self) {
        self.x.foo()
    }
}

Will error out with:

test.rs:15:1: 19:2 error: cannot bound type `i32`, where clause bounds may only be attached to types involving type parameters
test.rs:15 impl Foo for Baz where i32: Foo {
test.rs:16     fn foo(&self) {
test.rs:17         self.x.foo()
test.rs:18     }
test.rs:19 }

In hand written code this usually won't come up, but this would be really helpful for a code generator like macro_rules! or #[derive], especially when dealing with associated types. Currently the only way to know if a type is generic or not is to manually walk it and see if any of the type paths start with the same name as a type parameter. If so, add that type to the predicate list. Instead, it would be much simpler if we could just add all the types listed in a field or enum to the predicate list and let the type checker report an error if one of those types doesn't implement the trait.

cc @nikomatsakis, @jroesch

@nikomatsakis
Copy link
Contributor

So I think this is a good reason to just permit where clauses like this. Ideally we'll report errors at the callee, though it's easiest to report errors at the caller.

@kmcallister kmcallister added the A-typesystem Area: The type system label Jan 16, 2015
@nikomatsakis
Copy link
Contributor

@erickt you seem to have disappeared from IRC, but the point about privacy is a good one -- I feel like the privacy rules are being too strict here.

@nikomatsakis
Copy link
Contributor

I'm not so sure about the privacy thing any more. After some conversation with @aturon we felt like the current rules might be best.

@Gankra
Copy link
Contributor

Gankra commented Jan 21, 2015

Should this be closed as wontfix/intended, then?

@jroesch
Copy link
Member

jroesch commented Feb 7, 2015

I lost track of this, did anyone remove the check that ensures bound types include a type parameter? I remember we had talked about shuffling around some error reporting as well, but its been too many days since I've thought about this.

@huonw
Copy link
Member

huonw commented Feb 8, 2015

@gankro I think the privacy rules are somewhat orthogonal to allowing non-generic where clauses (although the motivating use case is definitely undermined by the current privacy rules).

@steveklabnik
Copy link
Member

Triage: this now works. Full reproduction:

trait Foo {
    fn foo(&self);
}

impl Foo for i32 {
    fn foo(&self) { println!("success {}", self); }
}

struct Baz { x: i32 }

impl Foo for Baz where i32: Foo {
    fn foo(&self) {
        self.x.foo()
    }
}

fn main() {
    let b = Baz { x: 5 };
    b.foo();
}

@nikomatsakis
Copy link
Contributor

Huh. When did that happen? I know there have been some pending PRs about this, but there were some thorny issues, and I didn't realize that we'd made any actual changes here. @arielb1, do you know?

@arielb1
Copy link
Contributor

arielb1 commented Mar 8, 2016

What? I didn't implement this?

@arielb1 arielb1 reopened this Mar 8, 2016
@mitaa
Copy link
Contributor

mitaa commented Mar 8, 2016

I think this is the relevant commit: dbf994b

@nikomatsakis
Copy link
Contributor

Yeah I was wondering if this was somehow fallout from that change. That's a bit unexpected though.

@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2016
@nikomatsakis
Copy link
Contributor

So we do need a solution that addresses this example raised by @arielb1 👍

#[no_mangle]
pub fn bogus<'a>(x: &'a mut ()) where &'a mut (): Clone {
    <&'a mut ()>::clone(&x);
}

The key point in this example is that this where clause could never hold -- but because it's only parametric over a lifetime, we will try to trans it, and find that there is no existing impl. This could either fail in typeck -- but we'd have to search for impls using an erased parameter -- or else fail in trans -- where the absence of an impl could cause us to emit a trap or something like that, on the premise that the fn should not in fact be callable.

Basically in trans we can just check if we know statically that the fn could never be called and generate a trap or what have you. Or just don't translate it. Or something. This seems similar to the logic that we use to screen things out of vtables.

@nikomatsakis nikomatsakis changed the title Allow where clause predicates that contain no type parameters Don't ICE when uncallable functions occur due to unsatisfiable where clauses Mar 10, 2016
@nikomatsakis
Copy link
Contributor

triage: P-medium

@nrc
Copy link
Member

nrc commented Mar 17, 2016

triage: P-medium

1 similar comment
@nikomatsakis
Copy link
Contributor

triage: P-medium

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Mar 17, 2016
@brson brson added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Oct 20, 2016
@brson
Copy link
Contributor

brson commented Oct 20, 2016

P-low?

@steveklabnik
Copy link
Member

Triage: @nikomatsakis 's sample compiles, but if you try to use it, can never work. Aka, this is still an issue, as far as I can tell.

@vandenheuvel
Copy link
Contributor

#75961 is a duplicate of @nikomatsakis 's example. That issue contains more recent diagnostics: the example no longer compiles.

@vandenheuvel
Copy link
Contributor

The original issue now compiles with the trivial bounds feature, see #48214.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority 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.