search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.86k stars 12.67k forks source link

Tracking Issue for inline const patterns (RFC 2920) #76001

Open nikomatsakis opened 4 years ago

nikomatsakis commented 4 years ago

This is a tracking issue for the RFC "Inline const expressions and patterns" (rust-lang/rfcs#2920). Const expressions have been stabilized, but patterns have not The feature gate for the issue is #![feature(inline_const_pat)].

This was originally a tracking issue for const blocks in both expression and pattern position. Inline const expressions have been stabilized in https://github.com/rust-lang/rust/pull/104087 while patterns are still unstable.

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

jhpratt commented 4 years ago

With regard to the lint, is there any semantic difference between the two? Intuitively I would expect them to behave identically (static lifetimes).

RalfJung commented 4 years ago

They behave identically. The point of the lint is to have a single "idiomatic" form.

Naming: "inline const", "const block", or "anonymous const"?

The issue title calls it "const expressons" so maybe that should also be on the list.

I prefer "inline const" because it emphasizes that this is a totally separate body of code that is just written inline. That's much more like a closure than normal blocks.

Also, is anyone up for implementing this? (I can't even mentor this I am afraid, this affects surface-level syntax and MIR building which is way outside what I know.^^)

Lokathor commented 4 years ago

"const block" is clearly correct.

it matches "unsafe block" that way.

RalfJung commented 4 years ago

That false parallel with "unsafe block" is exactly why it is not correct. An unsafe block inherits scope, execution environment, everything from its parent block. An inline const does not.

ecstatic-morse commented 4 years ago

I'm happy to mentor, although by no means am I an expert on all the parts of the compiler you'll need to touch. There's already an AnonConst type in the AST and the HIR that should for the most part have the same rules as an inline const (e.g. no outside generic parameters), which should make the lowering part of this pretty easy.

If no one volunteers in the next few weeks, I'll try to set aside a day or two to implement this. However, I'm mostly in maintenance/review mode as far as Rust is concerned.

workingjubilee commented 4 years ago

Does const { } differ from normal block expressions, however? I think either "inline const" or "const expression" should be favored. Everyone's going to call it a constexpr anyways because of C++ user bleedover, and I don't think it's worth offering that much pushback this time. :^)

Hm... I need to fix my computer still. Guess I have a good reason today.

memoryruins commented 4 years ago

Does const { } differ from normal block expressions, however?

const {} does not inherit the same scopes as block expressions do.

Everyone's going to call it a constexpr anyways because of C++

It can be a useful comparison when introducing C++ users to Rust's const and const fn, yet I have not seen most users conflate them. Additionally, C++ does not have constexpr { ... }.

I don't think it's worth offering that much pushback this time.

There are no mentions of constexpr or C++ on this issue before or the RFC. I find it unproductive to label it as pushback from that. The concerns about naming are mentioned in the RFC https://rust-lang.github.io/rfcs/2920-inline-const.html and the comments.

workingjubilee commented 4 years ago

Alas, I was trying to make a rather more casual and conversational remark rather than something that I expected to be picked over with a fine-tooth comb and labeled "unproductive". Resolved.

However, when discussing consts, someone literally referred to a similar expression as a "constexpr", to me, today, when they were thinking about trying to write a macro that attempted to implement this feature.

memoryruins commented 4 years ago

I was not responding to the idea of using constexpr unproductive, and I mentioned that the comparison to it can still be useful. I specifically referred to describing current naming discussions as "pushback" to be unproductive. I believe we have had a misunderstanding, and I would be open to discussing this more in a direct message on discord. I do not think we should continue this here.

spastorino commented 4 years ago

@ecstatic-morse I'd like to take this issue if it's not taken yet.

ecstatic-morse commented 4 years ago

Go for it @spastorino!

spastorino commented 4 years ago

I forgot to link this issue from the PR but the PR is now merged https://github.com/rust-lang/rust/pull/77124

camelid commented 3 years ago

I would like to write the documentation.

spastorino commented 3 years ago

I would like to write the documentation.

@camelid please once you have something up cc me so I can review.

camelid commented 3 years ago

Note that right now the feature is named inline-const pretty much everywhere.

ghost commented 3 years ago
  • [x] Implement the RFC (#77124 does most of it, modulo some bugs around range patterns that should be fixed in #78116)

As the above checkbox has been checked and both #77124 and #78116 have been merged, I assume this feature is fully implemented. Why is it still in the INCOMPLETE_FEATURES list?

oli-obk commented 3 years ago

I think that's just an oversight, we should remove it from the incomplete features list.

ghost commented 3 years ago

I think that's just an oversight, we should remove it from the incomplete features list.

@oli-obk I'm willing to submit a PR, so I can use this in my code without warning.

oli-obk commented 3 years ago

Great! Feel free to r? @oli-obk it

RalfJung commented 3 years ago

While working on promotion recently, here's something I wondered about... would it make sense for const {} to use the promotion machinery? After all, that machinery already has all the support we need to let the const refer to generics of the surrounding context.

The check for the code in there should be the usual const check of course, not the implicit promotion check. But arguably the same is true for explicit promotion contexts, isn't it? const {} would be as explicit as it gets for promotion contexts. ;)

The current explicit promotion contexts are rustc_args_required_const and inline assembly const operands... do we still have time to take away promotion from both of them and require a const literal or inline const block?

lcnr commented 3 years ago

@RalfJung I am not sure if i understood your question correctly

would it make sense for const {} to use the promotion machinery?

I personally don't know what exactly you are thinking of here. At least from where I am approaching this the way const blocks should deal with the generics of the surrounding context is quite different to promotion. Especially considering potentially failing operations like const { 64 / mem::size_of::<T>() } which we probably want to allow but imo should require feature(const_evaluatable_checked). I can't really see that working well with promoteds as it happens during typeck while promotion happens afterwards.

The current explicit promotion contexts are rustc_args_required_const and inline assembly const operands... do we still have time to take away promotion from both of them and require a const literal or inline const block?

idk, it would sure be nice to do so. Don't know too much about either of those, I am however surprised that inline assembly const operands rely on promotion at all rn :laughing: would have expected them to use an AnonConst instead.

RalfJung commented 3 years ago

I personally don't know what exactly you are thinking of here.

Basically, compile const { expr } like expr during MIR building (except with restrictions for which local variables are in scope), and then use this as a promotion site in transform/promote_consts.rs.

const {} is primarily a means to replace implicit promotion with something more explicit. Promotion already does everything we want const {} to do, except that it is subject to some serious restrictions because it is implicit. So using promotion to implement const {} seems like the easiest option.

Especially considering potentially failing operations like const { 64 / mem::size_of::() } which we probably want to allow but imo should require feature(const_evaluatable_checked).

Uh, that's a big move compared to any prior discussion I am aware of.^^ This might smash all hopes I had of stabilizing this any time soon, which could be quite problematic for getting promotion cleaned up before it is too late.

I expected this would work just like promoteds and associated consts work today. No need to treat these likes const generics. (I know basically nothing about const_evaluatable_checked, just that it is somehow related to ensuring that const generic parameters do not fail to evaluate.)

idk, it would sure be nice to do so. Don't know too much about either of those, I am however surprised that inline assembly const operands rely on promotion at all rn laughing would have expected them to use an AnonConst instead.

If AnonConst is the same as inline const expressions, then asm! predates that by quite a bit.

lcnr commented 3 years ago

AnonConst are used to represent constant expressions, so they are used for array lengths and associated constants.

moving this to zulip until I have a more solid understanding of our goals here. https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/inline.20consts

RalfJung commented 3 years ago

Independent of the discussion around const_evaluatable_checked, another way to phrase my point is: I think it is a bad idea to have both "explicit promotion" and inline consts. The two serve almost the same purpose, so having to come up with, maintain, and test two separate sets of rules is a lot of extra work for no gain that I can see.

I think we should aim at unifying the two. This means treating rustc_args_required_const arguments (I assume some of the functions decorated with this are already stable?) as inline consts (and potentially, with an edition change, requiring the const { ... } keyword if the argument isn't a literal or named const); for asm! const arguments there's still time to work out the details since this isn't stable yet.

RalfJung commented 3 years ago

Replying to @oli-obk's comment here

For rustc_args_required_const we can't do this because the invocation of the function would need to resolve the call before knowing that the argument is a const.

Name resolution happens before MIR construction, so we could still do it on that level... not sure how well that would work with the current implementaton of inline consts though.

Also, besides the point I made above about unifying explicit promotion and inline consts, here's another one: inline consts (with support for using generics) and implicit promotion seem to have a lot in common -- both are expressions that are syntactically "embedded" in their parent context and want to inherit at least some of its information (generics; possibly type inference information as well). So maybe it makes sense to implement them in similar ways?

It is this point together with the above that lead to my first message on the subject. The feedback I got helped tease the two points apart. :) I think the first one is much stronger than the second.

oli-obk commented 3 years ago

Name resolution happens before MIR construction, so we could still do it on that level... not sure how well that would work with the current implementaton of inline consts though.

Ah, good point, we could do this as an "expansion" in AST -> HIR lowering

oli-obk commented 3 years ago

both are expressions that are syntactically "embedded" in their parent context and want to inherit at least some of its information (generics; possibly type inference information as well). So maybe it makes sense to implement them in similar ways?

While not generally possible for anonymous constants, just for inline consts, that could be doable, but we'd need some sort of marker on the local that contains the final value of the inline const, so that "promotion" can reliably pick it up and uplift its creating MIR statements to a promoted MIR. Though considering that we had all the relevant information about what expressions are part of it before creating the MIR, picking it apart again later seems odd.

The oppsite way of unifying the systems would be to figure out what code is (implicitly) promotable on the HIR, but we explicitly moved away from such a system due to various problems with it.

An intermediate way could be to give inline consts (and possibly repeat expression lengths) their own wrapper type around AnonConst, which does not give them their own DefId, but THIR lowering will just create promoteds at MIR creation time instead of putting them into their own MIR body that may end up having cycles with their parent MIR body queries. That would still not unify the creation systems of promotion and inline consts, but it would treat them exactly the same once we are not processing HIR anymore.

RalfJung commented 3 years ago

Ah, good point, we could do this as an "expansion" in AST -> HIR lowering

Ah right, HIR is already name-resolved... so this should not be so bad.

The oppsite way of unifying the systems would be to figure out what code is (implicitly) promotable on the HIR, but we explicitly moved away from such a system due to various problems with it.

Yeah... I guess on the HIR, + might still be a function call using the Add trait. So that's probably not a good idea.

So, together with everything else you said, I tend to agree that it makes more sense to immediately build separate MIR bodies. That probably also makes it easier to use the normal const-expr checks here.

The key simplification that we can hopefully achieve, then, is to make transform/promote_consts.rs only about (implicit) lifetime extension, and nothing else. "Promotion" and "lifetime extension" might mean the same thing again, as they once did. ;)

That would still not unify the creation systems of promotion and inline consts, but it would treat them exactly the same once we are not processing HIR anymore.

Promoteds are already very similar to regular consts on the MIR, so I don't think this would buy us much.

oli-obk commented 3 years ago

So, together with everything else you said, I tend to agree that it makes more sense to immediately build separate MIR bodies. That probably also makes it easier to use the normal const-expr checks here.

Promoteds are already very similar to regular consts on the MIR, so I don't think this would buy us much.

These two statements seem at odd to each other (since the first one may mean the "build promoteds at THIR time, but the second one says that doesn't gain us much), so just to be clear, so we are all on the same page: We don't change anything in the current system except that we generate AnonConst in HIR lowering for rustc_args_required_const, thus giving it its own DefId and thus MIR.

RalfJung commented 3 years ago

Sorry, to be more clear: in the second statement I meant the way they look once promotion is done messing up the MIR. Then promoteds are just an Operand::Const, just like regular consts and inline const blocks, right?

oli-obk commented 3 years ago

yes.

RalfJung commented 3 years ago

We don't change anything in the current system except that we generate AnonConst in HIR lowering for rustc_args_required_const, thus giving it its own DefId and thus MIR.

Right, and likewise for asm! const arguments.

This is assuming that equipping AnonConst with support for generics isn't too hard (that's the thing that already works for promoteds).

RalfJung commented 3 years ago

Regarding rustc_args_required_const, also see https://github.com/rust-lang/stdarch/issues/248. However, not promoting them at all breaks widely used crates, see https://github.com/rust-lang/rust/pull/80759. So if we want to get rid of explicit promotion, we do indeed have to use inline consts for rustc_args_required_const -- or we use implicit promotion.

jhpratt commented 3 years ago

What are the blockers here? Naming is something that should be trivially resolved and doesn't really matter as to the language itself. The only other issue listed in the issue is a lint, which I personally don't think should preclude stabilization.

RalfJung commented 3 years ago

There are some ICEs listed here; AFAIK at least some of these are critical.

RalfJung commented 3 years ago

Regarding the discussion whether inline consts that use surrounding generics should be subject to const_evaluatable_checked-style rules or not, here are some more arguments for why I think they should not:

oli-obk commented 3 years ago

I know @estebank was shitposting on twitter, but I think the following actually has merit:

Postfix .const for inline const expressions (not patterns though, as that probably has parsing issues). Doing this for expressions has various advantages:

RalfJung commented 3 years ago

I don't think I like it... we talk about const-contexts for a reason, this is a very different "scope" to operate in. postfix makes this way too subtle. Also it doesn't support cases well where the thing inside the const block is not a method call (chain).

RalfJung commented 3 years ago

FWIW, some (crude) approximation of this feature can be implemented on stable with macros. ;)

c410-f3r commented 2 years ago

let_chains has the same "non-terminals" problem and was accepted regardless. Maybe the team will deal with it the next edition? IDK...

That said, what else is preventing the stabilization of inline_const?

nbdd0121 commented 2 years ago

I believe I have fixed most problems with inline_consts. Inline const in pattern position still has some soundness issues; it's now in a separate feature gate, but I still hope that could be fixed before inline const in expr position is stablised.

Also, I have some feature extension to inline_const planned and prototyped, but didn't have a lot time recently to finalize and polish it enough for a PR. For example, allow inline consts to reference generic parameters. In my prototype I can see that this is possible, and is very useful (e.g. you could use const { assert!() } as static asserts).

c410-f3r commented 2 years ago

I believe I have fixed most problems with inline_consts. Inline const in pattern position still has some soundness issues; it's now in a separate feature gate, but I still hope that could be fixed before inline const in expr position is stablised.

Also, I have some feature extension to inline_const planned and prototyped, but didn't have a lot time recently to finalize and polish it enough for a PR. For example, allow inline consts to reference generic parameters. In my prototype I can see that this is possible, and is very useful (e.g. you could use const { assert!() } as static asserts).

Thanks for the uddate, @nbdd0121!

RalfJung commented 2 years ago

From what I heard, the asm! macro folks are eagerly awaiting inline const stabilization since const arguments in asm! are blocked on this. So it might make sense to not block inline consts in expressions on patterns or generic support -- those can be added later in a future-compatible way, can't they?

nbdd0121 commented 2 years ago

The asm!'s const is no longer same as inline const's, after inline const got type inference.

nbdd0121 commented 2 years ago

A bit of more detail: inline const used to be a thin wrapper around AnonConst implemented in rustc, which is also used by array lengths, repeat counts, etc. Asm's const also uses AnonConst. However this is changed because AnonConst is type checked itself while for inline const type inference is wanted. Today, even though inline const impl still uses AnonConst, it has additional handling and is typechecked in a similar way to closures. Asm's const and array lengths have DefKind::AnonConst while inline const now is DefKind::InlineConst.

Amanieu commented 2 years ago

Should asm! be changed to use InlineConst as well? It's not entirely clear to me what the differences are, but it seems that InlineConst allows the use of generic parameters of the containing function.

spastorino commented 2 years ago

I've just noticed that I'm assigned to this issue but I'm not working on it. I've done some initial work in the past but I'm not doing anything since a long while.

I guess @lcnr maybe be actively working on this or may know who is working on it.

lcnr commented 2 years ago

Should asm! be changed to use InlineConst as well? It's not entirely clear to me what the differences are, but it seems that InlineConst allows the use of generic parameters of the containing function.

yes, asm! should probably also use InlineConst instead of ordinary anon consts. @Amanieu please ping me on zulip if that's something you want to look into. Don't have too much time rn but want to really clean up the story wrt constants in rust.

scottmcm commented 2 years ago

Random idea I just had: what if ? was special in inline consts? I don't think it ever makes sense for ? inside a const{} to return from the enclosing function -- it might not even be possible.

So we could potentially have const { "foo".parse()? } mean the same as const { "foo".parse().unwrap() }, with the error case giving a compilation error.

Usual disclaimer: this is an unbaked idea. It might be terrible, and is not a proposal.

lcnr commented 2 years ago

I would expect this to be to case. Inline consts should behave similar to closures in that regard.