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

Crate relies on implicit promotion of const fn calls #46

Open
RalfJung opened this issue Dec 28, 2020 · 12 comments
Open

Crate relies on implicit promotion of const fn calls #46

RalfJung opened this issue Dec 28, 2020 · 12 comments

Comments

@RalfJung
Copy link

There are plans to change the rules for implicit promotion such that calls to const fn are not implicitly promoted to 'static lifetime any more. A crater experiment showed that only very few crates would be affected by this change, and this is one of them.

You can see an example of the relevant build failure here. Usually there is a way to work around this by explicitly putting the intermediate result into a const, of which a reference is then taken. However, since this is macro-generated code, I was unable to find the right spot(s) in the code where this would have to be done.

@rodrimati1992
Copy link
Owner

rodrimati1992 commented Dec 28, 2020

😦 This would be two orders of magnitude more work than every other time that the compiler decided to break my code combined.
The macros would also generate significantly more boilerplate.

The primary issue is that I can't just put the promoted values in a const item in the same scope,
because they reference generic parameters.
So I would have to move those to associated constants, and everything they depend on.

@rodrimati1992
Copy link
Owner

rodrimati1992 commented Dec 28, 2020

So is there an easy way to get the nightly compiler with these changes on x86-64 linux to try making those changes in abi_stable?

@rodrimati1992
Copy link
Owner

I wouldn't have as much of a problem with this change if it was easier to declare constants that can access the generic parameters in the surrounding scope, the only way to do that today is with associated constants.

@RalfJung
Copy link
Author

they reference generic parameters

Ouch, yeah that makes it quite annoying. :/
Note that nothing here is decided yet -- the RFC got accepted, but the RFC explicitly said that we'll do crater experiments to determine the fallout of rejecting such code, and explore alternatives if the fallout is considered too big. This kind of feedback is very useful to determine the best path forward.

The annoying situation here is that the code only ever got accepted because the compiler was extended without RFCs or design/planning... and now we realize that the design that makes most sense accepts less code than what the compiler accepted through these accidents.

Maybe we should wait until inline const expressions support surrounding generics (@oli-obk any timeline for that?).

So is there an easy way to get the nightly compiler with these changes on x86-64 linux to try making those changes in abi_stable?

Depends on your notion of "easy". ;)
With https://github.com/kennytm/rustup-toolchain-install-master installed, you can do rustup-toolchain-install-master 81275a1445c7502a974118e0c69b795ab60be991.

@oli-obk
Copy link

oli-obk commented Dec 28, 2020

Maybe we should wait until inline const expressions support surrounding generics (@oli-obk any timeline for that?).

We can probably start experimenting with that quickly, as making them support surrounding generics is unproblematic when compared to allowing surrounding generics in array lengths.

@rodrimati1992
Copy link
Owner

rodrimati1992 commented Dec 29, 2020

I've downloaded the rustc from rust-lang/rust#80243.

Starting with the changes to abi_stable that are entirely internal, looks like making abi_stable compile with these changes to promotion would take multiple days, since I pervasively use promotion of the parameters/return values of const fns in macros, in ways that would require not only rewriting the macros but also every use of those macros.

It also would require making a breaking change to an attribute (requiring the user to pass the type of the expression passed to the #[sabi(extra_checks = "<expression>")] helper attribute).

If what you've said about uses of this in the wild is true, I've used implicit promotion of calls to user-defined const fn more than every other user of Rust combined.

@rodrimati1992
Copy link
Owner

rodrimati1992 commented Jan 5, 2021

This comment is about the changes made to make abi_stable compile with the pull request that disables promotion of all const fn calls

So I worked on getting most of the code working between sunday and monday (in the fix_fn_promotion branch), it wasn't too difficult, but it took a while.

Changes to make code work

The relevant changes I needed to make code work were:

  • Moving borrows that used to be promoted into (associated) consts.

  • Move the elements passed to the rslice macro into a separate &[_] const item, and replace the rslice macro invocation with RSlice::from_slice called with the slice constant.

Broken API

The API that were broken (described in sections below) can mostly be fixed with inline consts (const {}) that can refer to generic parameters in scope, used internally by the macros.

While this is not something that will affect me, using inline consts for promotion inside the tag and rslice macros would be breaking for anyone who accesses local variables in the macro invocations.

tag macro

The tag macro allows constructing json-like constants of the Tag type.

The pull request breaks the tag macro, by causing the input elements to dangle if any element expression contains a function call or a nested invocation of the tag macro.

Before the pull request, the tag macro worked in a tail expression to initialize a const or static.

These are documentation examples that break in the tagging module:

I intend Tag to also be usable with the StableAbi derive through the #[sabi(tag = "....")] helper attribute, where it has to be able to refer to associated constants and (now) const parameters.

rslice macro

The pull request breaks the rslice macro,
by causing the input elements to dangle if any element expression contains a function call.

Before the pull request, the rslice macro worked in a tail expression to initialize a const or static.

Code that uses rslice like this normally works, and doesn't with hte pull request:

use abi_stable::{
    std_types::RSlice,
    rslice,
};

const fn foo() -> u8 {
    100
}

const _: RSlice<'_, u8> = rslice![0, 1, foo()];

I had to refactor all code like that into:

const S: &[u8] = &[0, 1, foo()];
const _: RSlice<'_, u8> = RSlice::from_slice(S);

effectively making the macro ususable for non-literal and non-const elements.

@oli-obk
Copy link

oli-obk commented Jan 5, 2021

Thank you for this in depth analysis! I'm not sure what our next steps are, we are just figuring out things on our end. It is still possible we'll never end up changing the rules

@RalfJung
Copy link
Author

RalfJung commented Jan 6, 2021

Thank you indeed. :-)

While this is not something that will affect me, using inline consts for promotion inside the tag and rslice macros would be breaking for anyone who accesses local variables in the macro invocations.

Could you explain this in some more detail? Promotion will also not apply if local variables are used in the expression, so I don't think using const {} instead can possibly break anything.

@rodrimati1992
Copy link
Owner

@RalfJung
This is currently valid:

const fn compute_u8() -> u8 {
    10
}

const FOO: &[u8] = {
    let number = compute_u8();
    
    &[number, number]
};

If rslice macro changed to use const { &[$($elem),*] } to promote the array, this:

use abi_stable::std_types::RSlice;
use abi_stable::rslice;

const fn compute_u8() -> u8 {
    10
}

const FOO: RSlice<'_, u8> = {
    let number = compute_u8();
    
    rslice![number, number]
};

would not compile, because AFAIK inline consts cannot refer to local variables.

@RalfJung
Copy link
Author

RalfJung commented Jan 6, 2021

That is true. However, the tail expression rule isn't changing, so you shouldn't have to use const {...} here even with my PR.

@RalfJung
Copy link
Author

RalfJung commented Jan 6, 2021

Or, to use terminology from the promotion documentation -- your FOO (in the first example) does not use promotion. It uses the "enclosing scope rule", and no amount of changes to promotion will make it stop working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants