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

Avoid a bunch of booleans in favor of Result<(), ErrorGuaranteed> as that more robustly proves that an error has been emitted #126317

Merged
merged 6 commits into from
Jun 12, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 12, 2024

pulled out of #126316

This PR cannot have any effect on compilation.
All it does is shift a Ty::new_misc_error to a span_delayed_bug and preserve the ErrorGuaranteed in all other cases

@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Jun 12, 2024
@workingjubilee
Copy link
Member

This PR cannot have any effect on compilation.

hm, if so then
@bors rollup

if segment.ident.name == kw::Empty {
span_bug!(rcvr.span, "empty method name")
} else {
match self.report_method_error(expr.hir_id, rcvr_t, error, expected, false) {
Copy link
Member

@compiler-errors compiler-errors Jun 12, 2024

Choose a reason for hiding this comment

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

please make report_method_error return an ErrorGuaranteed. there's no reason to return Result<Diag, ErrorGuaranteed> when the only two callsites just call diag.emit().

};
if has_error {
if let Err(guar) = has_error {
let err_inputs = self.err_args(args_no_rcvr.len());
Copy link
Member

Choose a reason for hiding this comment

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

make err_args take the ErrorGuaranteed

@@ -576,7 +573,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
no_match_data: &mut NoMatchData<'tcx>,
expected: Expectation<'tcx>,
trait_missing_method: bool,
) -> Option<Diag<'_>> {
) -> Result<Diag<'_>, ErrorGuaranteed> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
) -> Result<Diag<'_>, ErrorGuaranteed> {
) -> ErrorGuaranteed {

@@ -608,24 +605,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

// We could pass the file for long types into these two, but it isn't strictly necessary
// given how targeted they are.
if self.suggest_wrapping_range_with_parens(
self.suggest_wrapping_range_with_parens(
Copy link
Member

Choose a reason for hiding this comment

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

These probably shouldn't be named suggest_*, since they're emitting their own error. Could you think of a better name for this?

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me when ci is green

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 12, 2024

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jun 12, 2024

📌 Commit 6299071 has been approved by compiler-errors

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 Jun 12, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
…mpiler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125869 (Add `target_env = "p1"` to the `wasm32-wasip1` target)
 - rust-lang#126019 (Add TODO comment to unsafe env modification)
 - rust-lang#126036 (Migrate `run-make/short-ice` to `rmake`)
 - rust-lang#126276 (Detect pub structs never constructed even though they impl pub trait with assoc constants)
 - rust-lang#126282 (Ensure self-contained linker is only enabled on dev/nightly )
 - rust-lang#126317 (Avoid a bunch of booleans in favor of Result<(), ErrorGuaranteed> as that more robustly proves that an error has been emitted)
 - rust-lang#126324 (Adjust LoongArch64 data layouts for LLVM update)
 - rust-lang#126340 (Fix outdated predacates_of.rs comments)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7276f34 into rust-lang:master Jun 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
Rollup merge of rust-lang#126317 - oli-obk:recursive_rpit4, r=compiler-errors

Avoid a bunch of booleans in favor of Result<(), ErrorGuaranteed> as that more robustly proves that an error has been emitted

pulled out of rust-lang#126316

This PR cannot have any effect on compilation.
All it does is shift a `Ty::new_misc_error` to a `span_delayed_bug` and preserve the `ErrorGuaranteed` in all other cases
@@ -101,13 +103,19 @@ pub fn init_logger(cfg: LoggerConfig) -> Result<(), Error> {
Err(_) => false,
};

let lines = match cfg.lines {
Ok(v) => &v == "1",
Err(_) => false,
Copy link
Member

Choose a reason for hiding this comment

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

I think a surprising side-effect of this change was to turn off the lines by default?

Even when they are turned on, they look very different... much harder than before to just "move up" and find earlier things on the same level of nesting, since the line is now just a snake that moves left and right with the indentation, it no longer properly shows what nesting level we are getting back to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lines were already turned off in an earlier PR by someone else because they broke code folding which allowed you to fold entire sub-spans. After I switched tracing-tree to use the snaky line, you get both the lines (which I find useful) and the code folding, so I made it opt-in. Locally I was replacing | manually with spaces in the last year just to get code folding in IDEs.

Copy link
Member

Choose a reason for hiding this comment

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

What's "code folding"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you hover over the line numbers in vscode, folding thingies like for directories show up. These are indentation based I think, but in code they may be squirly bracket based

@oli-obk oli-obk deleted the recursive_rpit4 branch August 10, 2024 20:07
@RalfJung
Copy link
Member

RalfJung commented Aug 11, 2024 via email

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

7 participants