borrowck: Added location support to BIR nodes #3013

braw-lee · 2024-05-20T10:01:45Z

depends on #3076
Added location field to BIR::Statement and BIR::Place, updated the BIR builders to take the source location from HIR and pass it to BIR node.

braw-lee · 2024-05-21T08:40:32Z

should I work on my branch the whole time, and you can merge it after 2-3 months?
or should I just create new PRs whenever I get something done and you will merge those from time to time? @CohenArthur @P-E-P

P-E-P · 2024-05-21T08:42:37Z

should I work on my branch the whole time, and you can merge it after 2-3 months? or should I just create new PRs whenever I get something done and you will merge those from time to time? @CohenArthur @P-E-P

Actually it would be better to merge your content quite often. This way other contributors can follow your work and can work over it. Moreover it'll be easier to review and you won't have to make huge rebase.

We may be a little slow to merge some PRs sometime because reviewing them take some time. You may create some PRs on top of another with the mention "Requires #PRNUMBER", you may even put a link to the actual diff just like in #3014. If you do so please keep those PR in draft whilst it's dependencies are not merged so they don't end up merged.

CohenArthur · 2024-05-24T12:03:01Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-internal.h

+  void push_tmp_assignment (PlaceId rhs, location_t location)
  {
    push_tmp_assignment (new Assignment (rhs), ctx.place_db[rhs].tyty);


we're not using the location argument here yet - I assume this is intentional?

missed it, will clean it up

CohenArthur · 2024-05-24T12:04:17Z

gcc/rust/checks/errors/borrowck/rust-bir.h

@@ -77,26 +77,40 @@ class Statement
  // otherwise: <unused>
  std::unique_ptr<AbstractExpr> expr;
  TyTy::BaseType *type;
+  // stores location of the actual expression from source code
+  // currently only available when kind == Kind::ASSIGNMENT
+  tl::optional<location_t> location;


do we want to make this optional? if this is just a temporary measure, then we should add a FIXME comment

If this is a TODO, it would make more sense to use just location and init it to UNKNOWN_LOCATION.

ok, will fix that

jdupak

Mostly looks good. There are some issues around Places.

jdupak · 2024-05-24T12:08:34Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-pattern.h

@@ -82,7 +82,9 @@ class PatternBindingBuilder : protected AbstractBuilder,

    if (init.has_value ())
      {
-	push_assignment (translated, init.value ());
+	push_assignment (
+	  translated, init.value (),


Maybe, it would make more sense here to pass the location to the visit identifier method.

jdupak · 2024-05-24T12:14:44Z

gcc/rust/checks/errors/borrowck/rust-bir-place.h

@@ -78,12 +78,13 @@ struct Place
  TyTy::BaseType *tyty;
  FreeRegions regions{{}};
  std::vector<LoanId> borrowed_by{};
+  location_t location;


I am not sure, if this is needed. We might have just locations for Origin.

jdupak · 2024-05-24T12:16:52Z

gcc/rust/checks/errors/borrowck/rust-bir-place.h

@@ -255,7 +256,9 @@ class PlaceDB

  PlaceId add_variable (NodeId id, TyTy::BaseType *tyty)
  {
-    return add_place ({Place::VARIABLE, id, {}, is_type_copy (tyty), tyty}, 0);
+    return add_place (
+      {Place::VARIABLE, id, {}, is_type_copy (tyty), tyty, tyty->get_locus ()},


Using location of type does not seem right. As I mentioned before, we might not oven need it here. Alternative could be to extract the location from NodeId. But that can always be done on demand.

actually I was first trying to use get_ast_item() method to retrieve the location, but it seems like the location is copied from AST->HIR->TyTy, so getting locus from NodeId or the TyTy should be same, unless we plan to change the location information of TyTy in future

jdupak · 2024-05-24T12:19:17Z

gcc/rust/checks/errors/borrowck/rust-bir.h

@@ -77,26 +77,40 @@ class Statement
  // otherwise: <unused>
  std::unique_ptr<AbstractExpr> expr;
  TyTy::BaseType *type;
+  // stores location of the actual expression from source code
+  // currently only available when kind == Kind::ASSIGNMENT
+  tl::optional<location_t> location;


If this is a TODO, it would make more sense to use just location and init it to UNKNOWN_LOCATION.

CohenArthur · 2024-07-16T11:40:59Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-internal.h

+  template <typename T>
+  void move_all (T &args, std::vector<location_t> capture_locations)
  {
-    std::transform (args.begin (), args.end (), args.begin (),
-		    [this] (PlaceId arg) { return move_place (arg); });
+    rust_assert (args.size () == capture_locations.size ());
+    std::transform (args.begin (), args.end (), capture_locations.begin (),
+		    args.begin (), [this] (PlaceId arg, location_t location) {
+		      return move_place (arg, location);
+		    });


hm, I don't know if the second parameters should be called capture_locations and not locations. as far as I understand it, this isn't strictly restricted to closure captures, right? @jdupak what do you think?

Well , I think that captures are the only place, where you have multiple different locations. But In that case this function should be renamed. I'm not sure if such function makes sense in this context.

CohenArthur · 2024-07-16T15:36:02Z

This needs to be rebased as well @braw-lee

P-E-P · 2024-07-17T09:11:52Z

gcc/rust/checks/errors/borrowck/ffi-polonius/src/gccrs_ffi_generated.rs

-    pub loan_errors: bool,
-    pub subset_errors: bool,
-    pub move_errors: bool,
+    pub loan_errors: *mut FFIVector,
+    pub move_errors: *mut FFIVector,
+    pub subset_errors: *mut FFIVector,


Are those changes generated using bindgen ?

P-E-P · 2024-07-17T09:33:00Z

gcc/rust/checks/errors/borrowck/ffi-polonius/src/lib.rs

@@ -46,6 +63,12 @@ impl From<GccrsAtom> for usize {
    }
 }

+impl From<&GccrsAtom> for usize {


Do we really need to take this by reference ? GccrsAtom is Copy this looks weird since we already have a From implementation for GccrsAtom I bet every situation can be dealt with at the call site.

This commit adds location_t to BIR::Statement where type is ASSIGNMENT this information will be later used for reporting borrow-checking errors. gcc/rust/ChangeLog: * checks/errors/borrowck/rust-bir-builder-expr-stmt.cc (ExprStmtBuilder::visit): Added location parameter. * checks/errors/borrowck/rust-bir-builder-internal.h: Likewise. * checks/errors/borrowck/rust-bir-builder-lazyboolexpr.h: Likewise. * checks/errors/borrowck/rust-bir-builder-pattern.h: Likewise. * checks/errors/borrowck/rust-bir-builder.h: Likewise. * checks/errors/borrowck/rust-bir.h: Likewise. Signed-off-by: Kushal Pal <[email protected]>

This commit adds location_t to BIR::Loan, this location will point to location is source code where the borrow occured, this information will be useful for reporting borrow-checking errors. gcc/rust/ChangeLog: * checks/errors/borrowck/rust-bir-builder-internal.h: Fill location for loan. * checks/errors/borrowck/rust-bir-place.h (struct Loan): Add location field. Signed-off-by: Kushal Pal <[email protected]>

CohenArthur

LGTM!

P-E-P requested review from jdupak and P-E-P May 21, 2024 09:07

P-E-P added the borrow checking label May 21, 2024

CohenArthur reviewed May 24, 2024

View reviewed changes

jdupak suggested changes May 24, 2024

View reviewed changes

braw-lee force-pushed the add_location branch from 6438ea7 to 2ee0eb8 Compare May 27, 2024 01:23

braw-lee marked this pull request as draft May 27, 2024 11:58

braw-lee force-pushed the add_location branch from 2ee0eb8 to 24b7f70 Compare May 27, 2024 12:05

braw-lee force-pushed the add_location branch 4 times, most recently from 7c4222c to 0f34d93 Compare July 11, 2024 03:04

braw-lee marked this pull request as ready for review July 12, 2024 06:55

CohenArthur reviewed Jul 16, 2024

View reviewed changes

CohenArthur approved these changes Jul 16, 2024

View reviewed changes

CohenArthur requested review from jdupak and CohenArthur July 16, 2024 15:32

P-E-P reviewed Jul 17, 2024

View reviewed changes

braw-lee added 2 commits July 18, 2024 02:44

braw-lee force-pushed the add_location branch from 0f34d93 to 6b5cf3e Compare July 18, 2024 02:45

braw-lee mentioned this pull request Jul 19, 2024

Better loan errors #3088

Merged

CohenArthur approved these changes Jul 24, 2024

View reviewed changes

P-E-P added this pull request to the merge queue Aug 1, 2024

Merged via the queue into Rust-GCC:master with commit bcd860f Aug 1, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

borrowck: Added location support to BIR nodes #3013

borrowck: Added location support to BIR nodes #3013

braw-lee commented May 20, 2024 •

edited

Loading

braw-lee commented May 21, 2024

P-E-P commented May 21, 2024 •

edited

Loading

CohenArthur May 24, 2024

braw-lee May 24, 2024

CohenArthur May 24, 2024

jdupak May 24, 2024

braw-lee May 24, 2024

jdupak left a comment

jdupak May 24, 2024

jdupak May 24, 2024

jdupak May 24, 2024

braw-lee May 24, 2024

jdupak May 24, 2024

CohenArthur Jul 16, 2024

jdupak Jul 16, 2024

CohenArthur commented Jul 16, 2024

P-E-P Jul 17, 2024

P-E-P Jul 17, 2024

CohenArthur left a comment

borrowck: Added location support to BIR nodes #3013

borrowck: Added location support to BIR nodes #3013

Conversation

braw-lee commented May 20, 2024 • edited Loading

braw-lee commented May 21, 2024

P-E-P commented May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdupak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CohenArthur commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CohenArthur left a comment

Choose a reason for hiding this comment

braw-lee commented May 20, 2024 •

edited

Loading

P-E-P commented May 21, 2024 •

edited

Loading