Skip to content

Convert debug_assert! to assert! in Binder::dummy #86867

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
Merged
merged 1 commit into from
Jul 5, 2021
Merged

Convert debug_assert! to assert! in Binder::dummy #86867

merged 1 commit into from
Jul 5, 2021

Conversation

JohnTitor
Copy link
Member

This is needed for #85350 not to be passed.
r? @jackh726

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2021
@jackh726
Copy link
Member

jackh726 commented Jul 4, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 4, 2021
@bors
Copy link
Collaborator

bors commented Jul 4, 2021

⌛ Trying commit 93882e4 with merge faffbef7d36114148cbd40b384c2bf707d799961...

@bors
Copy link
Collaborator

bors commented Jul 4, 2021

☀️ Try build successful - checks-actions
Build commit: faffbef7d36114148cbd40b384c2bf707d799961 (faffbef7d36114148cbd40b384c2bf707d799961)

@rust-timer
Copy link
Collaborator

Queued faffbef7d36114148cbd40b384c2bf707d799961 with parent 308fc23, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (faffbef7d36114148cbd40b384c2bf707d799961): comparison url.

Summary: This benchmark run did not return any significant changes.

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 4, 2021
@jackh726
Copy link
Member

jackh726 commented Jul 4, 2021

Okay, mostly perf neutral. Maybe a very slight regression, but not really significant.

Given that this prevents potential future bugs, @bors r+

@bors
Copy link
Collaborator

bors commented Jul 4, 2021

📌 Commit 93882e4 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 4, 2021
@klensy
Copy link
Contributor

klensy commented Jul 4, 2021 •
edited
Loading

Perf's max-rss green for most tests, why this changed?

I mean, changing debug_assert to assert gives positive effect, if this perf results not a random.

@jyn514
Copy link
Member

jyn514 commented Jul 5, 2021

@klensy probably the compiler happened to pick different CGU partitioning

@bors
Copy link
Collaborator

bors commented Jul 5, 2021

⌛ Testing commit 93882e4 with merge 44860d1...

@bors
Copy link
Collaborator

bors commented Jul 5, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 5, 2021
@bors bors merged commit 44860d1 into rust-lang:master Jul 5, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 5, 2021
@JohnTitor JohnTitor deleted the convert-to-actual-assert branch July 5, 2021 11:35
@flip1995
Copy link
Member

@jackh726 @JohnTitor With this change, we started to see ICEs in Clippy. E.g. #7447 and this change.

In both cases this ICE came from calling cx.tcx.layout_of(ty) without checking if ty.has_escaping_bound_vars(). Is the layout_of thing a red herring? If this can only come from calling layout_of then my fix would be to just call ty.has_escaping_bound_vars() before calling layout_of in Clippy everytime.

This was referenced Jul 16, 2021
Merged
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2021
@bors
Auto merge of rust-lang#87271 - flip1995:clippyup, r=Manishearth
fad295b 
Update Clippy

This is an out-of-cycle Clippy update, to fix 3 ICEs before the release (This should be merged before beta is branched):

rust-lang/rust-clippy#7470
rust-lang/rust-clippy#7471
rust-lang/rust-clippy#7473

cc `@jackh726` `@JohnTitor` rust-lang/rust-clippy#7470 was caused by rust-lang#86867. I saw the same ICE in the last rustup for Clippy though, so this might be a more general problem. Is there something we should check before calling `layout_of`? Should we always check for `ty.has_escaping_bound_vars()` before calling `layout_of`? Or is this overkill?

r? `@Manishearth`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
9 participants
@JohnTitor @jackh726 @rust-timer @bors @klensy @jyn514 @flip1995 @rust-highfive @rustbot