Implemented method translation to BIR. #2892

braw-lee · 2024-02-29T04:01:00Z

Fixes #2888
Depends on #2889

jdupak · 2024-04-06T07:41:28Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+  // compile `self` and handle autoref
+  PlaceId self = visit_expr (*expr.get_receiver ());
+  auto ty = ctx.place_db[self].tyty;
+  if (ty->get_kind () == TyTy::REF)


This seems to be inverted. Autoref needs to make reference to values that are not references.

Autoderef may be also needed here. What you want to achieve is to have exactly one reference (in terms of levels).

jdupak · 2024-04-06T07:54:00Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

-{}
+{
+  // TODO : need ExprStmtBuilder::visit_expr (HIR::PathExprSegment)
+  PlaceId fn = visit_expr (expr.get_method_name ());


The trick for this is to look into the gccrs backend and check how Philip is already doing it. That you need here is at: gcc\rust\backend\rust-compile-expr.cc:1281

Note that the back-end needs to handle way more then we need to do here. He only care about the flow of data and its types.

jdupak · 2024-05-12T15:55:51Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+  // it might be resolved to a trait item
+  HIR::TraitItem *trait_item = mappings->lookup_trait_item_defid (id);
+  HIR::Trait *trait = mappings->lookup_trait_item_mapping (
+    trait_item->get_mappings ().get_hirid ());
+
+  Resolver::TraitReference *trait_ref
+    = &Resolver::TraitReference::error_node ();
+  bool ok
+    = ctx.tyctx.lookup_trait_reference (trait->get_mappings ().get_defid (),
+					&trait_ref);
+  rust_assert (ok);
+
+  // the type resolver can only resolve type bounds to their trait
+  // item so its up to us to figure out if this path should resolve
+  // to an trait-impl-block-item or if it can be defaulted to the
+  // trait-impl-item's definition
+  const HIR::PathIdentSegment segment (trait_item->trait_identifier ());
+  auto root = receiver->get_root ();
+  auto candidates
+    = Resolver::PathProbeImplTrait::Probe (root, segment, trait_ref);
+  if (candidates.size () == 0)
+    {
+      // this means we are defaulting back to the trait_item if
+      // possible
+      Resolver::TraitItemReference *trait_item_ref = nullptr;
+      bool ok = trait_ref->lookup_hir_trait_item (*trait_item, &trait_item_ref);
+      rust_assert (ok);				    // found
+      rust_assert (trait_item_ref->is_optional ()); // has definition
+
+      TyTy::BaseType *resolved_fntype = nullptr;
+      bool fntype_resolved
+	= ctx.tyctx.lookup_type (trait_item_ref->get_mappings ().get_hirid (),
+				 &resolved_fntype);
+
+      rust_assert (fntype_resolved);
+      rust_assert (resolved_fntype->is<TyTy::FnType> ());
+      return ctx.place_db.get_constant (resolved_fntype);
+    }
+
+  const Resolver::PathProbeCandidate *selectedCandidate = nullptr;
+
+  // filter for the possible case of non fn type items
+  std::set<Resolver::PathProbeCandidate> filteredFunctionCandidates;
+  for (auto &candidate : candidates)
+    {
+      bool is_fntype = candidate.ty->get_kind () == TyTy::TypeKind::FNDEF;
+      if (!is_fntype)
+	continue;
+
+      filteredFunctionCandidates.insert (candidate);
+    }
+
+  // look for the exact fntype
+  for (auto &candidate : filteredFunctionCandidates)
+    {
+      if (filteredFunctionCandidates.size () == 1)
+	{
+	  selectedCandidate = &candidate;
+	  break;
+	}
+
+      bool compatable
+	= Resolver::types_compatable (TyTy::TyWithLocation (candidate.ty),
+				      TyTy::TyWithLocation (fntype), expr_locus,
+				      false);
+
+      if (compatable)
+	{
+	  selectedCandidate = &candidate;
+	  break;
+	}
+    }
+
+  rust_assert (selectedCandidate != nullptr);


I think, all of this should be in a function shared with the backend. It does not seem wise to duplicate it (it is absolutely fine for experiments).

jdupak · 2024-05-12T16:01:05Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+      TyTy::BaseType *resolved_fntype = nullptr;
+      bool fntype_resolved
+	= ctx.tyctx.lookup_type (resolved_item->get_mappings ().get_hirid (),
+				 &resolved_fntype);
+
+      rust_assert (fntype_resolved);
+      rust_assert (resolved_fntype->is<TyTy::FnType> ());
+      return ctx.place_db.get_constant (resolved_fntype);


Can be simplified to:

return ctx.place_db.get_constant ( lookup_type (resolved_item)->as<TyTy::FnType> ());

(This is a common operation, so I have tooling for it.)

jdupak · 2024-05-12T16:08:01Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+	  break;
+	case Resolver::Adjustment::AdjustmentType::DEREF:
+	case Resolver::Adjustment::AdjustmentType::DEREF_MUT:
+	  // TODO: need to handle autoderef i.e achieve one reference in terms


ctx.place_db.lookup_or_add_path (Place::DEREF, ty, adjusted_self)

jdupak · 2024-05-12T16:09:06Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+}
+
+PlaceId
+ExprStmtBuilder::resolve_adjustments (


This is good. Maybe we can use it in more places, where I took a shortcut.

jdupak · 2024-05-12T16:09:35Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+  const std::vector<Resolver::Adjustment> &adjustments, const PlaceId self)
+{
+  PlaceId adjusted_self = self;
+  auto ty = ctx.place_db[adjusted_self].tyty;


Move this inside the loop to keep it up to date.

jdupak · 2024-05-12T16:13:21Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+	    = borrow_place (adjusted_self,
+			    new TyTy::ReferenceType (
+			      ty->get_ref (), TyTy::TyVar (ty->get_ref ()),
+			      Mutability::Imm));


Hmm, borrow place actually just needs the mutability. We should probably refactor to also take just mutability.

jdupak · 2024-05-12T16:15:51Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

+	case Resolver::Adjustment::AdjustmentType::ERROR:
+	case Resolver::Adjustment::AdjustmentType::INDIRECTION:
+	case Resolver::Adjustment::AdjustmentType::UNSIZE:
+	  // FIXME: Are these adjustments needed for borrowchecker


Not sure. @philberty could you help out here.

jdupak · 2024-05-12T16:35:14Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc

@@ -679,5 +731,149 @@ ExprStmtBuilder::visit (HIR::ExprStmt &stmt)
  if (result != INVALID_PLACE)
    push_tmp_assignment (result);
 }
