Skip to content

Explicit OIBIT impls hide the default impls #27554

Description

@wthrowe
#![allow(dead_code)]

trait Foo {}
struct A<T>(T);
unsafe impl<T: Foo> Sync for A<T> {} // Comment this line and the code compiles

trait IsSync: Sync {}
struct X;
impl IsSync for A<X> {} // error: the trait `Foo` is not implemented for the type `X` [E0277]

fn main() {}

Nothing in this example opts out of Sync or contains a type that opts out, so everything should be Sync.

I suspect this is the same underlying issue as #23072, but unlike in that case this seems like something that should be allowed.

Activity

  1. wthrowe commented on Aug 10, 2015

    @wthrowe
    ContributorAuthor

    I've been experimenting with ways to fix this, and, oddly, there's a unit test checking that this doesn't work: https://github.com/rust-lang/rust/blob/master/src/test/compile-fail/typeck-default-trait-impl-precedence.rs

    The OIBIT RFC seems pretty clear that this is supposed to be accepted, but I know that the rules specified there for negative impls are thought to be too lenient, so maybe this part is also frowned upon?

  2. added
    T-langRelevant to the language team
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on Oct 7, 2015
  3. nikomatsakis commented on Oct 8, 2015

    @nikomatsakis
    Contributor

    As explained in #23072, this was the semantics we implemented, which diverged somewhat from the RFC. It's interesting that this can come about without feature gates (though obvious in retrospect).

  4. nikomatsakis commented on Oct 14, 2015

    @nikomatsakis
    Contributor

    triage: P-medium

    We discussed this last week in the language subteam meeting. Our conclusion was that there is (potentially) an issue here where the desired semantics are not entirely clear. It is backwards incompatible to fix it but deemed low risk, because the bad scenario is when one is trying to "add to" the default set that obey a particular trait (or remove further from the negative set, I suppose).

    Some notes from our discussion:

    The intent of an impl like this is somewhat unclear. Why did one write the impl in the first place? Was the goal to cover a case that the default rules would have excluded? Or was it perhaps to narrow down the default rules to a smaller set of acceptable cases (which kind of "opts out" by "opting in")? The latter is the current semantics; it does seem plausible that a naive read of the code might think that the impl was listing out the cases that are Sync, versus adding to an implicit set. A short-term fix we might use is to try and report errors if the impl is "unnecessary". Overall though our conclusion was the current semantics ought to be revisited with specialization in mind, since there is a lot of overlap (no pun intended!) between the situation here and that of specialization.

  5. brson commented on Jul 14, 2016

    @brson
    Contributor

    Nominating for retriage and updates. Old type system issue.

  6. arielb1 commented on Jul 17, 2016

    @arielb1
    Contributor

    Not I-wrong.

  7. nikomatsakis commented on Jul 18, 2016

    @nikomatsakis
    Contributor

    I believe that there is no bug here: the compiler is doing the right thing, essentially. I've been meaning for some time to write-up an amendment to the OIBIT (now: auto trait) RFC to talk about it for some time. My current thoughts (more-or-less) are embedded in this gist, which also covers the connection to negative reasoning.

  8. removed
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on Jul 21, 2016
  9. nikomatsakis commented on Aug 4, 2016

    @nikomatsakis
    Contributor

    Going to close this issue -- this is the desired semantics -- but we do need to write an amendment to the RFC, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions