Handle auto trait migration #52

roxelo · 2021-04-08T23:13:11Z

This PR adds a new migration fixup to handle semantics change for traits when using capture_disjoint_fields feature

Closes rust-lang/project-rfc-2229#29
Closes rust-lang/project-rfc-2229#28

compiler/rustc_lint_defs/src/builtin.rs

compiler/rustc_typeck/src/check/upvar.rs

arora-aman · 2021-04-08T23:17:03Z

compiler/rustc_typeck/src/check/upvar.rs

@@ -542,19 +606,57 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
        closure_span: Span,
        closure_clause: hir::CaptureBy,
        min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
-    ) -> Vec<hir::HirId> {
+        should_do_drop_migration: bool,


this would need changing the doc string

I would keep the original funciton as is and create a new one. They don't share too much code together, and it just requires maintaining cotext of the conditionals while reading the code

I mean, I feel like it would need to be renamed in that case

Can we split this function into two? Compute 2229_drop_migrations and 2229_auto_trait_migrations

compiler/rustc_typeck/src/check/upvar.rs

compiler/rustc_lint_defs/src/builtin.rs

arora-aman · 2021-04-11T19:32:25Z

compiler/rustc_typeck/src/check/upvar.rs

@@ -525,6 +541,54 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                },
            );
        }
