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

Handle patterns within closures #45

Merged
merged 8 commits into from
Mar 22, 2021
Merged

Handle patterns within closures #45

merged 8 commits into from
Mar 22, 2021

Conversation

roxelo
Copy link
Member

@roxelo roxelo commented Jan 30, 2021

Using the following test:

#![feature(capture_disjoint_fields)]

fn main() {
    let _z = 9;
    let t = (String::new(), String::new());

    let _c = ||  {
          let (_t1, _) = t;
    };
}

The relevant log output is as follows:

DEBUG rustc_typeck::check::upvar RFC2229 = upvar.rs Initialized delegate InferBorrowKind
DEBUG rustc_typeck::expr_use_visitor RFC2229 = expr_use_visitor.rs walk_pat introduce fake_read
DEBUG rustc_typeck::check::upvar RFC2229 = upvar.rs Adding fake read PlaceWithHirId { hir_id: HirId { owner: DefId(0:3 ~ rox[317d]::main), local_id: 20 }, place: Place { base_ty: (std::string::String, std
DEBUG rustc_typeck::check::upvar RFC2229 = upvar.rs write results out to typeck_results
DEBUG rustc_typeck::check::writeback RFC2229 = writeback.rs call visit_fake_read_map()
DEBUG rustc_typeck::check::writeback RFC2229 = writeback.rs visit_fake_reads_map
DEBUG rustc_mir_build::thir::cx::expr RFC2229 = Closure expr.rs ExprKind::Closure
DEBUG rustc_mir_build::thir::cx::expr RFC2229 = closure_fake_read {DefId(0:4 ~ rox[317d]::main::{closure#0}): [Place { base_ty: (std::string::String, std::string::String), base: Upvar(UpvarId(HirId { owner: DefId(0:3 ~ rox[317d]::main), local_id: 17 };`t`;DefId(0:4 ~ rox[317d]::main::{closure#0}))), projections: [] }]}
DEBUG rustc_mir_build::build::expr::as_rvalue RFC2229 = as_rvalue.rs ExprKind::Closure
DEBUG rustc_mir_build::build::expr::as_rvalue RFC2229 = as_rvalue.rs Place { base_ty: (std::string::String, std::string::String), base: Upvar(UpvarId(HirId { owner: DefId(0:3 ~ rox[317d]::main), local_id: 17 };`t`;DefId(0:4 ~ rox[317d]::main::{closure#0}))), projections: [] }
DEBUG rustc_mir_build::build::expr::as_rvalue RFC2229 = as_rvalue pushing following fake_read HirId { owner: DefId(0:3 ~ rox[317d]::main), local_id: 17 }
DEBUG rustc_mir_build::build::expr::as_rvalue RFC2229 = as_rvalue pushing following mir_place _2
DEBUG rustc_mir_build::build::expr::as_rvalue RFC2229 = as_rvalue.rs ExprKind::Closure fake_read processing over

Copy link
Member

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

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

Just things that I feel need to chage. Will do nits later

compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/build/expr/as_rvalue.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/build/expr/as_place.rs Outdated Show resolved Hide resolved
let fake_read_upvar = this.hir.mirror(thir_place);
let mir_place = unpack!(block = this.as_place(block, fake_read_upvar));
debug!("RFC2229 = as_rvalue pushing following mir_place {:?}", mir_place);
this.cfg.push_fake_read(block, source_info, FakeReadCause::ForLet, mir_place);
Copy link
Member

Choose a reason for hiding this comment

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

There are two fake read causes. One ForLet and one ForMatch. We probably should capture the correct one. I don't think it will be too much work to do that, but I think if this works fine for now, we should delay that until we fix bugs/ we get close to getting this to work.

compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
@roxelo roxelo force-pushed the handle-patterns-take-2 branch 6 times, most recently from 751a599 to 84157b4 Compare February 19, 2021 05:27
compiler/rustc_mir_build/src/build/expr/as_place.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/build/matches/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/build/expr/as_place.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/build/expr/as_place.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
@roxelo roxelo force-pushed the handle-patterns-take-2 branch 2 times, most recently from 523a104 to 31e3451 Compare February 25, 2021 20:40
Copy link
Member

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

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

initial set of comments

compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/mod.rs Outdated Show resolved Hide resolved
@@ -1102,6 +1108,7 @@ struct InferBorrowKind<'a, 'tcx> {
/// Place { V1, [ProjectionKind::Field(Index=1, Variant=0)] } : CaptureKind { E2, MutableBorrow }
/// ```
capture_information: InferredCaptureInformation<'tcx>,
fake_reads: Vec<(Place<'tcx>, FakeReadCause)>,
Copy link
Member

Choose a reason for hiding this comment

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

Did you get a chance to try HashSet, because in case of

let c = || {
   let (t1, _) = t;
   let (_, t2) = t;
}

we will introduce 2 fake reads, when one should suffice

compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
@@ -608,6 +645,30 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
ty::Closure(..) | ty::Generator(..)
);

if let Some(fake_reads) = self.mc.typeck_results.closure_fake_reads.get(&closure_def_id) {
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: Read this logic properly

compiler/rustc_mir_build/src/build/expr/as_place.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/build/expr/as_rvalue.rs Outdated Show resolved Hide resolved
@@ -62,7 +62,7 @@

mod as_constant;
mod as_operand;
mod as_place;
pub mod as_place;
Copy link
Member

Choose a reason for hiding this comment

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

is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes,

error[E0603]: module `as_place` is private
  --> compiler/rustc_mir_build/src/build/mod.rs:2:25
   |
2  | use crate::build::expr::as_place::PlaceBuilder;
   |                         ^^^^^^^^ private module
   |
note: the module `as_place` is defined here
  --> compiler/rustc_mir_build/src/build/expr/mod.rs:65:1
   |
65 | mod as_place;
   | ^^^^^^^^^^^^^

error[E0603]: module `as_place` is private
  --> compiler/rustc_mir_build/src/build/matches/mod.rs:8:25
   |
8  | use crate::build::expr::as_place::PlaceBuilder;
   |                         ^^^^^^^^ private module
   |
note: the module `as_place` is defined here
  --> compiler/rustc_mir_build/src/build/expr/mod.rs:65:1
   |
65 | mod as_place;
   | ^^^^^^^^^^^^^

error[E0603]: module `as_place` is private
  --> compiler/rustc_mir_build/src/build/matches/simplify.rs:15:25
   |
15 | use crate::build::expr::as_place::PlaceBuilder;
   |                         ^^^^^^^^ private module
   |
note: the module `as_place` is defined here
  --> compiler/rustc_mir_build/src/build/expr/mod.rs:65:1
   |
65 | mod as_place;
   | ^^^^^^^^^^^^^

error[E0603]: module `as_place` is private
  --> compiler/rustc_mir_build/src/build/matches/test.rs:8:25
   |
8  | use crate::build::expr::as_place::PlaceBuilder;
   |                         ^^^^^^^^ private module
   |
note: the module `as_place` is defined here
  --> compiler/rustc_mir_build/src/build/expr/mod.rs:65:1
   |
65 | mod as_place;
   | ^^^^^^^^^^^^^

error[E0603]: module `as_place` is private
  --> compiler/rustc_mir_build/src/build/matches/util.rs:1:25
   |
1  | use crate::build::expr::as_place::PlaceBuilder;
   |                         ^^^^^^^^ private module
   |
note: the module `as_place` is defined here
  --> compiler/rustc_mir_build/src/build/expr/mod.rs:65:1
   |
65 | mod as_place;
   | ^^^^^^^^^^^^^

error: aborting due to 5 previous errors

@roxelo roxelo changed the title Handle patterns within closures [WIP] Handle patterns within closures Feb 25, 2021
@roxelo
Copy link
Member Author

roxelo commented Feb 25, 2021

Opened PR on rust-lang rust-lang#82536

@roxelo roxelo force-pushed the handle-patterns-take-2 branch 3 times, most recently from d566cb4 to 9aca998 Compare March 1, 2021 02:11
@roxelo roxelo force-pushed the handle-patterns-take-2 branch 3 times, most recently from 06ff810 to 459ebf8 Compare March 12, 2021 04:42
@sexxi-bot sexxi-bot merged commit f5d8117 into master Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants