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

Amend #911 const-fn to allow unsafe const functions #1245

Merged
merged 1 commit into from Oct 9, 2015

Conversation

thepowersgang
Copy link
Contributor

See discussion in #1207

@thepowersgang
Copy link
Contributor Author

I have implemented this in a branch (won't PR until this RFC is accepted). Link

Appears to work properly with just a simple change to the parser.

@Ericson2314
Copy link
Contributor

Yay!

For motivation: The Option in https://github.com/Kimundi/lazy-static.rs/blob/master/src/lib.rs#L101 is unnecessary but for this, seeing that the Once guards against invalid reads were std::mem::uninitialized() to be used to initialize the static instead of None.

@thepowersgang
Copy link
Contributor Author

Other motivation is NonZero and Unique, which underlie most collections. Having these be able to be const allows empty collections to be compile-time constructed.

@eddyb
Copy link
Member

eddyb commented Aug 12, 2015

@Ericson2314 std::mem::uninitialized() can't be used, as all constants have to be valid (pure) values.
Or it could be, with a compiler error if it ended up anywhere other than a static.

@glaebhoerl
Copy link
Contributor

I think it should be made clear somewhere that unsafe const fn means a function which may lead to undefined behavior in your program at runtime if used improperly, as opposed to a function which may lead to undefined behavior e.g. segfaults at compile-time, which would of course be radically insecure. This is perhaps obvious, but when I see "unsafe const fn", that's the first thing I think of.

@eddyb
Copy link
Member

eddyb commented Aug 12, 2015

@glaebhoerl constants are pure right now, so unsafety at compile-time is out of the question.
However, I do agree that it could be made clearer in the RFC text itself.

```rust
struct OptionalInt(u32);
impl OptionalInt {
/// Value must be non-zero
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't NonZero::new_unchecked be a better example here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, however Unique is more directly related to the collection types (as that's the libcore type they're built on)

@Ericson2314
Copy link
Contributor

@eddyb Oops, I've discussed this on IRC, probably with you. For the short term, s/uninitialized/zeroed/g then.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 13, 2015
@eddyb
Copy link
Member

eddyb commented Aug 13, 2015

@Ericson2314 That's still problematic, what would it do for &T?

@Ericson2314
Copy link
Contributor

Sandbox compile time evaluation. For this case, I think it would suffice to keep an extra "taint flag" on pointer values, and not allow dereferencing tainted ones.

@aturon aturon self-assigned this Aug 13, 2015
@thepowersgang
Copy link
Contributor Author

@glaebhoerl I would assume that's implicit in the idea of CTFE... but it may require explicit stating in the RFC.
@aturon Any blockers from you for this being accepted?

@codyps
Copy link

codyps commented Sep 8, 2015

I'd like to note that in the common usage of rust via cargo with build scripts, or nightly rust using plugins, we already have potentially undefined things occurring at compile time.

If there is a goal to avoid particular types of undefined things at compile time, but not all of them, within the rust project there probably should be some explicit note (somewhere, not necessarily this RFC) about what the goals are around undefined things occurring at compile time.

@eddyb @glaebhoerl

Edit: I suppose it might just be "avoid vulnerabilities in play.rust-lang.org"

@aturon
Copy link
Member

aturon commented Sep 15, 2015

@thepowersgang Sorry for the long silence here! I'm working my way through RFC backlog.

I personally don't see any problem with this RFC. I will propose it for "final comment period" in the next Lang Team meeting.

@aturon aturon added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Sep 21, 2015
@aturon
Copy link
Member

aturon commented Sep 21, 2015

This RFC is entering its final comment period.

@petrochenkov
Copy link
Contributor

Names returned from the string interner in libsyntax can be made NonZero ifwhen this RFC is accepted

struct OptionalInt(u32);
impl OptionalInt {
/// Value must be non-zero
unsafe const fn new(val: u32) -> OptionalInt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since const is not a part of function type and unsafe is, I'd prefer this to be const unsafe fn and not unsafe const fn

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“unsafe constant function” sounds more “right” to me than “constant unsafe function”, though your point is also valid.

Either way, I think it should be possible to put these in any order. I’ve already had some trouble remembering which goes first in pub extern fn x and now I’ll have to think what order pub, const and unsafe go in pub unsafe const fn x. pub is pretty easy since it “always goes first”, but there’s no such obvious rule for unsafe and const.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 @petrochenkov's order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer unsafe const fn because I see const fn as something different from fn, while unsafe is a mere qualifier.
There's no real reason why there aren't const fn() pointer types, they just weren't implemented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for @nagisa: allow both orders
There's no good reason to force users to learn a particular order.

If we decide on a preferred order, that can still be enforced by rustfmt. No need to make rustc overly strict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... both could be allowed, but would complicate the parser (if it is to correctly disallow unsafe const unsafe fn). I'll yeild to the language team on the final decision, but I'm leaning towards keeping a fixed order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @petrochenkov: unsafe fn feels like a more fundamental concept/thing than const fn. (const on a function doesn't outlaw it from being used as a non-const function, while unsafe does outlaw it from being used as a non-unsafe one.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddyb I see how a const fn ptr could allow for higher order functions at compile-time, I am not sure that is the right means to achieve that.

@nikomatsakis
Copy link
Contributor

It's official. The language design subteam has decided to accept this RFC. We did not, however, resolve the burning question of modifier order. We'll have to figure that out next week.

aturon added a commit that referenced this pull request Oct 9, 2015
Amend #911 const-fn to allow unsafe const functions
@aturon aturon merged commit f430e8b into rust-lang:master Oct 9, 2015
@aturon
Copy link
Member

aturon commented Oct 9, 2015

This RFC has been merged.

The lang team met and discussed this RFC again, and decided that:

  • In general, the current enforcement of ordering between fn attributes is a bit odd, and is probably better left to a lint and/or rustfmt. But that should be tackled in a separate RFC.
  • For now, we prefer the enforced order pub const unsafe fn. (It's effectively a tossup.)

And, as @nikomatsakis said already, in general we are in favor of the RFC.

@aturon aturon removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Oct 9, 2015
@glaebhoerl
Copy link
Contributor

@jmesmon (late response, I've only just gathered some thoughts)

I'd like to note that in the common usage of rust via cargo with build scripts, or nightly rust using plugins, we already have potentially undefined things occurring at compile time.

If there is a goal to avoid particular types of undefined things at compile time, but not all of them, within the rust project there probably should be some explicit note (somewhere, not necessarily this RFC) about what the goals are around undefined things occurring at compile time.

@eddyb @glaebhoerl

Edit: I suppose it might just be "avoid vulnerabilities in play.rust-lang.org"

That's the down-to-earth view of it, yeah. But as an example of why this would bother me more deeply, consider that rustc is written in Rust, a language which intends to guarantee that programs written in it have well-defined, memory safe behavior for all possible inputs. Certain inputs (source files) causing rustc to exhibit undefined behavior would then be logically equivalent to rustc containing at least one unsafe block which is incorrect ("not actually safe").

The examples of cargo scripts and especially compiler plugins are similar in a way, but feel qualitatively different to me (perhaps it's the distinction between arbitrary and undefined).

@Parakleta
Copy link

It would be ideal if this change could be applied to transmute and const-ness applied to the functions/methods that rely on it (such as as_ptr etc.) so that it would be possible to more freely cast static and constant values. Between this and #1187 it would then be possible to solve issue #400 with a simple macro.

@eddyb
Copy link
Member

eddyb commented Oct 27, 2015

@Parakleta That's pretty difficult, as it requires representing values as polysemantic bags of bits (for example, constant references right now are only the value they point to, they have no address associated with them until runtime).
And it would have to be partial, i.e. certain values are poisoned and attempting to use them in certain ways (dereferencing a raw pointer created from an integer, for example) would cause a compilation error.

Our const evaluation is seriously lacking even without this massive increase in complexity you are proposing.

@Parakleta
Copy link

I don't understand why dereferencing a pointer created from an integer through transmute should cause a compilation error. This would be exactly the kind of thing I'd want to be able to do with a const unsafe fn transmute.

@eddyb
Copy link
Member

eddyb commented Oct 27, 2015

@Parakleta How would you implement it? Memory doesn't exist at compile-time.
Something like &0 as *const i32 as usize may not even be constant in the executable binary, with ASLR it will have to be patched when loaded or computed on the spot (relative to the instruction pointer).

@Parakleta
Copy link

Managed memory doesn't, since you expect the runtime to make it for you when the program is launched, but unmanaged memory does. Specifically look at the memory mapped registers of any MCU. I want something like const I2C_ADDR: *mut u8 = transmute(0x0020);

@Parakleta
Copy link

I just realised we're talking about slightly different things. In your example taking the address of a constant integer wouldn't work, it would need to force the integer to be static, and then taking the address is fine.

So &'static 0 as *const i32 as usize would probably be meaningless, but still valid.

@eddyb
Copy link
Member

eddyb commented Oct 27, 2015

You already have const I2C_ADDR: *mut u8 = 0x0020 as *mut u8;, don't you?

And no, the concept of memory does not exist at compile-time. Managed or unmanaged.
Memory is instantiated at run-time. The memory space is a run-time concept.

At compile-time you can only deal with safe symbolic references, mutable variables at most (as long as you preserve "execution order").

Once you introduce transmutes, you have to track every bit and where it comes from, and somehow allow reconstructing symbolic values from the shattered bits of a different symbolic value (in this context, a safe reference).

Constructing symbolic references from constant integers would be impossible, as there is no symbolic information associated to any constant integer address.

@Parakleta
Copy link

You're right, 0x0020 as *mut u8; does work. Somehow I missed that.

I'm not really following your argument too well, the point I'm stuck at is just that I cannot get a pointer to the data within a &'static str and I cannot convert a [u8] to an [i8] at compile time because both of these operations require transmute. I had assumed transmute was just the same as using a union cast in C which I can do at compile time. Surely a transmute at compile time just grabs the bits you feed in and pretends they belong to some other type, so that then they can be interpreted as that other type. I thought it was a no-op except from the perspective of type checking.

@eddyb
Copy link
Member

eddyb commented Oct 27, 2015

The bits of a reference do not exist at compile-time, as the address is decided at link or load time.

Can you dereference arbitrary pointers or perform arithmetic on addresses of statics in C constant expressions (or C++11 constexpr)?

@dgrunwald
Copy link
Contributor

When using the Python C API, it's normal to cast function pointers within static initializers:

static PyMethodDef cc_methods[] = {
    {"varargs", function_with_varargs, METH_VARARGS, NULL},
    {"keywords", (PyCFunction)function_with_kwargs, METH_KEYWORDS, NULL},
    {0}
};

struct PyMethodDef expects type PyCFunction, but elements with flag METH_KEYWORDS are actually of type PyCFunctionWithKeywords.

Currently there doesn't seem to be any way to do the same operation in Rust; for rust-cpython I've had to use static mut and patch in the function pointer at runtime :(

@Parakleta
Copy link

The bits of a reference do not, but the type does and at compile time I can change the type that those bits are intended to represent even if the bits are still unknown. Arithmetic on the addresses of statics is done to some extent at link time to address into arrays and structs, although I don't know how general purpose this is, maybe just offsets.

I think I understand now that your concern is that transmute would make &0 as *const i32 as usize possible despite it being declared illegal in RFC 911 (for good reason). Your use of the word 'poisoning' makes sense to me now. Essentially a distinction needs to be made between compile time constants and link time constants.

@eddyb
Copy link
Member

eddyb commented Oct 27, 2015

@dgrunwald That's one of those cases that makes me sad we didn't go with some combination of reference/pointer to fn instead of having fn itself be the pointer.
Because casting *const fn(i32) to *const fn() would just work (it would also provide nullability without Option being involved).

Technically, we could support the cast with the current fn pointer types as long as the resulting type is unsafe, preventing a bad call in safe code.

@Centril Centril added A-unsafe Unsafe related proposals & ideas A-const-eval Proposals relating to compile time evaluation (CTFE). labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Proposals relating to compile time evaluation (CTFE). A-unsafe Unsafe related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet