-
Notifications
You must be signed in to change notification settings - Fork 488
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
const
expression can borrow static items
#1610
base: master
Are you sure you want to change the base?
const
expression can borrow static items
#1610
Conversation
const
expression can borrow static items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update https://github.com/rust-lang/reference/blob/master/src/items/static-items.md which mentions this isn't possible?
I'm wondering if this might be confusion (and perhaps my understanding is missing) with regards to what counts as a borrow. IIUC, statics have an implied &
, is that correct? For example, if you see const C: i32 = SOME_STATIC;
, there's no obvious borrow there.
3d0233b
to
75faa87
Compare
@ehuss I updated the wording to include simply "use of statics". I have updated |
We probably want to update the
Since it'd now make sense for that to also address the fact that constants may refer to statics with some limitations. |
Also in
...that seems to need to change also. |
More generally, it seems what we need to do here is to loosen the various places in the Reference that have these restrictions, and in loosening them, add the necessary caveats, e.g. with respect to |
There's no borrow at all there, obvious or otherwise. MIR happens to represent |
src/items/constant-items.md
Outdated
## Use and reference to `static` items | ||
|
||
When a constant item or constant block is defined, [`static` items] can be used, borrowed or taken address of. | ||
By extension, you are allowed to call methods that immutably borrows the `static` items as receivers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this should definitely explain that it is not allowed to read from or write to any mutable static (static mut
and static !Freeze
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we already have a link into the reference to explain the static !Freeze
items? It makes sense to me to make a reference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is largely missing. I guess I have to expand this section in const_eval.md
. I can do that.
That's a somewhat nonsensical statement as constants are values, they don't have an address. |
I opened #1624 to fix the part about "address of a constant". |
@rustbot ready
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anywhere that mentions the extern static
restrictions mentioned in the stabilization report?
@ehuss I added a section on |
a2dde9c
to
8f8cf46
Compare
8f8cf46
to
97abd04
Compare
I pushed a commit to simplify things. After reading more carefully, I was feeling like this was duplicating content that is already specified in the const_eval chapter. I'm reluctant to duplicate things since it makes it more challenging to maintain. I'm also a little reluctant to add deeper "exploration" style content which is essentially restating other rules. There is more specific reasoning in the commit message. If you disagree or think there is something that is now not documented, please let me know! I realize this removes a lot of content from this PR, and I apologize for that. I probably should have been paying more attention earlier on and provided some guidance. As for having more examples that illustrate specific const-eval limitations, I expect those to be added later over time (perhaps through the test suite links). @traviscross Would you be willing to give this a final look? I think this is ready to go from my perspective. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! ❤️
src/items/static-items.md
Outdated
* The type must have the `Sync` trait bound to allow thread-safe access. | ||
* Constants cannot refer to statics. | ||
All access to a static is safe, | ||
provided that the type must have the `Sync` trait bound to allow thread-safe access. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it sound like a !Sync
static would be unsafe to access. But actually it's just rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have rewritten this to use more direct language. I'm not sure why the original was written to say "it is safe to ...", since there isn't anything to imply that it was unsafe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the wording, to mention that a type with !Sync
is rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I picked the changeset from @ehuss actually. I think those wordings are better.
src/const_eval.md
Outdated
@@ -23,7 +23,8 @@ to be run. | |||
* [Const parameters]. | |||
* [Paths] to [functions] and [constants]. | |||
Recursively defining constants is not allowed. | |||
* Paths to [statics]. These are only allowed within the initializer of a static. | |||
* Paths to immutable [statics]. | |||
* Reads of [`extern` statics] are not allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reads from and writes to static mut
and static
with interior mutability are also not allowed.
And then there are restrictions on references that escape into the final value of the const/static. More precisely, this is about expressions that have been subject to lifetime extension in the top-level scope of a const/static initializer. Those references must also be of type &T
and point to a value without an UnsafeCell
.
And finally, specifically for const
there also can't be any references to anything mutable in the final value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm trying to unpack what you are saying here and align that with the current rules.
Reads from and writes to
static mut
…
Isn't this implied by the fact that you are not allowed to refer to static mut
at all by the rule that says paths only to immutable statics are allowed?
Reads from and writes to [..]
static
with interior mutability are also not allowed.
Thanks, I have added that.
And then there are restrictions on references that escape into the final value of the const/static.
Isn't this specified in the later rule that limits borrows of UnsafeCell
that are transient? (That was my reading of it, and why I didn't say it explicitly.)
And finally, specifically for
const
there also can't be any references to anything mutable in the final value.
Thanks, I have added that to the constant-items chapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually have found the points mentioned down in the list here, in a bullet point starting with All forms of [borrow]s, including raw borrows, with one limitation ...
. I think we have it covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this implied by the fact that you are not allowed to refer to static mut at all by the rule that says paths only to immutable statics are allowed?
That's not correct, referring to static mut is permitted.
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=7519fded08041a618e0b57c710ab6c25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this specified in the later rule that limits borrows of UnsafeCell that are transient? (That was my reading of it, and why I didn't say it explicitly.)
Ah right, I forgot that those are already documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not correct, referring to static mut is permitted.
OK, then I go back to my comment earlier about why the word "immutable" is being added here.
I would think this PR shouldn't add the word immutable, and the edit @dingxiangfei2009 just added about not being allowed to read or write from static mut
should cover everything?
(Because as written now, it says you can only do paths to immutable statics, but that does not seem to be true based on the playground above.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you are right, immutable
should not be mentioned here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be the last edit, do you agree?
src/const_eval.md
Outdated
@@ -23,7 +23,10 @@ to be run. | |||
* [Const parameters]. | |||
* [Paths] to [functions] and [constants]. | |||
Recursively defining constants is not allowed. | |||
* Paths to [statics]. These are only allowed within the initializer of a static. | |||
* Paths to immutable [statics] with these exception with these restrictions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Paths to immutable [statics] with these exception with these restrictions. | |
* Paths to [statics] with the following restrictions: |
src/const_eval.md
Outdated
* Paths to immutable [statics] with these exception with these restrictions. | ||
* Reads out of and writes into [`extern` statics] are not allowed. | ||
* Reads out of and writes into either a `static` with data equipped with interior mutability, | ||
or a whole or parts of `static mut`, are not allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by the wording here. Why does static mut
emphasize this "whole or part of", but interior mutable static
does not? The two are treated exactly the same so they should also be described the same.
The actual underlying check is simply "is this global mutable memory -- if yes, reject read", and "if this memory global -- if yes, reject write".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code accepted?
static FOO: (u8, UnsafeCell<u8>) = (0, UnsafeCell::new(1));
const BAR: u8 = FOO.0;
Edit: Checked and indeed it is not. So yeah, the text shouldn't differ in wording between the two.
67f4754
to
e8c43a6
Compare
@rustbot author We reviewed this in the call today. This still needs the edits about to cover the case of accesses to mutable statics being allowed. |
src/const_eval.md
Outdated
@@ -23,7 +23,10 @@ to be run. | |||
* [Const parameters]. | |||
* [Paths] to [functions] and [constants]. | |||
Recursively defining constants is not allowed. | |||
* Paths to [statics]. These are only allowed within the initializer of a static. | |||
* Paths to immutable [statics] with these restrictions and observations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might still need to be updated, since it allows static mut
.
* Paths to immutable [statics] with these restrictions and observations. | |
* Paths to [statics] with these restrictions and observations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
36c84b0
to
86e6f2e
Compare
@rustbot ready
|
@RalfJung How do you feel about the final wording here? |
src/const_eval.md
Outdated
@@ -42,7 +42,10 @@ r[const-eval.const-expr.path-item] | |||
Recursively defining constants is not allowed. | |||
|
|||
r[const-eval.const-expr.path-static] | |||
* Paths to [statics]. These are only allowed within the initializer of a static. | |||
* Paths to [statics] with these restrictions and observations. | |||
* In particular, reads and writes to any `static`, `static mut` or [`extern` statics] is not allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reads from immutable statics are allowed.
src/const_eval.md
Outdated
* Immutable borrows and pointers into immutable part of a `static` are allowed and observes the same restriction | ||
on all other forms of [borrow]s as mentioned below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't quite parse for me. I'm also not sure what the point of this bullet is -- is there any reason to not just entirely remove it?
51f2499
to
4a6ac9c
Compare
@@ -42,7 +42,8 @@ r[const-eval.const-expr.path-item] | |||
Recursively defining constants is not allowed. | |||
|
|||
r[const-eval.const-expr.path-static] | |||
* Paths to [statics]. These are only allowed within the initializer of a static. | |||
* Paths to [statics] with these restrictions and observations. | |||
* In particular, reads and writes to any `static mut` or [`extern` statics] is not allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So over in #1657, there is proposed language:
Static initializers may refer to and even read from other statics.
When reading from mutable statics, they read the initial value of that static.
The language in this PR seems contradictory to the language in that one, no? Or is there a subtle distinction we're making? Do we need some carve-out here?
(We talked this through on the lang-docs call and couldn't immediately work it out.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this already works on stable
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=59b79563db4fe5d5df26ab3f27cd4f1c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung Can you write down exactly what we should say here? It would be helpful since we don't know exactly what the rules are. From looking at the implementation, I'm guessing, is it the CanAccessMutGlobal
setting, which appears to be set only when in a static initializer? Does this need to say that you cannot read from static mut
unless inside a static initializer, and that you can never write to a static mut
, and that reading and writing extern
statics always disallowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes:
- reads from mutable statics (
static mut
, and interior mutablestatic
) are forbidden except when evaluating the initializer expression of astatic
/static mut
(so to be clear, the read can be syntactically inside aconst fn
or not, what matters is the const context, i.e. whether this is computing the initial value of a static or not) - writes to mutable statics are forbidden everywhere
- reads from and writes to extern static are forbidden everywhere
Tracked by rust-lang/rust#119618
Stabilization report: rust-lang/rust#128183
This PR updates the documentation to mention that now
static
items can be used for borrowing, explicitly or implicitly, which permits general use of them inconst
context.