+
+        if !need_traits_migrations.is_empty() {
+            let (migration_string, migrated_variables_concat) =


I'd create a function or clousre that does most of this, the two codeblocks are basically the same with the paramters being the lint and the associated messages

compiler/rustc_typeck/src/check/upvar.rs

arora-aman · 2021-04-11T19:34:11Z

compiler/rustc_typeck/src/check/upvar.rs

@@ -542,19 +606,57 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
        closure_span: Span,
        closure_clause: hir::CaptureBy,
        min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
-    ) -> Vec<hir::HirId> {
+        should_do_drop_migration: bool,


Can we split this function into two? Compute 2229_drop_migrations and 2229_auto_trait_migrations

src/test/ui/closures/2229_closure_analysis/migrations/auto_traits.fixed

src/test/ui/closures/2229_closure_analysis/migrations/mir_calls_to_shims.fixed

src/test/ui/closures/2229_closure_analysis/migrations/auto_traits.rs

compiler/rustc_lint_defs/src/builtin.rs

arora-aman · 2021-04-20T02:49:38Z

compiler/rustc_typeck/src/check/upvar.rs

+        &self,
+        set_reasons: FxHashSet<&str>
+    ) -> String {
+        let mut reasons = String::new();


you can create a vector, keep pushing reason
then you can call x.join(", ")

Doesn't work well if we want to include the words "trait implementation" too

arora-aman · 2021-04-20T03:07:17Z

compiler/rustc_typeck/src/check/upvar.rs

+        let cause = ObligationCause::misc(self.tcx.hir().span(var_hir_id), self.body_id);
+
+        // <ty as Clone>
+        let clone_trait_ref = TraitRef {


maybe create a vector of tuple (trait_def_id, reason_string) and then iterator over them?

Don't think it would help a lot, maybe creating a function instead? I may come back to this once I am in a more final version

compiler/rustc_typeck/src/check/upvar.rs

arora-aman · 2021-04-20T03:26:55Z

compiler/rustc_typeck/src/check/upvar.rs

+            return None
+        };
+
+        let is_not_completely_captured = !root_var_min_capture_list


what we really need to check is that if path A implement the trait and then any of the captured paths don't implement the trait

arora-aman · 2021-04-20T03:37:54Z

compiler/rustc_typeck/src/check/upvar.rs

 }

 /// Return a two string tuple (s1, s2)
 /// - s1: Line of code that is needed for the migration: eg: `let _ = (&x, ...)`.
 /// - s2: Comma separated names of the variables being migrated.
 fn migration_suggestion_for_2229(
    tcx: TyCtxt<'_>,
-    need_migrations: &Vec<hir::HirId>,
+    need_migrations: &FxHashSet<hir::HirId>,


we should keep this a vector; we want to preserve order. otherwise things might get dropped in a different ordered.

Though we have a ticket to fix it overall, so might not be too bad 🙃

Nvm i see why you're using hashset. I'm dumb

Tis ok, I figured out how to use vector instead without have dups

roxelo

@arora-aman Started addressing some of your comments from last night. Still having a few issues here. If you have any ideas, let me know. I'll do a cleanup later and fix any formatting issues

roxelo · 2021-04-20T19:49:01Z

src/test/ui/closures/2229_closure_analysis/migrations/mir_calls_to_shims.rs

    let f = panic::AssertUnwindSafe(f);
    let result = panic::catch_unwind(move || {
        //~^ ERROR: 'Sync' trait implementation affected for closure because of `capture_disjoint_fields`
        //~| HELP: add a dummy let to cause `f` to be fully captured
-        f.0()
+        f.0.0


I can't figure out how to trigger the 'Sync' error here. Are we sure there should be an issue with AssertUnwindSafe?

why would sync trigger here? Also all objects are by default Send and Sync unles they contain something that is not Send/Sync, making Foo and i32 Send/Sync.

It was an example used in one of the Issues. I was under the impression this should trigger an error? Should it not?

not for the ysnc trait, but for the AssertUnwindSafe trati

Ohh, so I should also check for this one?

roxelo · 2021-04-20T19:50:27Z

compiler/rustc_typeck/src/check/upvar.rs

-        } else {
-            // The upvar is mentioned within the closure but no path starting from it is
-            // used.
+            // <ty as Sync>


I can't figure out how we can check for Send trait? Maybe I am missing where it is suppose to be??

arora-aman · 2021-04-21T04:36:25Z

compiler/rustc_typeck/src/check/upvar.rs

+        // Check whether catpured fields also implement the trait
+        let mut temp_reasons = FxHashSet::default();
+
+        for capture in root_var_min_capture_list.iter() {


I think this function can use some form of helper, because I see a quite a fair bit of repetition.

Something like

// return true if the variables needs to be migrated fn 2229_trait_migration_helper(&self, var_hir_id, trait_def_id) -> bool { // checks that if the trait is satisifed by root varirable; if not return false // check if the trait is not satisfied by an of the captured paths, if so return true. }

One thing to check with niko would be if we want to report why a paricular root variable is being migrated, i.e. store (variable, reason) pairs.

arora-aman · 2021-04-21T04:42:18Z

compiler/rustc_typeck/src/check/upvar.rs

+            reasons = format!("{}'Unpin'", reasons);
+        }
+
+        if set_reasons.contains("Clone")


you can just check for is reasons is not empty

arora-aman · 2021-04-21T04:46:01Z

compiler/rustc_typeck/src/check/upvar.rs

                closure_hir_id,
                span,
                |lint| {
                    let mut diagnostics_builder = lint.build(
-                        "drop order affected for closure because of `capture_disjoint_fields`",
+                        format!(
+                            "{} affected for closure because of `capture_disjoint_fields`",


Also I'mm not sure if "Sync trait affected for closure" is the best wording

Combine all 2229 migrations under one flag name

arora-aman reviewed Apr 8, 2021

View reviewed changes

arora-aman reviewed Apr 11, 2021

View reviewed changes

roxelo force-pushed the rox-auto-trait branch from bf4d44e to 3949790 Compare April 20, 2021 01:46

arora-aman reviewed Apr 20, 2021

View reviewed changes

arora-aman self-assigned this Apr 20, 2021

roxelo commented Apr 20, 2021

View reviewed changes

arora-aman reviewed Apr 21, 2021

View reviewed changes

roxelo force-pushed the rox-auto-trait branch 2 times, most recently from cdb2fe6 to 25f3d74 Compare April 30, 2021 00:18

roxelo added 2 commits May 6, 2021 14:17

Add additional migrations to handle auto-traits and clone traits

9afea61

Combine all 2229 migrations under one flag name

Fix some nits

ad5e18d

roxelo force-pushed the rox-auto-trait branch from 25f3d74 to ad5e18d Compare May 6, 2021 18:58

roxelo added 2 commits May 6, 2021 15:14

Add docstring

acfc708

use the correct attributes and add helper function

564b4de

roxelo force-pushed the rox-auto-trait branch from a895081 to 564b4de Compare May 11, 2021 18:02

sexxi-bot merged commit 28e2b29 into master May 12, 2021

Handle auto trait migration #52

Handle auto trait migration #52

Uh oh!

Conversation

roxelo commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roxelo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roxelo commented Apr 8, 2021 •

edited

Loading