Stream: t-compiler

Topic: unstable fn ptr in const


Taylor Cramer (Apr 05 2019 at 17:54, on Zulip):

@oli do you have a preference as to how https://github.com/rust-lang/rust/issues/59725#issuecomment-480345963 is implemented? You mentioned before it should be easy. I was planning to write up a change, but wanted to check with you just in case there was some particular way you wanted it done. Or would you rather do it?

Taylor Cramer (Apr 05 2019 at 17:55, on Zulip):

(cc @centril )

Taylor Cramer (Apr 05 2019 at 17:55, on Zulip):

(cc also @RalfJ , I suppose ;) )

centril (Apr 05 2019 at 18:04, on Zulip):

@Taylor Cramer I recall we discussed approaching this like #[rustc_promotable] or something

centril (Apr 05 2019 at 18:05, on Zulip):

i.e. we straight up forbid function pointers right now, but basically you check for the exemption attribute and if it is on a function (by def_id) then you let it slide

centril (Apr 05 2019 at 18:05, on Zulip):

and then there's the usual gating of attributes and such

centril (Apr 05 2019 at 18:07, on Zulip):

here I think a change is needed: https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_mir/transform/qualify_min_const_fn.rs.html#94

Taylor Cramer (Apr 05 2019 at 18:11, on Zulip):

@centril Ah, I see, that check only runs on local_decls, not all rvalues?

Taylor Cramer (Apr 05 2019 at 18:12, on Zulip):

(so callers can still pass an rvalue function pointer without getting an error)

Matthew Jasper (Apr 05 2019 at 18:18, on Zulip):

It will create a temporary unless they declare a constant though:

const FN_PTR: fn() = ...;

const fn() {
// ...
     special_fn(FN_PTR);
}
Matthew Jasper (Apr 05 2019 at 18:20, on Zulip):

Promotion doesn't care though. So users can do:

fn make_vtable() -> &'static _ {
    &special_fn(call as fn())
}
centril (Apr 05 2019 at 18:23, on Zulip):

@Taylor Cramer going for a walk, I'll check back later

Taylor Cramer (Apr 05 2019 at 18:31, on Zulip):

@Matthew Jasper oh, it seems backwards to me that it doesn't affect promotion

Matthew Jasper (Apr 05 2019 at 18:32, on Zulip):

well, min const fn is being stabilized carefully, and promotion was not, so that's how it is.

Taylor Cramer (Apr 05 2019 at 21:16, on Zulip):

https://github.com/rust-lang/rust/pull/59739/commits/701d2197f4ecbe6d168408d59888868bc56e1f2d

centril (Apr 05 2019 at 21:20, on Zulip):

Please split that out into a separate PR

Taylor Cramer (Apr 05 2019 at 21:22, on Zulip):

@centril I considered that and did not do it because otherwise there's no test, and there's not an easy way to add one seeing as it requires adding a stable function to libstd

Taylor Cramer (Apr 05 2019 at 21:22, on Zulip):

I'd rather not land untested code

centril (Apr 05 2019 at 21:23, on Zulip):

@Taylor Cramer It is precisely because this code cannot land because it is not sufficiently tested that I want it split out

Taylor Cramer (Apr 05 2019 at 21:23, on Zulip):

@centril Sorry, I don't understand how splitting it out into a separate PR solves anything

Taylor Cramer (Apr 05 2019 at 21:23, on Zulip):

it's already a separate commit

Taylor Cramer (Apr 05 2019 at 21:23, on Zulip):

You're welcome to comment on that commit and I'm happy to make changes to it there

Taylor Cramer (Apr 05 2019 at 21:24, on Zulip):

there's just no reason to try and land it on its own

centril (Apr 05 2019 at 21:24, on Zulip):

you can use staged_api and such to add tests

Taylor Cramer (Apr 05 2019 at 21:24, on Zulip):

That doesn't add any value

Taylor Cramer (Apr 05 2019 at 21:24, on Zulip):

