Replace `#[default_method_body_is_const]` with `#[const_trait]` by oli-obk · Pull Request #96964 · rust-lang/rust

oli-obk · 2022-05-12T07:46:47Z

pulled out of #96077

related issues: #67792 and #92158

This is groundwork to only allowing impl const Trait for traits that are marked with #[const_trait]. This is necessary to prevent adding a new default method from becoming a breaking change (as it could be a non-const fn).

rust-highfive · 2022-05-12T07:46:50Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-05-12T07:46:52Z

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

bors · 2022-05-12T15:51:45Z

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

fee1-dead · 2022-05-16T09:31:53Z

(Please do not r? me because I have written parts of this PR.)

jhpratt

Part of me would like the syntax to require const fn for the default methods, as it's not immediately clear that it's the case with this change. But I understand that the syntax/attributes is largely in flux, so that's not a deal-breaker for me.

Overall, the PR looks good. Just some minor nits.

bors · 2022-05-17T09:40:57Z

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

jhpratt

LGTM pending rebase.

compiler-errors

r=me, left a few comments (or ignore them, that's fine too)

compiler-errors · 2022-05-28T06:06:22Z

I wish if chains could be booleans, sigh

I'm not sure what this means :)

sorry, meant let chains. just don't like how this has to be a mutable assignment lol. not an actionable comment.

…ait can be treated as `const`

…esugar to a method in a new impl block

Co-authored-by: fee1-dead <ent3rm4n@gmail.com>

oli-obk · 2022-05-30T08:54:28Z

@bors r=compiler-errors

bors · 2022-05-30T08:54:31Z

📌 Commit 2f96fbe has been approved by compiler-errors

bors · 2022-05-30T09:19:06Z

⌛ Testing commit 2f96fbe with merge 5c780b9...

bors · 2022-05-30T11:59:49Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5c780b9 to master...

rust-timer · 2022-05-30T13:18:01Z

Finished benchmarking commit (5c780b9): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.2%	0.3%	9
Regressions 😿 (secondary)	0.2%	0.3%	3
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.1%	-1.1%	3
All 😿🎉 (primary)	0.2%	0.3%	9

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.5%	2.5%	2
Regressions 😿 (secondary)	3.6%	3.9%	2
Improvements 🎉 (primary)	-1.3%	-1.3%	1
Improvements 🎉 (secondary)	-2.6%	-3.2%	2
All 😿🎉 (primary)	0.6%	2.5%	3

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	2.2%	2.2%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.2%	2.2%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

the arithmetic mean of the percent change↩↩² ↩³
number of relevant changes↩↩² ↩³

pnkfelix · 2022-06-01T02:44:45Z

The primary regressions are mostly in variants of stm32f4, with a two serde and one syn thrown in for good measure.
The improvements are solely to ctfe stress test, which I guess makes sense given the PR?
Anyway, the regressions seem minor, and they are contained to an unstable feature that the stdlib is using.
Marking as triaged.

@rustbot label: +perf-regression-triaged

fee1-dead · 2022-07-25T05:28:38Z

- let trait_def_id = imp.of_trait.as_ref()?.trait_def_id()?;
- let ancestors = tcx
- .trait_def(trait_def_id)
- .ancestors(tcx, item.def_id.to_def_id())
- .ok()?;
- let mut to_implement = Vec::new();
-
- for trait_item in tcx.associated_items(trait_def_id).in_definition_order()
- {
- if let ty::AssocItem {
- kind: ty::AssocKind::Fn,
- defaultness,
- def_id: trait_item_id,
- ..
- } = *trait_item
- {
- // we can ignore functions that do not have default bodies:
- // if those are unimplemented it will be caught by typeck.
- if !defaultness.has_value()
- || tcx
- .has_attr(trait_item_id, sym::default_method_body_is_const)
- {
- continue;
- }
-
- let is_implemented = ancestors
- .leaf_def(tcx, trait_item_id)
- .map(|node_item| !node_item.defining_node.is_from_trait())
- .unwrap_or(false);
-
- if !is_implemented {
- to_implement.push(trait_item_id);
- }
- }
- }
-
- // all nonconst trait functions (not marked with #[default_method_body_is_const])
- // must be implemented
- if !to_implement.is_empty() {
- let not_implemented = to_implement
- .into_iter()
- .map(|did| tcx.item_name(did).to_string())
- .collect::<Vec<_>>()
- .join("`, `");
- tcx
- .sess
- .struct_span_err(
- item.span,
- "const trait implementations may not use non-const default functions",
- )
- .note(&format!("`{}` not implemented", not_implemented))
- .emit();
- }
- }


