Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAssociate an allocator to boxes #50882
Conversation
rust-highfive
assigned
bluss
May 19, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @bluss (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
label
May 19, 2018
This comment has been minimized.
This comment has been minimized.
|
Note this has trivial conflicts with PR #50880, so whichever goes in first will need the other to be rebased on top of it. |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
glandium
force-pushed the
glandium:box
branch
from
096f701
to
e726409
May 19, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
glandium
force-pushed the
glandium:box
branch
from
e726409
to
ddfbc0a
May 19, 2018
This comment has been minimized.
This comment has been minimized.
|
The various UI test changes raise the interesting question whether the compiler should "hide" default generic params in error messages. |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
glandium
force-pushed the
glandium:box
branch
from
ddfbc0a
to
3359246
May 19, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
glandium
force-pushed the
glandium:box
branch
from
3359246
to
62020b2
May 19, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
glandium
force-pushed the
glandium:box
branch
from
62020b2
to
59d5866
May 20, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
glandium
force-pushed the
glandium:box
branch
from
59d5866
to
1e5328e
May 20, 2018
This comment has been minimized.
This comment has been minimized.
|
A few comments now that this is passing tests:
|
This comment has been minimized.
This comment has been minimized.
|
|
glandium
force-pushed the
glandium:box
branch
from
1e5328e
to
9aaeb4f
May 24, 2018
This comment has been minimized.
This comment has been minimized.
|
Triage ping, @bluss / @rust-lang/libs: This PR needs you review, and, I assume, a crater run & FCP. |
This comment has been minimized.
This comment has been minimized.
|
We've long wanted to do something like this but we've been hesitant to do so historically due to various compiler bugs (which indeed aren't well tracked!). From the looks of the diff here it seems that error messages related to |
This comment has been minimized.
This comment has been minimized.
|
There is a crates.io crate that does this, but it can't replace Box. It's only by implementing things in liballoc that we end up figuring out what's good and what's bad about it. See #50882 (comment). |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Sure yeah makes sense, but as a PR to rust is this expected to be merged? I'd personally still be wary of the error message segregation |
This comment has been minimized.
This comment has been minimized.
|
Re, degredation* |
This comment has been minimized.
This comment has been minimized.
|
As a PR to rust, it allows tests to run. A crater run could be useful, for instance. There are some implementation details that still need to be figured out. For example, the current patch doesn't remove the use of the box keyword, so only Box has fn new, while Box<T, A: Alloc + Default> theoretically could have it. |
glandium
force-pushed the
glandium:box
branch
from
9aaeb4f
to
d568c8c
May 30, 2018
This comment has been minimized.
This comment has been minimized.
|
Ping from triage! How should we move forward with this? |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Ok. There has to be something wrong with either the compiler or the test. |
This comment has been minimized.
This comment has been minimized.
|
Sorry for the delay in posing this! As I mentioned before we've long wanted to do this but since A number of members on @rust-lang/libs have expressed concern about moving forward in this direction (adding allocator parameters everywhere). We don't have the time to work through these concerns, though, before the 2018 edition release. As a result this will likely need to be delayed at least until after the 2018 edition release. |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton I'm perfectly find with mine being tabled for now, but this one by @glandium is much smaller, more targeted, and more researched. You mention the list of compiler issues isn't even written down. If anyone knows them, it's @glandium; they've been diligently chasing down issues in commits like 43b24c6, dfa6111, 6614fa0, 64f5233, bd8c177, and b5c2b79 as part of working towards this PR. IMO if we're ever going to prioritize this, and gather relevant data so the lang team does less work overall, we should merge this on an experimental basis with the understanding that it will be reverted at the first sign of trouble. Either we raise the bus factor, or I fear nothing will happen ever. |
This comment has been minimized.
This comment has been minimized.
|
Indeed gathering data is good! My point is that we can't amongst the libs team have a serious discussion about landing this until after the edition. |
This comment has been minimized.
This comment has been minimized.
|
I'm just still disappointed that unstable methods in an unstable create ( I'm reminded of the vision best described by @gnzlbg in rust-lang/rfcs#2480 (comment) on making |
This comment has been minimized.
This comment has been minimized.
|
I'll note that it is possible to experiment with crates today, to some extent, and I've done so with the allocator_api crate, that implements BUT "to some extent" is key. Because the liballoc exposes types rather than traits, one can't use an hypothetical That being said, this PR is a relatively small step to start experimenting with these things, and, like @Ericson2314, I don't see why this would need to wait. Well, except for the fact that, at the moment, I'm concerned that it breaks some compiler assumptions and that that would be the reason for the test failure. |
This comment has been minimized.
This comment has been minimized.
|
Yeah, it's precisely because this experiment will take so long that that it's important to unlock the parallelism of not hogging tons of lib team time. |
glandium
referenced this pull request
Jul 25, 2018
Closed
Miscompilation while working on PR #50882 #52694
This comment has been minimized.
This comment has been minimized.
|
Yes the libs team understands the value in experimentation and doesn't want to unnecessarily block efforts, but this PR is touching the |
This comment has been minimized.
This comment has been minimized.
|
Ping from triage! If I understand correctly, this PR is blocked until after the 2018 edition ships, which is still quite some time to go. As such I'm closing this PR as blocked, following our triage procedure. Please add a comment or re-open if I missunderstood. |
TimNN
closed this
Aug 7, 2018
TimNN
added
S-blocked-closed
and removed
S-waiting-on-author
labels
Aug 7, 2018
This comment has been minimized.
This comment has been minimized.
|
@glandium Do you have plans to reopen this PR now that the 2018 edition has shipped? This would be really great to have. |
This comment has been minimized.
This comment has been minimized.
|
Yes please! Happy to assist in any way once it is, too. |
This comment has been minimized.
This comment has been minimized.
|
As long as #52694 is a problem (and it is because travis uses a system llvm that is affected), I can't make progress on this. |
This comment has been minimized.
This comment has been minimized.
|
Can we |
This comment has been minimized.
This comment has been minimized.
|
Can we? |
This comment has been minimized.
This comment has been minimized.
|
So... I tried to gave a quick try to the idea, but I hit two snags:
which I don't know what to do about.
With:
I can still compile code that uses |
This comment has been minimized.
This comment has been minimized.
That apparently comes from #54383. @mikeyhew, @nikomatsakis, @eddyb, any idea how to get around this? |
This comment has been minimized.
This comment has been minimized.
|
Thanks does trying! And sorry I didn't respond earlier. I think we can make an Also did you push this experiment yet? I'd love to try it. |
This comment has been minimized.
This comment has been minimized.
In this PR, you update the definition of pub struct Box<T: ?Sized, A: Alloc = Global>(Unique<T>, A);The problem is You need to edit this code here: https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/coherence/builtin.rs#L226. There are a couple options:
The real fix will be to support dynamic dispatch for method receivers with arbitrary ABIs, but that's pretty far off. For now it's probably best to use the |
This comment has been minimized.
This comment has been minimized.
I haven't fully rebased, because I was only trying to see if box_free could be made to type-error. Considering the |
This comment has been minimized.
This comment has been minimized.
|
Ah, what a mess! The one good thing is hopefully |
This comment has been minimized.
This comment has been minimized.
|
At this point it might be easier to support extra fields in I believe such support could work if built on top of the chalk-oriented "bounded/universe" types, e.g. computing the layout of This could also be used to represent the type of the data pointer field of DSTs, without overloading the meaning of a |
This comment has been minimized.
This comment has been minimized.
|
So the good news is that #52694 is now "magically" gone. I'm currently in the process of reviving this patch in a different form, working around the |
This comment has been minimized.
This comment has been minimized.
|
Opened #58457. |
glandium commentedMay 19, 2018
•
edited
This turns
Box<T>intoBox<T, A: Alloc = Global>. This is aminimalist change to achieve this, not touching anything that could have
backwards incompatible consequences like requiring type annotations in
places where they currently aren't required,
per #50822 (comment)