(having it in a separate PR)

Taylor Cramer (Apr 05 2019 at 21:26, on Zulip):

staged_api

ah, great-- I looked around for something just like that

centril (Apr 05 2019 at 21:30, on Zulip):

@Taylor Cramer https://github.com/rust-lang/rust/search?q=rustc_promotable&unscoped_q=rustc_promotable

Taylor Cramer (Apr 05 2019 at 21:30, on Zulip):

yup, already got the test done, will push in just a second

centril (Apr 05 2019 at 21:31, on Zulip):

also add stability tests and whatnot

Taylor Cramer (Apr 05 2019 at 21:31, on Zulip):

@centril rustc_ attributes are always unstable, behind rustc_attrs

Taylor Cramer (Apr 05 2019 at 21:31, on Zulip):

@centril still want a test?>

centril (Apr 05 2019 at 21:31, on Zulip):

Sure, it should still have tests

Taylor Cramer (Apr 05 2019 at 21:32, on Zulip):

htere are tests showing that rustc_xxx for any xxx is unstable

Taylor Cramer (Apr 05 2019 at 21:32, on Zulip):

but sure

centril (Apr 05 2019 at 21:32, on Zulip):

I know; I generally assume that rustc is incorrect and specify tests independently of whatever machinery rustc uses.

Taylor Cramer (Apr 05 2019 at 21:33, on Zulip):

lol

centril (Apr 05 2019 at 21:33, on Zulip):

so that they act independently

centril (Apr 05 2019 at 21:34, on Zulip):

for example, when I wrote tests for associated type bounds there was a whole lot of redundancy, https://gist.github.com/Centril/2470b7f89657da457ed25078a9cdab72, but redundancy is good in testing.

Taylor Cramer (Apr 05 2019 at 21:36, on Zulip):

Okay, pushed tests

Taylor Cramer (Apr 05 2019 at 21:38, on Zulip):

https://github.com/rust-lang/rust/pull/59739/commits/b5493e541f6da8d848880592c5ac9180f75b254d

Taylor Cramer (Apr 05 2019 at 21:53, on Zulip):

@centril (btw, I'm not just keeping things as one PR because I'm trying to be a pain-- it's legitimately time-consuming and frustrating for me to have PR stacks that are 4+ deep when each has no independent value)

Taylor Cramer (Apr 05 2019 at 21:53, on Zulip):

especially when I have already split things out into separate commits

centril (Apr 05 2019 at 21:54, on Zulip):

independent value: easier to review ;)

centril (Apr 05 2019 at 21:54, on Zulip):

but yes, I understand that PR stacks are annoying sometimes

centril (Apr 05 2019 at 21:54, on Zulip):

I have gone through that also

Taylor Cramer (Apr 05 2019 at 21:54, on Zulip):

it is easier for you to click on the specific commit to review than it is for me to rebase down the chain :)

centril (Apr 05 2019 at 21:54, on Zulip):

fwiw, I am reviewing atm ;)

Taylor Cramer (Apr 05 2019 at 21:54, on Zulip):

esp. since you have to do that anyways if you want to leave comments on the correct PR

centril (Apr 05 2019 at 21:54, on Zulip):

(by clicking through the commits)

Taylor Cramer (Apr 05 2019 at 21:55, on Zulip):

otherwise you're reviewing the PR + all the preceding ones

Taylor Cramer (Apr 05 2019 at 21:55, on Zulip):

/me whines about github

centril (Apr 05 2019 at 21:56, on Zulip):

at least GH isn't taking away my https://inbox.google.com ;)

centril (Apr 05 2019 at 22:04, on Zulip):

@Taylor Cramer can you make changes in https://github.com/rust-lang/rust/blob/master/src/libsyntax/diagnostic_list.rs#L421 as well?

Taylor Cramer (Apr 05 2019 at 22:04, on Zulip):

@centril ah crap

Taylor Cramer (Apr 05 2019 at 22:04, on Zulip):