I think we forgot to actually forbid impl const on non #[const_trait] which would be here

rust-highfive assigned compiler-errors May 12, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 12, 2022

fee1-dead reviewed May 12, 2022
View reviewed changes

Comment threadcompiler/rustc_passes/src/check_attr.rs Outdated

oli-obk force-pushed the const_trait_mvp branch from ddf548b to e33eae5Compare May 16, 2022 08:26

jhpratt reviewed May 16, 2022
View reviewed changes

Comment threadcompiler/rustc_metadata/src/rmeta/encoder.rs Outdated
Comment threadcompiler/rustc_passes/src/check_attr.rs Outdated

This comment was marked as resolved.
Sign in to view

This comment was marked as off-topic.
Sign in to view

oli-obk force-pushed the const_trait_mvp branch from b38bdb0 to f6115dbCompare May 17, 2022 06:57

jhpratt approved these changes May 17, 2022
View reviewed changes

oli-obk force-pushed the const_trait_mvp branch from f6115db to e31530dCompare May 27, 2022 17:15

compiler-errors approved these changes May 28, 2022
View reviewed changes

fee1-deadand others added 5 commits May 30, 2022 08:52

Remove #[default..] and add #[const_trait]
257f065

Add a helper function for checking whether a default function in a tr…
4d390de
…ait can be treated as `const`

Reduce the scope of a mutable variable
ecaf7b7

Default methods of traits are also AssocFn defs as they essentially d…
0e3d8d2
…esugar to a method in a new impl block

update diagnostic message on removed attribute
2f96fbe
Co-authored-by: fee1-dead <ent3rm4n@gmail.com>

oli-obk force-pushed the const_trait_mvp branch from e31530d to 2f96fbeCompare May 30, 2022 08:52

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 30, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 30, 2022

bors merged commit 5c780b9 into rust-lang:masterMay 30, 2022

rustbot added this to the 1.63.0 milestone May 30, 2022

rustbot added the perf-regression Performance regression. label May 30, 2022

oli-obk deleted the const_trait_mvp branch May 30, 2022 13:34

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 1, 2022

fee1-dead reviewed Jul 25, 2022
View reviewed changes

kadiwa4 mentioned this pull request Jul 25, 2022
impl const wrongly accepts impl with non-const provided methods #79450
Closed

Uh oh!

Uh oh!

Conversation

oli-obk commented May 12, 2022

Uh oh!

rust-highfive commented May 12, 2022

Uh oh!

rust-highfive commented May 12, 2022

Uh oh!

Uh oh!

bors commented May 12, 2022

Uh oh!

fee1-dead commented May 16, 2022

Uh oh!

jhpratt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

bors commented May 17, 2022

Uh oh!

jhpratt left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errorsMay 28, 2022

Choose a reason for hiding this comment

Uh oh!

fee1-deadMay 28, 2022

Choose a reason for hiding this comment

Uh oh!

compiler-errorsMay 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oli-obk commented May 30, 2022

Uh oh!

bors commented May 30, 2022

Uh oh!

bors commented May 30, 2022

Uh oh!

bors commented May 30, 2022

Uh oh!

rust-timer commented May 30, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

pnkfelix commented Jun 1, 2022

Uh oh!

fee1-deadJul 25, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants