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

Move const trait bounds checks to MIR constck #109557

Merged
merged 2 commits into from
Mar 28, 2023
Merged

Move const trait bounds checks to MIR constck #109557

merged 2 commits into from
Mar 28, 2023

Conversation

fee1-dead
Copy link
Member

Fixes #109543. When checking paths in HIR typeck, we don't want to check for const predicates since all we want might just be a function pointer. Therefore we move this to MIR constck and check that bounds are met during MIR constck.

r? @oli-obk

@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 Mar 24, 2023
fee1-dead
fee1-dead commented Mar 24, 2023
View reviewed changes
tests/ui/consts/const-block-const-bound.stderr
Comment on lines -17 to +13
LL | f(&UnconstDrop);
| +
LL | f(&mut UnconstDrop);
| ++++
LL | &f(UnconstDrop);
| +
LL | &mut f(UnconstDrop);
| ++++
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a regression that has something to do with the spans and how trait selection is different in typeck and constck. I will look into this.

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 looks like the span of the ObligationCause is what caused this. Kind of hard to fix since we don't have access to the machinery that typeck can use.

cjgillot
cjgillot reviewed Mar 25, 2023
View reviewed changes
tests/ui/const-generics/issue-93647.stderr
@@ -1,3 +1,17 @@
error[E0277]: the trait bound `[closure@$DIR/issue-93647.rs:2:6: 2:8]: Fn<()>` is not satisfied
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the message say that we expect a const bound here too?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 27, 2023

@bors r+

The diagnostics issues are either not visible on stable or do have a path for fixing them properly after we land #107123

@bors
Copy link
Contributor

bors commented Mar 27, 2023

📌 Commit c02569683951662e5bde9ebc681235e01ed8a168 has been approved by oli-obk

It is now in the queue for this repository.

@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 Mar 27, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Mar 27, 2023

@bors p=1 fixes stable regressions

@oli-obk oli-obk added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Mar 27, 2023
@bors
Copy link
Contributor

bors commented Mar 27, 2023

⌛ Testing commit c02569683951662e5bde9ebc681235e01ed8a168 with merge 6f6f4caf07539ad18b0d0345e1a041635eda3cdf...

@bors
Copy link
Contributor

bors commented Mar 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 Mar 27, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fee1-dead
Copy link
Member Author

@bors retry: spurious

@fee1-dead
Copy link
Member Author

@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 Mar 28, 2023
@bors
Copy link
Contributor

bors commented Mar 28, 2023

⌛ Testing commit c02569683951662e5bde9ebc681235e01ed8a168 with merge 47b10403e4c0c841f3c427479288049d82377ac2...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 28, 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 Mar 28, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Mar 28, 2023

legit tidy failure

@fee1-dead
Move const trait bounds checks to MIR constck
b17e668 
Fixes rust-lang#109543. When checking paths in HIR typeck, we don't want to check
for const predicates since all we want might just be a function pointer.
Therefore we move this to MIR constck and check that bounds are met
during MIR constck.
@fee1-dead
Copy link
Member Author

Fixed

@oli-obk
Copy link
Contributor

oli-obk commented Mar 28, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Mar 28, 2023

📌 Commit 054009d has been approved by oli-obk

It is now in the queue for this repository.

@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 Mar 28, 2023
@bors
Copy link
Contributor

bors commented Mar 28, 2023

⌛ Testing commit 054009d with merge 6066037...

@bors
Copy link
Contributor

bors commented Mar 28, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6066037 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2023
@bors bors merged commit 6066037 into rust-lang:master Mar 28, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 28, 2023
@fee1-dead fee1-dead deleted the mv-const-traits branch March 28, 2023 12:20
@Noratrieb Noratrieb mentioned this pull request Mar 28, 2023
Open
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6066037): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.3% [0.3%, 0.4%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

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
Regressions ❌
(secondary)		 3.5% [3.2%, 3.9%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.7% [-4.7%, -4.7%] 1
All ❌✅ (primary) - - 0

Cycles

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

@cuviper cuviper mentioned this pull request Mar 28, 2023
Closed
2 tasks
@apiraino
Copy link
Contributor

@oli-obk is the stable backport nomination meant for a theoretical 1.68.3 (if it will ever be discussed) or this fix can wait for the next stable (20th, April i.e. ~3 weeks from now)?

@apiraino
Copy link
Contributor

Beta backports declined as per compiler team on Zulip, the rationale being that even removing the ~const bounds seems like too high cost compared to reward for stable.

@rustbot label -beta-nominated -stable-nominated

@rustbot rustbot removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@oli-obk oli-obk

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.

Function pointers break in const code after ~const bounds are added
8 participants
@fee1-dead @oli-obk @bors @rust-log-analyzer @rust-timer @apiraino @cjgillot @rustbot