the PR I was changing is in your rollup

Taylor Cramer (Apr 05 2019 at 22:05, on Zulip):

:/

Taylor Cramer (Apr 05 2019 at 22:05, on Zulip):

what do

centril (Apr 05 2019 at 22:05, on Zulip):

my rollup isn't getting r-ed ^^

Taylor Cramer (Apr 05 2019 at 22:05, on Zulip):

@centril yeah, so I guess I'll make the changes in the second PR?

Taylor Cramer (Apr 05 2019 at 22:05, on Zulip):

even though that's not where that change was introduced

centril (Apr 05 2019 at 22:06, on Zulip):

@Taylor Cramer we have 3 PRs right?

Taylor Cramer (Apr 05 2019 at 22:06, on Zulip):

@centril correct

centril (Apr 05 2019 at 22:06, on Zulip):

dizzifying

Taylor Cramer (Apr 05 2019 at 22:06, on Zulip):

re diagnostics_list, yes

Taylor Cramer (Apr 05 2019 at 22:06, on Zulip):

(well, 4 if you count your rollup XD)

centril (Apr 05 2019 at 22:07, on Zulip):

:D

Taylor Cramer (Apr 05 2019 at 22:12, on Zulip):

@centril so I guess I'll change the second PR to include the changes from the first

Taylor Cramer (Apr 05 2019 at 22:12, on Zulip):

that will be lost when it's closed by your rollup PR

Taylor Cramer (Apr 05 2019 at 22:12, on Zulip):

and just not rebase my second PR

centril (Apr 05 2019 at 22:13, on Zulip):

@Taylor Cramer hmm; do those tests need rustc_const_unstable? also, could you duplicate the allow_const_fn_ptr.rs without the const_fn gate?

centril (Apr 05 2019 at 22:13, on Zulip):

@Taylor Cramer the rollup just failed, https://github.com/rust-lang/rust/pull/59736#issuecomment-480438342

Taylor Cramer (Apr 05 2019 at 22:17, on Zulip):

oh for gosh sakes

Taylor Cramer (Apr 05 2019 at 22:17, on Zulip):

I just committed the changes to the second PR

Taylor Cramer (Apr 05 2019 at 22:17, on Zulip):

what a mess XD

centril (Apr 05 2019 at 22:19, on Zulip):

I would suggest using the strategy of waiting ;)

Taylor Cramer (Apr 05 2019 at 22:22, on Zulip):

@centril no waiting

Taylor Cramer (Apr 05 2019 at 22:22, on Zulip):

now is the time for stabilization!

Taylor Cramer (Apr 05 2019 at 22:23, on Zulip):

so but seriously

centril (Apr 05 2019 at 22:23, on Zulip):

@Taylor Cramer hmm; why does RawWaker::new use the hack attribute?

centril (Apr 05 2019 at 22:23, on Zulip):

it accepts no function pointers

centril (Apr 05 2019 at 22:23, on Zulip):

(not directly)

Taylor Cramer (Apr 05 2019 at 22:23, on Zulip):

probably doesn't need to

centril (Apr 05 2019 at 22:30, on Zulip):

argh... why doesn't GH let me leave comments on commits :confused:

centril (Apr 05 2019 at 22:49, on Zulip):

@Taylor Cramer btw, there's @bors r=withoutboats for that

centril (Apr 05 2019 at 22:49, on Zulip):

git history looks weird otherwise

Taylor Cramer (Apr 05 2019 at 22:56, on Zulip):

@centril

Also please extend this test with a file that doesn't use const_fn

Taylor Cramer (Apr 05 2019 at 22:56, on Zulip):

^^ah yeah sorry just wasn't thinking about it

centril (Apr 05 2019 at 22:56, on Zulip):

hehe ^^

Taylor Cramer (Apr 05 2019 at 22:56, on Zulip):

can you explain more what you meant about that test?

Taylor Cramer (Apr 05 2019 at 22:56, on Zulip):

