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

Introduce const Trait (always-const trait bounds) #119099

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

fmease
Copy link
Member

@fmease fmease commented Dec 18, 2023

Feature const_trait_impl currently lacks a way to express “always const” trait bounds. This makes it impossible to define generic items like fns or structs which contain types that depend on const method calls (*). While the final design and esp. the syntax of effects / keyword generics isn't set in stone, some version of “always const” trait bounds will very likely form a part of it. Further, their implementation is trivial thanks to the effects backbone.

Not sure if this needs t-lang sign-off though.

(*):

#![feature(const_trait_impl, effects, generic_const_exprs)]

fn compute<T: const Trait>() -> Type<{ T::generate() }> { /*…*/ }

struct Store<T: const Trait>
where
    Type<{ T::generate() }>:,
{
    field: Type<{ T::generate() }>,
}

Lastly, “always const” trait bounds are a perfect fit for generic_const_items.

#![feature(const_trait_impl, effects, generic_const_items)]

const DEFAULT<T: const Default>: T = T::default();

Previously, we (oli, fee1-dead and I) wanted to reinterpret ~const Trait as const Trait in generic const items which would've been quite surprising and not very generalizable.
Supersedes #117530.


cc @oli-obk

As discussed
r? fee1-dead (or compiler)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 18, 2023
@fmease fmease added F-const_trait_impl `#![feature(const_trait_impl)]` F-effects `#![feature(effects)]` labels Dec 18, 2023
@rust-log-analyzer

This comment has been minimized.

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
Comment on lines +2618 to +2611
let id = lcx.next_node_id();
let hir_id = lcx.next_id();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this {Node,Hir}Id creation below the HirId registration for the hir::ExprKind::Path above to accommodate the control flow, I hope that doesn't break any invariants (?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as long as you didn't move it outside of a nested self.with_* function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... hm... it's probably fine...

compiler/rustc_ast_passes/src/ast_validation.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/mod.rs Show resolved Hide resolved
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have tests to check that we deny ~const in const items and structs and stuff?

@fmease
Copy link
Member Author

fmease commented Dec 18, 2023

Do we have tests to check that we deny ~const in const items and structs and stuff?

Yes, we do: https://github.com/rust-lang/rust/blob/master/tests/ui/rfcs/rfc-2632-const-trait-impl/tilde-const-invalid-places.rs

@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Dec 19, 2023
@fmease fmease marked this pull request as ready for review December 19, 2023 11:57
@fmease fmease removed the A-rustdoc-json Area: Rustdoc JSON backend label Dec 19, 2023
@@ -786,6 +794,7 @@ impl<'a> Parser<'a> {
|| self.check(&token::Not)
|| self.check(&token::Question)
|| self.check(&token::Tilde)
|| self.check_keyword(kw::Const)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to investigate if this visibly changes the MBE matching behavior for stable users. I hope not since this check is required for correctness.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem to be a problem. I've added a test to ensure that const Trait doesn't regress stable code: tests/ui/rfcs/rfc-2632-const-trait-impl/mbe-bare-trait-objects-const-trait-bounds.rs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's obvious in hindsight but this doesn't regress the aforementioned test because I haven't (and I won't) add kw::Const & TokenKind::Tilde to can_begin_type.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Dec 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the always-const-trait-bounds branch 4 times, most recently from 310df46 to 2abd167 Compare December 19, 2023 19:52
@bors
Copy link
Contributor

bors commented Dec 22, 2023