+
+PlaceId
+ExprStmtBuilder::resolve_method (TyTy::FnType *fntype, TyTy::BaseType *receiver,


I am kinda convinced that this all method is not needed. We start of with a FnType. But that is all we need. I think.

jdupak · 2024-05-12T18:03:33Z

TLDR: @braw-lee I cannot push to your branch, so this is my suggested change: https://github.com/jdupak/gccrs/tree/borrowck-wip

jdupak · 2024-05-12T18:05:06Z

If you have time, I suggest adding test for some convoluted testcases.

gcc/rust/ChangeLog: * checks/errors/borrowck/rust-bir-builder-expr-stmt.cc (ExprStmtBuilder::visit): Implementation of resolving HIR::MethodCallExpr to BIR::CallExpr. (ExprStmtBuilder::visit_and_adjust): Function to visit `self` parameter and apply the required adjustments. * checks/errors/borrowck/rust-bir-builder-expr-stmt.h: Header for new helper function. * checks/errors/borrowck/rust-bir-builder-internal.h: New method for borrowing `self` parameter. gcc/testsuite/ChangeLog: * rust/borrowck/methods.rs: New test. Signed-off-by: Kushal Pal <kushalpal109@gmail.com>

braw-lee force-pushed the borrowck-wip branch from 5d29597 to c474ba8 Compare February 29, 2024 06:05

braw-lee force-pushed the borrowck-wip branch from c474ba8 to 59fdeef Compare March 9, 2024 13:02

braw-lee force-pushed the borrowck-wip branch from 28fa188 to 02636bc Compare April 6, 2024 04:52

jdupak reviewed Apr 6, 2024

View reviewed changes

braw-lee force-pushed the borrowck-wip branch 3 times, most recently from 8bcd111 to 2d69d47 Compare May 2, 2024 12:30

jdupak reviewed May 12, 2024

View reviewed changes

braw-lee force-pushed the borrowck-wip branch 4 times, most recently from 6ca7765 to 1a157cd Compare May 13, 2024 11:47

braw-lee force-pushed the borrowck-wip branch from 1a157cd to 73758ea Compare May 13, 2024 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented method translation to BIR. #2892

Implemented method translation to BIR. #2892

braw-lee commented Feb 29, 2024

jdupak Apr 6, 2024

jdupak Apr 6, 2024 •

edited

jdupak Apr 6, 2024

jdupak May 12, 2024

jdupak May 12, 2024

jdupak May 12, 2024

jdupak May 12, 2024

jdupak May 12, 2024

jdupak May 12, 2024

jdupak May 12, 2024

jdupak May 12, 2024

jdupak commented May 12, 2024

jdupak commented May 12, 2024

Implemented method translation to BIR. #2892

Are you sure you want to change the base?

Implemented method translation to BIR. #2892

Conversation

braw-lee commented Feb 29, 2024

Choose a reason for hiding this comment

jdupak Apr 6, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdupak commented May 12, 2024

jdupak commented May 12, 2024

jdupak Apr 6, 2024 •

edited