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 all unstable placement features #48333

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

aidanhs
Copy link
Member

@aidanhs aidanhs commented Feb 18, 2018

Closes #22181, #27779. Effectively makes the assortment of placement RFCs (rust-lang/rfcs#470, rust-lang/rfcs#809, rust-lang/rfcs#1228) 'unaccepted'. It leaves box_syntax and keeps the <- token as recognised by libsyntax.


I don't know the correct process for unaccepting an unstable feature that was accepted as an RFC so...here's a PR.

Let me preface this by saying I'm not particularly happy about doing this (I know it'll be unpopular), but I think it's the most honest expression of how things stand today. I've been motivated by a post on reddit which asks when these features will be stable - the features have received little RFC-style design work since the end of 2015 (~2 years ago) and leaving them in limbo confuses people who want to know where they're up to. Without additional design work that needs to happen (see the collection of unresolved questions later in this post) they can't really get stabilised, and I think that design work would be most suited to an RFC rather than (currently mostly unused) experimental features in Rust nightly.

I have my own motivations - it's very simple to 'defeat' placement in debug mode today and I don't want a placement in Rust that a) has no guarantees to work and b) has no plan for in-place serde deserialisation.

There's a quote in [1]: "Ordinarily these uncertainties might lead to the RFC being postponed. [The RFC seems like a promising direction hence we will accept since it] will thus give us immediate experience with the design and help in determining the best final solution.". I propose that there have been enough additional uncertainties raised since then that the original direction is less promising and we should be think about the problem anew.

(a historical note: the first mention of placement (under that name - uninit pointers were earlier) in an RFC AFAIK is [0] in late 2014 (pre-1.0). RFCs since then have built on this base - [1] is a comment in Feb 2015 accepting a more conservative design of the Place* traits - this is back when serde still required aster and seemed to break every other nightly! A lot has changed since then, perhaps placement should too)


Edit: you should read the summary comment instead of the notes below.

Concrete unresolved questions include:

  • making placement work in debug mode [7]
  • making placement work for serde/with fallible creation [5], [irlo2], [8]
  • trait design:
    • opting into not consuming the placer in Placer::make_place - [2]
    • trait proliferation - [4] (+ others in that thread)
    • fallible allocation - [3], [4] (+ others in that thread)
  • support for DSTs/unsized structs (if at all) - [1], [6]

More speculative unresolved questions include:

  • better trait design with in the context of future language features [irlo1] (Q11), [irlo3]
  • interaction between custom allocators and placement [irlo3]

[0] rust-lang/rfcs#470
[1] rust-lang/rfcs#809 (comment)
[2] rust-lang/rfcs#1286
[3] rust-lang/rfcs#1315
[4] #27779 (comment)
[5] #27779 (comment)
[6] #27779 (comment)
[7] #27779 (comment)
[8] rust-lang/rfcs#1228 (comment)
[irlo1] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789
[irlo2] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789/19
[irlo3] https://internals.rust-lang.org/t/lang-team-minutes-feature-status-report-placement-in-and-box/4646

@rust-highfive
Copy link
Collaborator

r? @TimNN

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2018
@aidanhs aidanhs mentioned this pull request Feb 18, 2018
4 tasks
@aidanhs aidanhs changed the title Removes all unstable placement features Remove all unstable placement features Feb 18, 2018
@aidanhs aidanhs force-pushed the aphs-no-place-for-placement branch from 2d8c890 to 7be7e40 Compare February 18, 2018 22:19
@aidanhs
Copy link
Member Author

aidanhs commented Feb 18, 2018

Probably for the lang team? r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned TimNN Feb 18, 2018
@estebank
Copy link
Contributor

@rust-lang/lang

@estebank
Copy link
Contributor

