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

Tracking issue for RFC 2526, "Support underscores as constant names" #54912

Closed
3 of 4 tasks
Centril opened this issue Oct 8, 2018 · 46 comments · Fixed by rust-lang/reference#620
Closed
3 of 4 tasks
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Oct 8, 2018

This is a tracking issue for the RFC "Support underscores as constant names" (rust-lang/rfcs#2526).

Steps:

Unresolved questions:

None

@Centril Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Oct 8, 2018
@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 8, 2018

Ugh, seems like a legitimized hack rather than a... well, any of several proper features to which this can be generalized.

At least this is a very tiny hack.
Implementation strategy is the same as for use Trait as _;.
If we encounter _ instead of the const name identifier, then we create a gensym for it instead of storing it in AST directly (so that different _s do not conflict).
Together with a feature gate and tests that would be all the implementation.

@Centril
Copy link
Contributor Author

Centril commented Oct 8, 2018

Ugh, seems like a legitimized hack rather than a... well, any of several proper features to which this can be generalized.

Hehe, I agree we should generalize (to patterns) eventually but it seemed that it was much more complex from @eddyb's comments. :)

@dsciarra
Copy link
Contributor

dsciarra commented Oct 9, 2018

looks interesting, I will give it a try!

@earthengine
Copy link

@petrochenkov

Can we also ensure that those gensyms get removed after before codegen? This means, the generated MIR should be removed in some MIR rounds.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 11, 2018

We don't need to remove any MIR. Unused private constants don't end up in metadata.

@earthengine
Copy link

earthengine commented Oct 12, 2018

@oli-obk

Fine. There is one "inconsistency" in Rust today:

let _: str = *"123"; //Legal
let _v: str = *"123"; //Illegal unless unsized_rvalues

When implementing this, how do we want to justify

const _: str = *"123";
const _v: str = *"123";

Shall we repeat the inconsistency, or shall we count both valid or invalid?

@oli-obk
Copy link
Contributor

oli-obk commented Oct 12, 2018

I'd start out with both being disallowed, since that is the current behaviour of constants. But I have no strong opinion.

bors added a commit that referenced this issue Oct 14, 2018
…enkov

Support underscore as constant name

Issue: #54912
@Centril Centril added B-unstable Blocker: Implemented in the nightly compiler and unstable. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. labels Oct 17, 2018
@Centril
Copy link
Contributor Author

Centril commented Nov 11, 2018

Setting earliest date for stabilization proposal to 25th november 2018 (14 days from now).

We'll likely want to discuss what direction generalizations should go in before stabilizing.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 15, 2018

The RFC did not discuss static _: T = ...;. Is that supposed to work, too? (the implementation does in fact allow this, accidentally even without a feature gate)

Fix for the stability hole is in #55983

@nikomatsakis
Copy link
Contributor

I'm having a bit of second thoughts about this RFC. I feel like (a) the intention of const _: T = { .. } is pretty unclear and (b) it opens some weird questions about e.g. generic constants and how they fit here and (c) it doesn't support full patterns, which it kind of suggests it would support.

The motivation of "ensure some code type-checks" is real though. I sort of like the syntax const { ... } for that purpose. Thoughts?

I guess I'm probably just re-raising thoughts that came up on the RFC.

@RalfJung
Copy link
Member

I sort of like the syntax const { ... } for that purpose. Thoughts?

Could that be re-used for explicit promotion as @oli-obk plans it?

@nikomatsakis
Copy link
Contributor

@RalfJung seems plausible. The one concern I could see is whether there would be some conflict as const { .. } could be an item and an expression, and we might want the latter to inherit the lexical scope from the surrounding fn, so it can refer to generic constant names -- but we could just say that const { .. } items cannot appear in functions (since...what's the point).

@cramertj
Copy link
Member

but we could just say that const { .. } items cannot appear in functions (since...what's the point).

Wouldn't that defeat the purpose of explicit promotion?

@Centril
Copy link
Contributor Author

Centril commented Dec 13, 2018

With respect to a) the intention of const _: T = { ... }; being unclear I agree with that in isolation. However, in the context of being wrapped in a macro such as const_assert! { ... } or inside a derive macro it feels rather clear what the purpose of this construct is.

My main worry is instead b) and c). That is, would we permit this?