☔ The latest upstream changes (presumably #119163) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
compiler/rustc_ast/src/token.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/item.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Show resolved Hide resolved
compiler/rustc_hir_analysis/messages.ftl Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
Comment on lines 847 to 851
self.sess.emit_err(errors::ModifierLifetime {
span,
modifier: "const",
padding: " ",
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not see an ui test for the padding suggestion. Could you give me a link to it or add it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, padding didn't actually make sense. I've removed it. I could run-rustfix for the -Zparse-only test tests/ui/parser/bounds-type.rs but I'm not sure if it's worth it esp. since it concerns a maybe-incorrect suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for run-rustfix, just an ui test that would have covered this code path with stderr would be fine. Though removing it also works.

Copy link
Member Author

@fmease fmease Dec 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tool-only suggestion atm, hence it doesn't show up on stderr.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 23, 2023
…=compiler-errors

Clean up `check_consts` and misc fixes

1. Remove most of the logic around erroring with trait methods. I have kept the part resolving it to a concrete impl, as that is used for const stability checks.
2. Turning on `effects` causes ICE with generic args, due to `~const Tr` when `Tr` is not `#[const_trait]` tripping up expectation in code that handles generic args, more specifically here:
https://github.com/rust-lang/rust/blob/8681e077b8afa99d60acf8f8470a012a3ce709a5/compiler/rustc_hir_analysis/src/astconv/generics.rs#L377

We set `arg_count.correct` to `Err` to correctly signal that an error has already been reported.

3. UI test blesses.

Edit(fmease): Fixes rust-lang#117244 (UI test is in rust-lang#119099 for now).

r? compiler-errors
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2023
…ompiler-errors

Clean up `check_consts` and misc fixes

1. Remove most of the logic around erroring with trait methods. I have kept the part resolving it to a concrete impl, as that is used for const stability checks.
2. Turning on `effects` causes ICE with generic args, due to `~const Tr` when `Tr` is not `#[const_trait]` tripping up expectation in code that handles generic args, more specifically here:
https://github.com/rust-lang/rust/blob/8681e077b8afa99d60acf8f8470a012a3ce709a5/compiler/rustc_hir_analysis/src/astconv/generics.rs#L377

We set `arg_count.correct` to `Err` to correctly signal that an error has already been reported.

3. UI test blesses.

Edit(fmease): Fixes rust-lang#117244 (UI test is in rust-lang#119099 for now).

r? compiler-errors
@bors
Copy link
Contributor

bors commented Dec 27, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 27, 2023
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@fmease
Copy link
Member Author

fmease commented Dec 27, 2023

Network error
@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2023
@bors
Copy link
Contributor

bors commented Dec 27, 2023

⌛ Testing commit 3eb48a3 with merge 88d69b7...

@bors
Copy link
Contributor

bors commented Dec 27, 2023

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 88d69b7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 27, 2023
@bors bors merged commit 88d69b7 into rust-lang:master Dec 27, 2023
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Dec 27, 2023
@fmease fmease deleted the always-const-trait-bounds branch December 27, 2023 21:33
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (88d69b7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.3% [0.1%, 0.5%] 2
Regressions ❌
(secondary)
0.9% [0.9%, 0.9%] 2
Improvements ✅
(primary)
-0.8% [-0.8%, -0.8%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.8%, 0.5%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.5%, 0.8%] 3
Regressions ❌
(secondary)
1.1% [1.1%, 1.1%] 1
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [-0.5%, 0.8%] 4

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 670.98s -> 672.366s (0.21%)
Artifact size: 312.28 MiB -> 312.33 MiB (0.02%)

fmease added a commit to fmease/rust that referenced this pull request Jan 2, 2024
…, r=compiler-errors

Pretty-print always-const trait predicates correctly

Follow-up to rust-lang#119099.

r? fee1-dead
fmease added a commit to fmease/rust that referenced this pull request Jan 3, 2024
…, r=compiler-errors

Pretty-print always-const trait predicates correctly

Follow-up to rust-lang#119099.

r? fee1-dead
fmease added a commit to fmease/rust that referenced this pull request Jan 3, 2024
…, r=compiler-errors

Pretty-print always-const trait predicates correctly

Follow-up to rust-lang#119099.

r? fee1-dead
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 3, 2024
Rollup merge of rust-lang#119476 - fmease:pp-always-const-trait-preds, r=compiler-errors

Pretty-print always-const trait predicates correctly

Follow-up to rust-lang#119099.

r? fee1-dead
@fmease fmease removed the A-rustdoc-json Area: Rustdoc JSON backend label Jan 7, 2024
@ytmimi
Copy link
Contributor

ytmimi commented Jan 21, 2024

@fmease @compiler-errors @fee1-dead I'm hoping I can get some help. I've bisected rust-lang/rustfmt#6035 back to this PR.

With the following input: m!(const N: usize = 0;);

rustfmt 1.7.0-dev (88d69b7 2023-12-27) outputs:

m!(const N: usize = 0;);

but the commit just before (rustfmt 1.7.0-dev (a861c89 2023-12-27)) outputs:

m!(
    const N: usize = 0;
);

Any idea why the formatting would have changed?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…=calebcartwright,ytmimi

rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg

r? `@ytmimi` and/or `@calebcartwright`
cc `@fmease`

I'm putting this on r-l/rust since it should fix the nightly rustfmt version. If you don't care about having this regression until the next rustfmt->rust sync, then I can move that PR over to r-l/rustfmt.

---

> Any idea why the formatting would have changed [from rust-lang#119099]?

**Copied over explanation:**

This has to do with the weirdness of the way that `parse_macro_arg` works. Unlike parsing nonterminal args in a macro-by-example, it eagerly tries, for example, to parse a type without checking that the beginning token may begin a type:

https://github.com/rust-lang/rustfmt/blob/bf967319e258acb9b1648a952bba52665eceaf52/src/parse/macros/mod.rs#L54

Contrast this to the nonterminal parsing code, which first checks that the nonterminal may begin with a given token:

https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L47

In rust-lang#119099, `@fmease` implemented a change so that `const Tr` would be parsed as `dyn const Tr` (a trait object to a const trait) in edition 2015.

This is okay for the purposes of macros, because he explicitly made sure that `const` did not get added to the list of tokens that may begin a `:ty` nonterminal kind: rust-lang#119099 (comment)

However, since rustfmt is not so careful about eagerly parsing macro args before checking that they're legal in macro position, this changed the way that the string of tokens is being parsed into macro args.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…=calebcartwright,ytmimi

rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg

r? ``@ytmimi`` and/or ``@calebcartwright``
cc ``@fmease``

I'm putting this on r-l/rust since it should fix the nightly rustfmt version. If you don't care about having this regression until the next rustfmt->rust sync, then I can move that PR over to r-l/rustfmt.

---

> Any idea why the formatting would have changed [from rust-lang#119099]?

**Copied over explanation:**

This has to do with the weirdness of the way that `parse_macro_arg` works. Unlike parsing nonterminal args in a macro-by-example, it eagerly tries, for example, to parse a type without checking that the beginning token may begin a type:

https://github.com/rust-lang/rustfmt/blob/bf967319e258acb9b1648a952bba52665eceaf52/src/parse/macros/mod.rs#L54

Contrast this to the nonterminal parsing code, which first checks that the nonterminal may begin with a given token:

https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L47

In rust-lang#119099, ``@fmease`` implemented a change so that `const Tr` would be parsed as `dyn const Tr` (a trait object to a const trait) in edition 2015.

This is okay for the purposes of macros, because he explicitly made sure that `const` did not get added to the list of tokens that may begin a `:ty` nonterminal kind: rust-lang#119099 (comment)

However, since rustfmt is not so careful about eagerly parsing macro args before checking that they're legal in macro position, this changed the way that the string of tokens is being parsed into macro args.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
Rollup merge of rust-lang#120218 - compiler-errors:parse_macro_arg, r=calebcartwright,ytmimi

rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg

r? ``@ytmimi`` and/or ``@calebcartwright``
cc ``@fmease``

I'm putting this on r-l/rust since it should fix the nightly rustfmt version. If you don't care about having this regression until the next rustfmt->rust sync, then I can move that PR over to r-l/rustfmt.

---

> Any idea why the formatting would have changed [from rust-lang#119099]?

**Copied over explanation:**

This has to do with the weirdness of the way that `parse_macro_arg` works. Unlike parsing nonterminal args in a macro-by-example, it eagerly tries, for example, to parse a type without checking that the beginning token may begin a type:

https://github.com/rust-lang/rustfmt/blob/bf967319e258acb9b1648a952bba52665eceaf52/src/parse/macros/mod.rs#L54

Contrast this to the nonterminal parsing code, which first checks that the nonterminal may begin with a given token:

https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L47

In rust-lang#119099, ``@fmease`` implemented a change so that `const Tr` would be parsed as `dyn const Tr` (a trait object to a const trait) in edition 2015.

This is okay for the purposes of macros, because he explicitly made sure that `const` did not get added to the list of tokens that may begin a `:ty` nonterminal kind: rust-lang#119099 (comment)

However, since rustfmt is not so careful about eagerly parsing macro args before checking that they're legal in macro position, this changed the way that the string of tokens is being parsed into macro args.
bors added a commit to rust-lang/rust-analyzer that referenced this pull request Feb 16, 2024
internal: Parse (nightly) `const` and `async` trait bounds

Both of these bound modifiers were added recently:

* `const` trait bounds: rust-lang/rust#119099
* `async` trait bounds: rust-lang/rust#120392

The latter will certainly will not do the right thing; namely, `async Fn` needs to be mapped to the `AsyncFn` trait. IDK how to do that, so advice would be appreciated, though perhaps we could land this first so the parser isn't complaining about these bounds?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_trait_impl `#![feature(const_trait_impl)]` F-effects `#![feature(effects)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants