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

Delayed bug audit #121285

Merged
merged 7 commits into from
Feb 27, 2024
Merged

Delayed bug audit #121285

merged 7 commits into from
Feb 27, 2024

Conversation

nnethercote
Copy link
Contributor

I went through all the calls to delayed_bug and span_delayed_bug and found a few places where they could be avoided.

r? @compiler-errors

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2024

Some changes occurred in match checking

cc @Nadrieril

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 19, 2024
@bors
Copy link
Contributor

bors commented Feb 20, 2024

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

@nnethercote
Copy link
Contributor Author

I rebased.

@bors
Copy link
Contributor

bors commented Feb 21, 2024

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

@compiler-errors
Copy link
Member

Sorry, I'm kind of swamped and don't currently have the patience to review this as closely as it deserves

r? compiler

@nnethercote
Copy link
Contributor Author

r? @lcnr

@rustbot rustbot assigned lcnr and unassigned wesleywiser Feb 21, 2024
Comment on lines 314 to 321
let result: Result<_, ErrorGuaranteed> = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
.fully_perform(self.infcx, span)
else {
.fully_perform(self.infcx, span);
let Ok(TypeOpOutput { output: norm_ty, constraints: c, .. }) = result else {
// Note: this path is currently not reached in any test, so
// any example that triggers this would be worth minimizing
// and converting into a test.
tcx.dcx().span_delayed_bug(span, format!("failed to normalize {ty:?}"));
continue;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit message explains it:

The existing code calls a function that returns `Result<_,
ErrorGuaranteed>`, and then calls `span_delayed_bug` pointlessly in the
`Err` case.

The comment was added since I first wrote this, I will remove it. Or I could instead insert a span_bug here.

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/adt.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/ident.rs Outdated Show resolved Hide resolved
The existing code calls a function that returns `Result<_,
ErrorGuaranteed>`, and then calls `span_delayed_bug` pointlessly in the
`Err` case.
I find the function much easier to read this way. Thanks to @kadiwa4 for
the suggestion.
…_ribs`.

`Resolver::report_error` always emits (this commit makes that clearer),
so the `span_delayed_bug` is unnecessary.
By returning error guarantees from a few functions it relies on.
By storing error guarantees in `RegionErrors`.
To make them more concise and similar to each other.
@nnethercote
Copy link
Contributor Author

New code is up, hopefully it addresses all the comments.

@lcnr
Copy link
Contributor

lcnr commented Feb 27, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Feb 27, 2024

📌 Commit a8a486c has been approved by lcnr

It is now in the queue for this repository.

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

bors commented Feb 27, 2024

⌛ Testing commit a8a486c with merge 9afdb8d...

@bors
Copy link
Contributor

bors commented Feb 27, 2024

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 9afdb8d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 27, 2024
@bors bors merged commit 9afdb8d into rust-lang:master Feb 27, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 27, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9afdb8d): comparison URL.

Overall result: ✅ improvements - 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
Improvements ✅
(primary)
-0.8% [-1.0%, -0.7%] 5
Improvements ✅
(secondary)
-0.4% [-0.6%, -0.2%] 24
All ❌✅ (primary) -0.8% [-1.0%, -0.7%] 5

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.8% [3.4%, 4.2%] 2
Improvements ✅
(primary)
-2.0% [-2.0%, -2.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.0% [-2.0%, -2.0%] 1

Cycles

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

Binary size

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

Bootstrap: 651.401s -> 650.65s (-0.12%)
Artifact size: 311.13 MiB -> 311.12 MiB (-0.00%)

@nnethercote nnethercote deleted the delayed_bug-audit branch February 27, 2024 20:54
@nnethercote
Copy link
Contributor Author

This wasn't expected to have any effect on performance. I suspect the (good) results are just noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

8 participants