[WIP] borrowck diagnostic migration with eager booted by AndyJado · Pull Request #104941 · rust-lang/rust

AndyJado · 2022-11-26T14:21:04Z

rustbot · 2022-11-26T14:21:12Z

(rustbot has picked a reviewer for you, use r? to override)

AndyJado · 2022-11-26T14:21:33Z

compiler/rustc_borrowck/src/session_diagnostics.rs

Support eager subdiagnostics again See rust-lang#104941 (comment) I'm not sure how to add a test for this. But I did pick some of the diagnostic structs in the mentioned PR and it works with them.

davidtwco · 2023-01-10T09:48:17Z

I'll look at this after we land #104055

Support eager subdiagnostics again See rust-lang#104941 (comment) I'm not sure how to add a test for this. But I did pick some of the diagnostic structs in the mentioned PR and it works with them.

Support eager subdiagnostics again See rust-lang/rust#104941 (comment) I'm not sure how to add a test for this. But I did pick some of the diagnostic structs in the mentioned PR and it works with them.

AndyJado · 2023-10-12T14:23:15Z

I'm back! I'll get the engine running in a few days.

AndyJado · 2023-10-17T06:14:23Z

tests/ui/span/borrowck-borrow-overloaded-auto-deref-mut.stderr

@davidtwco I suppose this is a need eager on Diagonostic (not Subdiag) case, I've made all my attempts and I can't pass ui test，they are like whack-a-mole

davidtwco

Left some more comments, you'll need to look at where the backticks are being added and make sure that we either only do them in the IntoDiagnosticArg implementations or in the Fluent messages.

davidtwco · 2023-11-04T02:59:10Z

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

+        debug!(
+            "report_mutability_error: item_msg and reason is moved to a struct due to diagnosing migration: [E0594] & [E0596], now they are typed value: {:?}",
+            &diagnostic
+        );


You don't need to keep these debug logs if they aren't necessary anymore.

davidtwco · 2023-11-04T02:59:39Z

compiler/rustc_borrowck/src/borrowck_errors.rs

@@ -1,9 +1,20 @@
+// #![deny(rustc::untranslatable_diagnostic)]
+// #![deny(rustc::diagnostic_outside_of_impl)]


This comment still seems relevant.

davidtwco · 2023-11-04T03:01:29Z

compiler/rustc_borrowck/src/diagnostics/mod.rs

        }
    }