(Doing this would also close #36611.)

@nikomatsakis
Copy link
Contributor

@aidanhs I've been wondering about the procedure to "unaccept" a feature myself on a few occasions. I guess this is as good as any. I think I'm inclined to agree that it makes sense to "back off" from supporting this right now until we have time to focus again.

@aidanhs aidanhs force-pushed the aphs-no-place-for-placement branch from 7be7e40 to 1141310 Compare February 19, 2018 17:44
@withoutboats withoutboats added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 19, 2018
@coder543
Copy link

I agree that this will be unpopular, but your reasoning is sound. Really, Rust needs to figure out the placement-new stuff. Ideally, this would be treated with the same urgency as SIMD, and it would be finished in 2018. It's a serious and confusing language gap.

@aturon
Copy link
Member

aturon commented Feb 19, 2018

Thanks @aidanhs, I think this is overdue.

@steveklabnik
Copy link
Member

I'm 👍 on this. Not sure if the old RFCs should be modified, or if we should have a new RFC (just merged) that "unaccepts" them, with a reference to it.

@Manishearth
Copy link
Member

I think we should just modify them and say "XYZ issues cropped up during the experimental period, this RFC has been unaccepted" at the top somewhere.

@nikomatsakis
Copy link
Contributor

Hmm so @Manishearth raises a good point. I think I am r+ on this change, but I would like to see a write-up summarizing some of the outstanding concerns that proved hard to resolve. I would be happy for this to be a comment on the tracking issue (which could then be closed), and/or added to the RFC. @aidanhs do you think you'd be up for gathering such a thing? It doesn't have to be a big thing, just some points so we don't start by scratching our heads and rediscovering constraints.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

I propose to merge this PR -- well, some variant of this PR. In particular, I propose that we accept @aidanhs's proposal and remove this feature for now (along with some kind of notes for the future on where the problems and open questions were).

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 21, 2018
@rfcbot
Copy link

rfcbot commented Feb 21, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@aidanhs
Copy link
Member Author

aidanhs commented Feb 22, 2018

I think I am r+ on this change, but I would like to see a write-up summarizing some of the outstanding concerns that proved hard to resolve.

How about I postpone (temporarily close) this PR and open an "unaccepting" PR on the RFCs repository instead and move the discussion over to there? On that RFC we can discuss the practicalities of "unaccepting" RFCs (deleting/adding a note at the top of unaccepted RFCs/adding a new RFC file/a combination of the previous/something else) and also make sure that the motivational text (be that an RFC itself, or just text in the PR comment) is comprehensive enough. Are you thinking something along the lines of my original issue text, but with more prose to elaborate on each item so a reader doesn't have to visit all the links?

If that sounds reasonable, I suggest cancelling the fcp and then you can fcp something more substantial on the RFC itself when it's ready. I realise that is a little annoying from the point of view of having to dig through yet more issues to find the current status of placement new, but maybe it'll help set a trend for any future "unacceptances".

Ideally, this would be treated with the same urgency as SIMD, and it would be finished in 2018. It's a serious and confusing language gap.

@coder543 I agree that it's a desirable feature, but I don't agree that we should treat it with the same urgency as SIMD. For one thing, I don't really think there's even a collective agreement on what the feature should end up being able to do (I want serde, some people want unsized, some people just want stable box syntax) and for another...we're starting the year off by moving the feature back to pre-RFC status!

Unless someone is waiting in the wings with a novel (since I'm not aware of any existing work that can easily be reused - I've mentioned before that C++ placement maps poorly onto Rust) proposal everyone agrees on almost immediately and they're willing to put in a heroic effort to implement it and get real-world feedback, I just can't see it happening - the time and hard work won't happen by itself, and I suspect a solution here is going to require a bunch of both.

I'd certainly love to be wrong though, and the first step is opening the floor again.

@aturon
Copy link
Member

aturon commented Feb 22, 2018

@aidanhs I'd rather not go through a full round of RFC to litigate this particular process concern; I think the lang team should just reach a decision about how it wants to approach this issue in situ.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Feb 22, 2018
@rfcbot
Copy link

rfcbot commented Feb 22, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 22, 2018
@nikomatsakis
Copy link
Contributor

Let's discuss at @rust-lang/lang meeting today.

@aidanhs
Copy link
Member Author

aidanhs commented Mar 28, 2018

I've appended my proposed summary comment below. Once this PR is r+ed (@nikomatsakis), I'll create the internals thread, create the comment on the tracking issue and create the RFCs PRs.

cc @alexcrichton


Edit: please read the complete summary comment instead of the draft summary.

DRAFT SUMMARY BELOW


Placement new is imminently about to be/has been removed as an unstable feature and the RFCs unaccepted. The approved/merged PR is at #48333 and the tracking issues were at #22181 and #27779. Note that this does not affect box syntax.

Find the internals thread where you can discuss this more at LINK-TODO. Please add any thoughts there.

So why remove placement?

The implementation does not fulfil the design goals

As described in rust-lang/rfcs#470 (referred to by the accepted rust-lang/rfcs#809), the implementation of placement new should

Add user-defined placement in expression (more succinctly, "an in expression"), an operator analogous to "placement new" in C++. This provides a way for a user to specify (1.) how the backing storage for some datum should be allocated, (2.) that the allocation should be ordered before the evaluation of the datum, and (3.) that the datum should preferably be stored directly into the backing storage (rather than allocating temporary storage on the stack and then copying the datum from the stack into the backing storage).

The summarised goals (from the same RFC text) are to be able to:

  1. Place an object at a specific address (references the original C++ goals)
  2. Allocate objects in arenas (references the original C++ goals)
  3. Be competitive with the implementation in C++

Now consider the description of C++ behaviour in https://isocpp.org/wiki/faq/dtors#placement-new and note that during construction, the this pointer will point to the allocated location, so that all fields are assigned directly to the allocated location. It follows that we must provide similar guarantees to achive goal 3 (be competitive with C++), so the "preferably" in the implementation description is not strong enough - it is actually necessary.

Unfortunately, it is easy to show that rust does not construct objects directly into the allocation in debug mode. This is an artificially simple case that uses a struct literal rather than the very common Rust pattern of 'return value construction' (most new functions).

It appears that the current implementation cannot competitive with C++ placement as-is. A new RFC might either propose different guarantees, or describe how the implementation should work given the very different method of construction in Rust (compared to C++). Straw man: "A call to a fn() -> T can be satisfied by a fn(&uninit T) function of the same name (allowing you to assign fields directly in the function body via the uninit reference)".

The functionality of placement is unpredictable

As described by the C++ goals for placement (mentioned above), placement is typically used because you need to have explicit control over the location an object is put at. We saw above that Rust fails in very simple cases, but even if it didn't there is a more general issue - there is no feedback to the user whether placement is actually working. For example, there is no way for a user to tell that linkedlist.back_place() <- [0u8; 10*1024*1024] is placed but linkedlist.back_place() <- [[0u8; 10*1024*1024]] is not.

Effectively, placement as implemented today is a 'slightly-better-effort to place values than normal assignment'. For an API that aims to offer additional control, this unpredictability is a significant problem. A new RFC might provide either provide clear guidance and documentation on what placement is guaranteed, or require that compilation will fail if a requested placement cannot succeed. Straw man 1: "Placement only works for arrays of bytes. Function calls (e.g. serde or anything with fallible creation) and DSTs will not work". Straw man 2: "If a same-name fn(&uninit T) does not exist for the fn() -> T call being placed, compilation will fail".

Specific unresolved questions

There are a number of specific unresolved questions around the RFC(s), but there has been effectively no design work for about 2 years. These include (some already covered above):

  • making placement work for serde/with fallible creation [5], [irlo2], [7]
  • trait design:
    • opting into not consuming the placer in Placer::make_place - [2]
    • trait proliferation - [4] (+ others in that thread)
    • fallible allocation - [3], [4] (+ others in that thread)
  • support for DSTs/unsized structs (if at all) - [1], [6]

More speculative unresolved questions include:

  • better trait design with in the context of future language features [irlo1] (Q11), [irlo3]
  • interaction between custom allocators and placement [irlo3]

[0] rust-lang/rfcs#470
[1] rust-lang/rfcs#809 (comment)
[2] rust-lang/rfcs#1286
[3] rust-lang/rfcs#1315
[4] #27779 (comment)
[5] #27779 (comment)
[6] #27779 (comment)
[7] rust-lang/rfcs#1228 (comment)
[irlo1] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789
[irlo2] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789/19
[irlo3] https://internals.rust-lang.org/t/lang-team-minutes-feature-status-report-placement-in-and-box/4646

I've opted to list these rather than going into detail, as they're generally covered comprehensively by the corresponding links. A future RFC might examine these points to identify areas to explictly address, including (in no particular order):

  • does the new RFC support DSTs? serde and fallible creation?
  • does the new RFC have a lot of traits? Is it justified?
  • can the new RFC handle cases where allocation fails? Does this align with wider language plans (if any) for fallible allocation?
  • are there upcoming/potential language features that could affect the design of the new RFC? e.g. custom allocators, NoMove, HKTs? What would the implications be?

@bors
Copy link
Contributor

bors commented Mar 29, 2018

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

@aidanhs aidanhs force-pushed the aphs-no-place-for-placement branch from abe792f to aabbba3 Compare March 29, 2018 17:55
@aidanhs
Copy link
Member Author

aidanhs commented Mar 29, 2018

Rebased

@bors
Copy link
Contributor

bors commented Apr 2, 2018

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

@SimonSapin SimonSapin force-pushed the aphs-no-place-for-placement branch from aabbba3 to 9b5859a Compare April 3, 2018 09:03
@SimonSapin
Copy link
Contributor

Oops, it looks like a PR of mine made this one unmergeable twice :( I’ve just force-pushed a rebase to your branch.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2018

📌 Commit 9b5859a has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2018
@SimonSapin
Copy link
Contributor

@bors: p=1

This PR has already bitrotted a couple times, and I’m working on more changes that should go on top of this to avoid further conflicts.

@bors
Copy link
Contributor

bors commented Apr 4, 2018

⌛ Testing commit 9b5859a with merge 199b7e2...

bors added a commit that referenced this pull request Apr 4, 2018
…sakis

Remove all unstable placement features

Closes #22181, #27779. Effectively makes the assortment of placement RFCs (rust-lang/rfcs#470, rust-lang/rfcs#809, rust-lang/rfcs#1228) 'unaccepted'. It leaves `box_syntax` and keeps the `<-` token as recognised by libsyntax.

------------------------

I don't know the correct process for unaccepting an unstable feature that was accepted as an RFC so...here's a PR.

Let me preface this by saying I'm not particularly happy about doing this (I know it'll be unpopular), but I think it's the most honest expression of how things stand today. I've been motivated by a [post on reddit](https://www.reddit.com/r/rust/comments/7wrqk2/when_will_box_and_placementin_syntax_be_stable/) which asks when these features will be stable - the features have received little RFC-style design work since the end of 2015 (~2 years ago) and leaving them in limbo confuses people who want to know where they're up to. Without additional design work that needs to happen (see the collection of unresolved questions later in this post) they can't really get stabilised, and I think that design work would be most suited to an RFC rather than (currently mostly unused) experimental features in Rust nightly.

I have my own motivations - it's very simple to 'defeat' placement in debug mode today and I don't want a placement in Rust that a) has no guarantees to work and b) has no plan for in-place serde deserialisation.

