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

Remove variances from traits and deprecate PhantomFn/MarkerTrait #23938

Merged
merged 5 commits into from
Apr 3, 2015

Conversation

nikomatsakis
Copy link
Contributor

There are still some remnants we could remove from the compiler (e.g. references to "subtraitrefs"; traits still have variance entries in the variance table), but this removes all user-visible bits I believe.

r? @pnkfelix

Fixes #22806 (since such traits would no longer exist)

@pnkfelix
Copy link
Member

pnkfelix commented Apr 1, 2015

anyway I'm still going to approve the PR

(update: there was once a note on a commit above about the deprecation strategy and whether we would be restricted from "undeprecating" these traits in the future. But I eventually realized that there is a fundamental difference between an item marked both #[unstable] #[deprecated] (where I think we can freely toggle the deprecation) and an item marked #[deprecated] with no unstable (which implies this was once #[stable] and thus can never adopt a different semantics nor transition back to #[unstable]).

@pnkfelix
Copy link
Member

pnkfelix commented Apr 1, 2015

@bors r+ d580d1b

@nikomatsakis
Copy link
Contributor Author

@bors r=pnkfelix e24f968

@nikomatsakis
Copy link
Contributor Author

Update: @alexcrichton encountered some errors combining this PR with the ty_relate code introduced by #23895.

@nikomatsakis
Copy link
Contributor Author

I have to look more deeply, but perhaps that suggests variance on traits is more useful than originally anticipated -- particularly around lifetime parameters.

@alexcrichton
Copy link
Member

@bors: r- (due to bad interaction with #23895)

@bors
Copy link
Contributor

bors commented Apr 2, 2015

☔ The latest upstream changes (presumably #23955) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor Author

OK, I rebased and fixed the bad interaction.

@nikomatsakis
Copy link
Contributor Author

Feeling a wee bit undecided about whether to take this patch, though I still lean in favor.

@nikomatsakis
Copy link
Contributor Author

@bors r=pnkfelix c2dba85

@nikomatsakis
Copy link
Contributor Author

Decided to go through with this.

@nikomatsakis
Copy link
Contributor Author

Basic explanation of reasoning:

  1. Still no concrete example.
  2. Current system is somewhat uneven -- you can get variance, but you really have to understand the rules. On the other hand, the price of MarkerTrait + PhantomFn is imposed on all users, and most people don't really grok what is necessary.
  3. In particular, Fn traits are invariant, which is not good, since this is the poster child for variance.
  4. Always possible, if awkward, to workaround:
    • either with HRTB or,
    • by introducing a wrapper (i.e., if you need a Foo<'a> trait where 'a can be made shorter, then you can make a FooAdapter<'a,'b> where 'a:'b type that implements Foo<'b> given a Foo<'a>.

This adds up to: better to address trait variance more comprehensively in future and make Fn and friends opt-in.

@bors
Copy link
Contributor

bors commented Apr 2, 2015

⌛ Testing commit c2dba85 with merge a167956...

@bors
Copy link
Contributor

bors commented Apr 2, 2015

💔 Test failed - auto-linux-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

On Thu, Apr 2, 2015 at 3:31 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-linux-64-nopt-t/builds/4301


Reply to this email directly or view it on GitHub
#23938 (comment).

@nikomatsakis
Copy link
Contributor Author

@bors p=1

giving high priority because this is a backwards compat thing

@bors
Copy link
Contributor

bors commented Apr 3, 2015

⌛ Testing commit c2dba85 with merge c14eb7c...

@bors
Copy link
Contributor

bors commented Apr 3, 2015

💔 Test failed - auto-mac-64-nopt-t

@pnkfelix
Copy link
Member

pnkfelix commented Apr 3, 2015

@bors retry

@pnkfelix
Copy link
Member

pnkfelix commented Apr 3, 2015

@bors force

@bors
Copy link
Contributor

bors commented Apr 3, 2015

⌛ Testing commit c2dba85 with merge 2615106...

bors added a commit that referenced this pull request Apr 3, 2015
There are still some remnants we could remove from the compiler (e.g. references to "subtraitrefs"; traits still have variance entries in the variance table), but this removes all user-visible bits I believe.

r? @pnkfelix 

Fixes #22806 (since such traits would no longer exist)
@pnkfelix
Copy link
Member

pnkfelix commented Apr 3, 2015

(I forced the retry in the hopes that the build products from the previous attempt would still be usable, but perhaps I was incorrect about that, since I think normally bors writes some notes here when it has discovered it can reuse build products from a previous attempt.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite recursion with a defaulted, contravariant trait
4 participants