+    pub(super) fn describe_place_typed(&self, place_ref: PlaceRef<'tcx>) -> DescribedPlace {


I presume this exists because changing describe_place to return a DescribedPlace` isn't practical?

describe_place is used a lot, I'm currently narrowing the scope so it covers only PlaceAndReason cases.

davidtwco · 2023-11-04T03:02:53Z

compiler/rustc_borrowck/src/diagnostics/mod.rs

+    fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> {
+        match self.0 {
+            Some(descr) => descr.into_diagnostic_arg(),
+            None => "value".into_diagnostic_arg(),


Maybe bug! here?

davidtwco · 2023-11-04T03:03:21Z

compiler/rustc_borrowck/src/diagnostics/mod.rs

 pub(super) struct IncludingTupleField(pub(super) bool);

+#[derive(Debug)]
+pub(super) struct DescribedPlace(pub(super) Option<String>);


Why is this necessary if self.0 already implements IntoDiagnosticArg?

davidtwco · 2023-11-04T03:05:18Z

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

-                    }
+                    let name = ptr_source.describe_for_immutable_place(self.infcx.tcx);
+                    diagnostic = PlaceAndReason::BehindPointer(
+                        self.describe_place_typed(access_place.as_ref()),


Ah, so the "value" case in DescribedPlace is to handle the situation of "data" here?

AndyJado · 2023-11-07T02:44:15Z

I tracked function emit_diagnostic() and located where backticks are added, and my assumption is that it happens in some macro.

    fn emit_diagnostic(&mut self, diagnostic: &mut Diagnostic) -> Option<ErrorGuaranteed> {
        dbg!(&diagnostic.args().collect::<Vec<_>>());

I was runing a single UI test.

RUST_LOG=[emit_diagnostic] ./x.py test tests/ui/issues/issue-21600.rs

Here's the output diff:

+       [compiler/rustc_errors/src/lib.rs:1326] &diagnostic.args().collect::<Vec<_>>() = [
+           (
+               "place",
+               Str(
+                   "x",
+               ),
+           ),
+       ]
1       error[E0596]: cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure
2         --> $DIR/issue-21600.rs:14:20
3          |

10         |         |       in this closure
11         |         expects `Fn` instead of `FnMut`
12
-       error[E0596]: cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure
+       [compiler/rustc_errors/src/lib.rs:1326] &diagnostic.args().collect::<Vec<_>>() = []
+       [compiler/rustc_errors/src/lib.rs:1326] &diagnostic.args().collect::<Vec<_>>() = [
+           (
+               "place",
+               Str(
+                   "`x`",
+               ),
+           ),
+       ]
+       error[E0596]: cannot borrow ``x`` as mutable, as it is a captured variable in a `Fn` closure

bors · 2023-11-26T23:53:46Z

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

Dylan-DPC · 2024-02-05T17:38:13Z

@AndyJado any updates on this? thanks

AndyJado · 2024-02-23T12:31:15Z

cannot_assign and cannot_borrow used to be string interpolation in one big function, after split them into structs E0594 and E0596, backtick repeats on some place description:

- error[E0596]: cannot borrow `r` as mutable
+ error[E0596]: cannot borrow ``r`` as mutable

I believe the answer lies in the macro implementation but I don't have the guts diving in. @Dylan-DPC

I'll keep conflict resolved.

bors · 2024-02-28T23:10:41Z

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

bors · 2024-03-08T20:48:18Z

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

rust-log-analyzer · 2024-03-09T06:49:01Z

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=AndyJado
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_2d9c9fb6-2530-44fc-a6c4-ad389920f053
GITHUB_EVENT_NAME=pull_request
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=2fd0b77025832c7006ee5c49548317a733185049
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_2d9c9fb6-2530-44fc-a6c4-ad389920f053
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_2d9c9fb6-2530-44fc-a6c4-ad389920f053
GITHUB_TRIGGERING_ACTOR=AndyJado
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/104941/merge
GITHUB_WORKFLOW_SHA=2fd0b77025832c7006ee5c49548317a733185049
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_20_X64=/opt/hostedtoolcache/go/1.20.14/x64
---

error: diagnostics should only be created in `IntoDiagnostic`/`AddToDiagnostic` impls
   --> compiler/rustc_borrowck/src/diagnostics/mod.rs:611:17
    |
611 |             err.subdiagnostic(dcx, f(args_span));

error: diagnostics should only be created in `IntoDiagnostic`/`AddToDiagnostic` impls
   --> compiler/rustc_borrowck/src/diagnostics/mod.rs:628:25
    |
---

error: diagnostics should only be created in `IntoDiagnostic`/`AddToDiagnostic` impls
   --> compiler/rustc_borrowck/src/diagnostics/mod.rs:681:17
    |
681 |             err.subdiagnostic(dcx, diag);

error: diagnostics should only be created in `IntoDiagnostic`/`AddToDiagnostic` impls
    --> compiler/rustc_borrowck/src/diagnostics/mod.rs:1053:25
     |
     |
1053 |                     err.subdiagnostic(
     |                         ^^^^^^^^^^^^^

error: diagnostics should only be created in `IntoDiagnostic`/`AddToDiagnostic` impls
    --> compiler/rustc_borrowck/src/diagnostics/mod.rs:1062:25
     |
1062 |                     err.subdiagnostic(self.dcx(), CaptureReasonNote::FnOnceMoveInCall { var_span });

error: diagnostics should only be created in `IntoDiagnostic`/`AddToDiagnostic` impls
    --> compiler/rustc_borrowck/src/diagnostics/mod.rs:1066:25
     |

bors · 2024-03-16T00:03:22Z

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

JohnCSimon · 2024-05-26T23:03:23Z

@AndyJado
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned compiler-errors Nov 26, 2022

rustbot assigned davidtwco and unassigned compiler-errors Nov 26, 2022

This comment was marked as resolved.

Sign in to view

AndyJado force-pushed the eager branch from 14b2fee to 8229c6a Compare November 30, 2022 05:40

This comment was marked as resolved.

Sign in to view

AndyJado commented Dec 16, 2022

View reviewed changes

compiler/rustc_borrowck/src/session_diagnostics.rs Outdated Show resolved Hide resolved

mejrs mentioned this pull request Dec 17, 2022

Support eager subdiagnostics again #105806

Merged

davidtwco added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2023

AndyJado force-pushed the eager branch from 8229c6a to bdcfebd Compare April 28, 2023 08:59

This comment was marked as resolved.

Sign in to view

AndyJado force-pushed the eager branch from bdcfebd to 5a4342d Compare May 5, 2023 00:28

This comment was marked as resolved.

Sign in to view

AndyJado force-pushed the eager branch from 5a4342d to ce2a2d3 Compare May 25, 2023 05:34

AndyJado marked this pull request as ready for review May 25, 2023 06:22

This comment was marked as resolved.

Sign in to view

AndyJado force-pushed the eager branch from ce2a2d3 to 6bc23ec Compare May 26, 2023 07:51