There's a quote in [1]: "Ordinarily these uncertainties might lead to the RFC being postponed. [The RFC seems like a promising direction hence we will accept since it] will thus give us immediate experience with the design and help in determining the best final solution.". I propose that there have been enough additional uncertainties raised since then that the original direction is less promising and we should be think about the problem anew.

(a historical note: the first mention of placement (under that name - uninit pointers were earlier) in an RFC AFAIK is [0] in late 2014 (pre-1.0). RFCs since then have built on this base - [1] is a comment in Feb 2015 accepting a more conservative design of the Place* traits - this is back when serde still required aster and seemed to break every other nightly! A lot has changed since then, perhaps placement should too)

------------------------

Concrete unresolved questions include:

 - making placement work in debug mode [7]
 - making placement work for serde/with fallible creation [5], [irlo2], [8]
 - trait design:
   - opting into not consuming the placer in `Placer::make_place` - [2]
   - trait proliferation - [4] (+ others in that thread)
   - fallible allocation - [3], [4] (+ others in that thread)
 - support for DSTs/unsized structs (if at all) - [1], [6]

More speculative unresolved questions include:

 - better trait design with in the context of future language features [irlo1] (Q11), [irlo3]
 - interaction between custom allocators and placement [irlo3]

[0] rust-lang/rfcs#470
[1] rust-lang/rfcs#809 (comment)
[2] rust-lang/rfcs#1286
[3] rust-lang/rfcs#1315
[4] #27779 (comment)
[5] #27779 (comment)
[6] #27779 (comment)
[7] #27779 (comment)
[8] rust-lang/rfcs#1228 (comment)
[irlo1] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789
[irlo2] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789/19
[irlo3] https://internals.rust-lang.org/t/lang-team-minutes-feature-status-report-placement-in-and-box/4646
@bors
Copy link
Contributor

bors commented Apr 4, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 199b7e2 to master...

@bors bors merged commit 9b5859a into rust-lang:master Apr 4, 2018
@kennytm-githubbot
Copy link

📣 Toolstate changed by #48333!

Tested on commit 199b7e2.
Direct link to PR: #48333

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk).

kennytm-githubbot added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Apr 4, 2018
Tested on commit rust-lang/rust@199b7e2.
Direct link to PR: <rust-lang/rust#48333>

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk).
@aidanhs aidanhs deleted the aphs-no-place-for-placement branch April 4, 2018 08:43
@est31 est31 mentioned this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

box and in expressions (tracking issue for RFC 809)