what are you trying to test by leaving out const_fn?

centril (Apr 05 2019 at 22:57, on Zulip):

that there's no special effects brought in by that feature gate; we have similar tests for min_const_fn

centril (Apr 05 2019 at 22:57, on Zulip):

(we have a lot of tests for min_const_fn ^^)

centril (Apr 05 2019 at 22:59, on Zulip):

moreover, it also tests that the gate isn't needed for what you want

Taylor Cramer (Apr 05 2019 at 23:13, on Zulip):

@centril sorry, I'm still not quite following

Taylor Cramer (Apr 05 2019 at 23:13, on Zulip):

const fn is needed

centril (Apr 05 2019 at 23:13, on Zulip):

@Taylor Cramer the feature gate?

Taylor Cramer (Apr 05 2019 at 23:13, on Zulip):

yeah, or at least it was when I was testing initially

Taylor Cramer (Apr 05 2019 at 23:14, on Zulip):

but I can try removing it (have to wait to find out-- rustc :( )

Taylor Cramer (Apr 05 2019 at 23:15, on Zulip):

trying now

centril (Apr 05 2019 at 23:16, on Zulip):

@Taylor Cramer btw, how are you doing these tests without getting "error: crate has missing stability attribute" ?

Taylor Cramer (Apr 05 2019 at 23:23, on Zulip):

@centril sorry, not sure what you mean

Taylor Cramer (Apr 05 2019 at 23:23, on Zulip):

everything has been working fine for me

centril (Apr 05 2019 at 23:24, on Zulip):

weird; I got that error on playground

Taylor Cramer (Apr 05 2019 at 23:24, on Zulip):

huh

Taylor Cramer (Apr 05 2019 at 23:24, on Zulip):

no idea

centril (Apr 05 2019 at 23:24, on Zulip):

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=e696d9513b54231078cc28db8e7b52ba

Taylor Cramer (Apr 05 2019 at 23:25, on Zulip):

@centril maybe the error I am getting is hiding that one

centril (Apr 05 2019 at 23:25, on Zulip):

@Taylor Cramer can you extract the fn compiles to a // run-pass test?

Taylor Cramer (Apr 05 2019 at 23:25, on Zulip):

@centril lol sure

centril (Apr 05 2019 at 23:26, on Zulip):

thanks

Taylor Cramer (Apr 05 2019 at 23:26, on Zulip):

getting damn picky about this

Taylor Cramer (Apr 05 2019 at 23:26, on Zulip):

;)

centril (Apr 05 2019 at 23:26, on Zulip):

:D

Taylor Cramer (Apr 05 2019 at 23:28, on Zulip):

yup, then I get the "crate has missing stability attribute" error

centril (Apr 05 2019 at 23:30, on Zulip):

good; logic is restored :P

Taylor Cramer (Apr 05 2019 at 23:32, on Zulip):

Okay, pushed

centril (Apr 05 2019 at 23:37, on Zulip):

thanks; looks good aside from my nit

Taylor Cramer (Apr 05 2019 at 23:37, on Zulip):

first you want run-pass

Taylor Cramer (Apr 05 2019 at 23:37, on Zulip):

then you want // run-pass

Taylor Cramer (Apr 05 2019 at 23:37, on Zulip):

XD

centril (Apr 05 2019 at 23:38, on Zulip):

@Taylor Cramer I think you read my zulip comment before I managed to edit ;)

Taylor Cramer (Apr 05 2019 at 23:38, on Zulip):

haha I definitely did

Taylor Cramer (Apr 05 2019 at 23:38, on Zulip):

XD

centril (Apr 05 2019 at 23:38, on Zulip):

:D

centril (Apr 05 2019 at 23:39, on Zulip):

I'm like a :turtle:

Taylor Cramer (Apr 05 2019 at 23:40, on Zulip):

done

centril (Apr 05 2019 at 23:43, on Zulip):

excellent

Last update: Nov 20 2019 at 02:15UTC