const (A, B): (AType, BType) = (1, 2);

(this would be the natural extension of const _: Type = expr; because _ acts as a pattern in this context.

but also this?

const A<T>: AType<T> = expr;

and how can these be composed?

If we write:

const (A<T>, B<U>): (AType<U>, BType<T>) = expr;

does that make any sense?

I suppose that we could support generic constants when A<T> is written and patterns otherwise; that likely works syntactically but it could cause confusion (or maybe not... I'm not sure).

Finally, ideally, to keep the language consistent, we should keep const and static items as much as possible in sync syntactically. If we support const _: Type = expr; we should imo support static _: Type = expr; even if we lint on the latter.


As for supporting const { ... }, I think the meaning here isn't clear... there could be 2 different interpretations:

  1. The entire block { ... } is evaluated at compile time. This interpretation is based on const items.
  2. The entire block may be evaluated at compile time if all variables referenced are. Notably, you could write: let x = random(); let y = const { x * x };. This interpretation is based on const fn items. 2. would subsume 1.

@RalfJung
Copy link
Member

Wouldn't that defeat the purpose of explicit promotion?

const expressions would be allowed in function bodies, just const items would not.

@DoumanAsh
Copy link

I feel like (a) the intention of const _: T = { .. } is pretty unclear

I believe it is pretty clear to user as we can use let VAR = { ... }

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2018

The entire block may be evaluated at compile time if all variables referenced are. Notably, you could write: let x = random(); let y = const { x * x };. This interpretation is based on const fn items. 2. would subsume 1.

I don't think that's a useful interpretation. There's absolutely nothing this const block could promise us. It's just regular code.

I agree with everything else. We should just allow constants with patterns and generic constants. We already support them inconveniently by defining a const function without arguments.

@Centril
Copy link
Contributor Author

Centril commented Jan 13, 2019

It's just regular code.

It's not. Notably, you may only reference existing bindings. Any function calls must still be const fn. For example, let y = const { random() }; would not be allowed.

I don't think that's a useful interpretation. There's absolutely nothing this const block could promise us.

There's plenty this would tell you:

  1. If you reference no bindings from outside you know it is evaluated at compile time.
  2. The block does not admit non-determinism even if bindings referenced are non-deterministically evaluated. This gives you a local way to enforce freedom from certain side-effects.

Now.. it might be confusing to have block form entitled const { .. } behave in this way, but that doesn't mean it isn't useful to behave in this way.

@nox
Copy link
Contributor

nox commented Mar 2, 2019

I don't really understand how generic constants can cause issues with unnamed consts, and AFAIK supporting _ is fully forwards-compatible with generalising const to fully accept patterns. Is there any chance this could be stabilised? Many derive macros out there currently generate a difficult-to-guess const name to scope extern crate and use statements.

@roblabla
Copy link
Contributor

Currently, the constants show up as _ in the documentation. https://sunriseos.github.io/SunriseOS/master/sunrise_ahci/hba/index.html#constants

@withoutboats
Copy link
Contributor

I think we should stabilize this as soon as its implemented; I'd expect statics and consts to both be irrefutable pattern positions. I don't think its important to keep statics and consts the same; I think its already inconsistent that they don't take irrefutable patterns, and bringing one of them closer to that now is better than waiting until they are both at feature parity and moving them together. In particular, unnamed statics have real semantic questions as to whether the place gets created or not.

@petrochenkov
Copy link
Contributor

If the _ is interpreted as a pattern, and not as a unique identifier like in use foo as _;, then the implementation needs to be changed to not add the names of _ consts into their parent modules.

@RalfJung
Copy link
Member

RalfJung commented May 16, 2019

let _ = is a weird beast, so the following sticks out from the RFC:

Just like let _, this doesn't introduce any new bindings, but still evaluates the rvalue at compile time like any other constant.

That's not "just like let _". let _ evaluates the place but never performs the place-to-rvalue coercion. Specifically, let _ = *v does not access memory, and let _ = mutex.lock().unwrap() does not drop the guard.

@earthengine
Copy link

It is unfortunate that Rust was designed that way and we do not have a way back. It is really confusing to even experienced users unless they jump into the very deep level of the language.

If this feature could ever be designed again, I would hope that let _ = ... to always performs the place-to-rvalue coercion unless it is no-op, so we have less weirdness.

That's not "just like let _". let _ evaluates the place but never performs the place-to-rvalue coercion. Specifically, let _ = *v does not access memory, and let _ = mutex.lock().unwrap() does not drop the guard.

@nox
Copy link
Contributor

nox commented May 17, 2019

Specifically, let _ = *v does not access memory, and let _ = mutex.lock().unwrap() does not drop the guard.

Did you mean that it does drop the guard?

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=94047cd4bd5f9e822ad19bdd55c15291

@RalfJung
Copy link
Member

Dang, it got me again... this does not drop though:

fn main() {
    let mutex = Mutex::new(());
    let val = mutex.lock().unwrap();
    let _ = val; // does not drop nor move.
    let _ = mutex.lock().unwrap();
}

@eddyb
Copy link
Member

eddyb commented May 22, 2019

That's not "just like let _". let _ evaluates the place but never performs the place-to-rvalue coercion. Specifically, let _ = *v does not access memory, and let _ = mutex.lock().unwrap() does not drop the guard.

I think the confusion here is the direction of the conversion. Matching against a pattern always does a "value to place" conversion, as patterns require places to do discriminant checks and field accesses, so:

  • let _ = place_expr; evaluates but doesn't access the place, so it's close to &raw place_expr
  • let _ = value_expr; evaluates the value, puts it in a temporary place, which gets dropped, so identical in behavior to drop(value_expr);, value_expr; or many other uses of a value expression where the value gets dropped - including &raw place_expr;!

So depending on some exact details around unsafe code and whatnot, let _ = e; might be equivalent to &raw e; in all cases (ignoring optimizations, or assuming unused address-of are ignored).

@Centril
Copy link
Contributor Author

Centril commented May 23, 2019

If the _ is interpreted as a pattern, and not as a unique identifier like in use foo as _;, then the implementation needs to be changed to not add the names of _ consts into their parent modules.

I think it would be good to fix this so that we treat const _: T = expr; as we do with extern { .. } and foo!(); in an item position right now. That is, it doesn't have a name, and we don't gensym. That might perhaps involve having Option<Ident> in the hir::Item.

@eddyb
Copy link
Member

eddyb commented May 24, 2019

You don't need Option because extern {...} and foo!(); already work with ident: Ident in {ast,hir}::Item (via the "Invalid" "keyword", aka Symbol::intern("")).

@Centril
Copy link
Contributor Author

Centril commented May 29, 2019

@petrochenkov Are there any observable differences between _ as PatKind::Wild and using gensyms as it is implemented right now (aside from rustdoc which isn't important here) such that it matters wrt. stability and forward compatibility?

@petrochenkov
Copy link
Contributor

Are there any observable differences between _ as PatKind::Wild and using gensyms as it is implemented right now

No, there should be no observable difference after #56392.
They just create some extra work for the resolver and also encoder/decoder, I think.

@Centril
Copy link
Contributor Author

Centril commented May 30, 2019

We discussed this on the language team meeting today and gave the green light for a stabilization report and PR (see above). I will be writing the report and adding it to the PR within a few hours.

bors bot added a commit to rust-lang/rust-analyzer that referenced this issue Sep 15, 2019
1847: Allow an underscore as the identifier in `const` items r=matklad a=ecstatic-morse

[RFC 2526](rust-lang/rust#54912) was recently stabilized, meaning `const _: i32 = 5;` is now a valid item.

Co-authored-by: Dylan MacKenzie <ecstaticmorse@gmail.com>
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 6, 2020
Omit underscore constants from rustdoc

Underscore constants from rust-lang/rfcs#2526 / rust-lang#54912 do not correspond to a nameable item and so are never useful in documentation.
<br>

#### Before:

> &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<img src="https://user-images.githubusercontent.com/1940490/71771409-0427cc80-2eef-11ea-8b7d-d9c74a873e7e.png" width="60%">

#### After:

> Not that.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 7, 2020
Omit underscore constants from rustdoc

Underscore constants from rust-lang/rfcs#2526 / rust-lang#54912 do not correspond to a nameable item and so are never useful in documentation.
<br>

#### Before:

> &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<img src="https://user-images.githubusercontent.com/1940490/71771409-0427cc80-2eef-11ea-8b7d-d9c74a873e7e.png" width="60%">

#### After:

> Not that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.