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

avoid relying on implicit promotion #41

Closed
wants to merge 1 commit into from

Conversation

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.

This PR adjusts the code to no longer rely on implicit promotion, by explicitly putting the result of the function call into a const item. Long-term, there will be the possibility of using inline const expressions instead, which will be more ergonomic.

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 28, 2020

That's really anoying: the only usage of the one letter const function is to have an ergonomic way of defining a keyboard keymap. See for example this: https://github.com/TeXitoi/ortho60-keyberon/blob/master/src/layout.rs

The users of keyberon highly rely on these functions to define their keymap. Is there another workaround? I'd prefer that these function always return a 'static that breaking all the existing keymap. I'll try to understand the RFC, a quick look didn't make the modification obvious.

@RalfJung
Copy link
Author

RalfJung commented Dec 28, 2020

The modification is as such: your code relies on the fact that &[l(1), k(LShift)] is implicitly converted into a const with value [l(1), k(LShift)], and a reference to that const. This is called "(implicit) promotion". This mechanism has caused a lot of pain over the last few years, and we finally have an idea for how to fix the pain: by only promoting things where we will know for sure that they will not fail to evaluate.

const fn can do anything, so they can fail to evaluate. We should never have promoted them to begin with, but sadly, promotion was extended over the years without RFCs or any kind of design or plan. So now we are attempting to "put the djinn back into the bottle", or at least seeing how hard that would be.

Other possible work-arounds that come to my mind:

  • Macros should work.
  • Enum variant constructors are promoted, so if l could be a variant of the Action enum, that would work.
  • If you can find some way to avoid having to take a reference (&), that should work.

@RalfJung
Copy link
Author

The macro could even be a generic one, e.g. a!(expr) could expand to

{
  const _HIDDEN: Action = expr;
  _HIDDEN
}

That would basically do promotion explicitly, and thus avoid relying on implicit promotion.

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 28, 2020

The modification is as such: your code relies on the fact that &[l(1), k(LShift)] is implicitly converted into a const with value [l(1), k(LShift)], and a reference to that const. This is called "(implicit) promotion".

How to do that explicitly? I need to declare a const for each array entry? Any other way?

Macros should work.

That's a possibility. I try to avoid them when possible, that's why I was using const fn

The better I can imagine is https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=2eaf47075e038b31d55e4f3e16bac214

But it looks quite hackish. I'd better define a l! macro.

Enum variant constructors are promoted, so if l could be a variant of the Action enum, that would work.

Well, fn l is really a shortcut, it's defined as

pub const fn l<T>(layer: usize) -> Action<T> {
    Action::Layer(layer)
}

If you can find some way to avoid having to take a reference (&), that should work.

Well, the idea is to have a static with the type &'static [&'static [&'static [Action<T>]]], so that doesn't seems to be an option.

@RalfJung
Copy link
Author

How to do that explicitly? I need to declare a const for each array entry? Any other way?

A const for each array would also work (but then the const type needs to state the array length).

Well, fn l is really a shortcut, it's defined as

Hm... I wonder if there's another way to define an alias, but I can't think of one.

The better I can imagine is

Yeah that looks pretty good as well.

@danielhenrymantilla
Copy link

danielhenrymantilla commented Dec 28, 2020

(but then the const type needs to state the array length)

Funnily enough, that isn't an issue by repeating the array expression:

macro_rules! const_array {(
    const $NAME:tt : [$T:ty; _] = [ $($expr:expr),+ $(,)? ];
) => (
    const $NAME : [$T; [$($expr),+].len()] = [$($expr),+];
)}

@RalfJung
Copy link
Author

Funnily enough, that isn't an issue by repeating the array expression:

Oh, nice trick. This will evaluate the expression twice, though.

@danielhenrymantilla
Copy link

Indeed, although, afaik, that shouldn't matter for a <constexpr> / const fn evaluated in a const, right?

@RalfJung
Copy link
Author

Not sure what you mean by "shouldn't matter" -- the CTFE engine will evaluate this code twice, then.

@danielhenrymantilla
Copy link

You're right, I should have clarified. I think that it affecting compile-time is indeed not to be disregarded; (I was thinking about some semantic change / something about the result of a const fn potentially depending on the number of times it was called; I know that such thing doesn't make sense for a const function, but I've heard that the const name could be a bit misleading 🤷).

Anyways,

-   const $NAME : [$T; [$($expr),+].len()] = [$($expr),+];
+   const $NAME : [$T; 0 $( + {ignore!($expr);1} )+] = [$($expr),+];
// or
+   const $NAME : [$T; [$( ignore!($expr) ),+].len()] = [$($expr),+];

can be a way to make sure the CTFE of the len is "simple enough" (where ignore! is some macro that ignores its input and yields some dummy expression: it can be hand-written, but stringify! works).

@RalfJung
Copy link
Author

RalfJung commented Jul 7, 2021

rust-lang/rust#80243 is closed, so let's also close this.

@RalfJung RalfJung closed this Jul 7, 2021
@TeXitoi
Copy link
Owner

TeXitoi commented Jul 7, 2021

OK, thanks.

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

Successfully merging this pull request may close these issues.

None yet

3 participants