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.

Sign up for GitHub

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

Jump to bottom

deny using default function in impl const Trait #86571

Merged
merged 2 commits into from
Jul 3, 2021
Merged

deny using default function in impl const Trait #86571

merged 2 commits into from
Jul 3, 2021

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Jun 23, 2021 •
edited
Loading

Fixes #79450.

I don't know if my implementation is correct:

  • The check is in rustc_passes::check_const, should I put it somewhere else instead?
  • Is my approach (to checking the impl) optimal? It works for the current tests, but it might have some issues or there might be a better way of doing this.

@rust-highfive
Copy link
Collaborator

r? @jackh726

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
jackh726
jackh726 requested changes Jun 28, 2021
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

I'm wondering if this would catch super trait functions. Can you add a test for that?

src/test/ui/consts/const-float-classify.rs Show resolved Hide resolved
@fee1-dead
Copy link
Member Author

fee1-dead commented Jun 29, 2021 •
edited
Loading

@rustbot label -S-waiting-on-author +S-waiting-on-review

@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 Jun 29, 2021
jackh726
jackh726 reviewed Jun 29, 2021
View reviewed changes
compiler/rustc_passes/src/check_const.rs
.filter(|it| matches!(it.kind, hir::AssocItemKind::Fn { .. }))
.count();

if trait_fn_cnt != impl_fn_cnt {
Copy link
Member

Choose a reason for hiding this comment

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

Also, this is kind of a hacky way to check if there are any non-implemented default functions. But it might just be the best.

@fee1-dead
Copy link
Member Author

if this would catch super trait functions

I don't understand, because with B: A you must implement A to implement B. But this raises a question, with super traits, does impl const B require impl const A? This is currently not enforced, but it also works because when it is not const, the functions of A would not be recognized as const (I guess).

Is there anything about super traits (and default implementations of functions of those) that it should catch? There is no way for a sub-trait to specify default impls for a super-trait, right?

@fee1-dead
Copy link
Member Author

cc @oli-obk

@fee1-dead fee1-dead mentioned this pull request Jun 30, 2021
Closed
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 30, 2021
@jackh726
Copy link
Member

if this would catch super trait functions

I don't understand, because with B: A you must implement A to implement B. But this raises a question, with super traits, does impl const B require impl const A? This is currently not enforced, but it also works because when it is not const, the functions of A would not be recognized as const (I guess).

Is there anything about super traits (and default implementations of functions of those) that it should catch? There is no way for a sub-trait to specify default impls for a super-trait, right?

Ah, you're right. Not sure about if impl const B should require impl const A; seems orthogonal. For this, I think this is fine except I'd like a comment basically explaining what this code does and why it's required.

@jackh726 jackh726 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 Jun 30, 2021
@fee1-dead
Copy link
Member Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2021
@jackh726
Copy link
Member

jackh726 commented Jul 2, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2021

📌 Commit 5e178b2 has been approved by jackh726

@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 Jul 2, 2021
@fee1-dead fee1-dead mentioned this pull request Jul 3, 2021
Merged
@bors
Copy link
Contributor

bors commented Jul 3, 2021

⌛ Testing commit 5e178b2 with merge 7014963...

@bors
Copy link
Contributor

bors commented Jul 3, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 7014963 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 3, 2021
@bors bors merged commit 7014963 into rust-lang:master Jul 3, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 3, 2021
@bors bors mentioned this pull request Jul 3, 2021
Closed
@fee1-dead fee1-dead deleted the const-trait-impl-fix branch May 12, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 requested changes

@jonas-schievink jonas-schievink jonas-schievink left review comments

Assignees

@jackh726 jackh726

Labels
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.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.

impl const wrongly accepts impl with non-const provided methods
7 participants
@fee1-dead @rust-highfive @rust-log-analyzer @jackh726 @bors @jonas-schievink @rustbot