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

Stabilize const_int_ops integer methods #51364

Closed
wants to merge 1 commit into from

Conversation

faern
Copy link
Contributor

@faern faern commented Jun 5, 2018

Fixes #51267 (tracking issue)
Stabilizing the methods made const fns in #51299

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 Jun 5, 2018
@oli-obk oli-obk added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 5, 2018
@nikomatsakis
Copy link
Contributor

Hmm, cc @oli-obk and @eddyb — I am mildly reluctant here on the basis of our ongoing discussion about just how to handle const fn when it comes to promotion and so forth. Thoughts? Should we put a hold on const fn stabilization?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 5, 2018

These const fns in particular are very simple numerical const fns. I was just about to write that these are totally fine, because they have no failure cases. Any integral input will result in a valid value...

And then realized that if you casted a reference to a pointer and then to a usize and then applied these functions, const eval would abort with an error, because you can't swap bytes of an abstract pointer, or count the ones in it. And we also cannot provide a value that will be exactly what you'd get at compile-time, because we don't know the compile-time address.

This is exactly the same issue as comparing any value derived from an address.

It is kind of fine to stabilize them because there's no way to obtain the address of anything as a value of integral type in promoteds (unions are forbidden, casting pointers to usize is forbidden). Outside of promoteds it's also fine, because you need to change code in order to get a compilation failure.

That said, we definitely need regression tests that try very hard to use these on addresses in promoteds .

@eddyb
Copy link
Member

eddyb commented Jun 5, 2018

cc @rust-lang/libs These functions were made const fn days ago.
I'm not sure if we have a process here, but this needs at least (the time of) FCP, right?

@faern
Copy link
Contributor Author

faern commented Jun 5, 2018

I might have submitted the stabilization PR too early. It's my first PR on stabilization of anything so I'm not sure what the process is. I got the impression that stabilizing const fns was fairly uncontroversial so I felt I could try. Feel free to educate me on how to proceed from here :)

@nikomatsakis
Copy link
Contributor

@oli-obk I have to say that this sort of ad-hoc logic makes me uncomfortable. =) That is, it feels easy for us to overlook some conditions or be a bit surprised... maybe we just need to commit to const fns exposing their bodies somehow when it comes to promotion. That said, didn't we say that we were going to have some special attribute that would allow us to promote const fns for now? In that case, I would worry a lot less.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 6, 2018

Yes, I haven't gotten around to writing that feature gate yet.

My post might have come off too much in support of stabilizing. I do worry about these const fns because in contrast to the other stable const fns, these ones do more than just create structs

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 8, 2018

I am personally opposed to stabilizing any more const fn until we have some way to limit which const fn will potentially be promoted. (cc @rust-lang/libs)

(Note to @faern — I don't want you to interpret this as a rebuke. It's just that we recently found some soundness issues around the way the compiler was handling const fn promotion, and so I want to tread carefully!)

@SimonSapin
Copy link
Contributor

@nikomatsakis What does promotion mean in this context?

@hanna-kruppe
Copy link
Contributor

Promotion of values to 'static, i.e., allowing let x: &'static _ = &<some expression>; to compile.

@faern
Copy link
Contributor Author

faern commented Jun 10, 2018

@nikomatsakis Fully understood! I don't interpret any of this as a rebuke towards me. Even though I would want this feature, I much more want the compiler/language to be sound :)

@pietroalbini pietroalbini added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 18, 2018
@pietroalbini
Copy link
Member

Ping from triage! What's the status of this PR?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 18, 2018

blocked on #51570

@pietroalbini pietroalbini added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2018
@TimNN TimNN added A-allocators Area: Custom and system allocators and removed A-allocators Area: Custom and system allocators labels Jun 26, 2018
@bors
Copy link
Contributor

bors commented Jun 30, 2018

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

@pietroalbini pietroalbini added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 9, 2018
@kennytm
Copy link
Member

kennytm commented Jul 31, 2018

@oli-obk Since #51570 has been closed, what should be done to this PR?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 1, 2018

@nikomatsakis Since there is no safe way to construct a constant that can be given to these functions as an argument and then fail evaluation, it seems fine to merge these.

Also we are explicitly forbidding unions in promoteds (http://play.rust-lang.org/?gist=817735ebe9b1cfd56042819d4bcf545e&version=stable&mode=debug&edition=2015) and casting raw pointers to integers, so these functions becoming const fn cannot cause working runtime code to suddenly produce an error.

That said. Until this is the RFCed strategy for const safety, we should probably just close this.

@kennytm kennytm added I-needs-decision Issue: In need of a decision. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Aug 1, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

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 @TimNN. (Feature Requests)

@nikomatsakis
Copy link
Contributor

I'm inclined to close this until we have a chosen strategy. Going to do so. Thanks @faern for the PR, in any case! =)

@faern faern deleted the stabilize-const-int-ops branch January 31, 2019 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-needs-decision Issue: In need of a decision. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.