-
Notifications
You must be signed in to change notification settings - Fork 144
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
Implemented method translation to BIR. #2892
base: master
Are you sure you want to change the base?
Conversation
5d29597
to
c474ba8
Compare
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be inverted. Autoref needs to make reference to values that are not references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autoderef may be also needed here. What you want to achieve is to have exactly one reference (in terms of levels).
{} | ||
{ | ||
// TODO : need ExprStmtBuilder::visit_expr (HIR::PathExprSegment) | ||
PlaceId fn = visit_expr (expr.get_method_name ()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8bcd111
to
2d69d47
Compare
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
break; | ||
case Resolver::Adjustment::AdjustmentType::DEREF: | ||
case Resolver::Adjustment::AdjustmentType::DEREF_MUT: | ||
// TODO: need to handle autoderef i.e achieve one reference in terms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx.place_db.lookup_or_add_path (Place::DEREF, ty, adjusted_self)
} | ||
|
||
PlaceId | ||
ExprStmtBuilder::resolve_adjustments ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. Maybe we can use it in more places, where I took a shortcut.
const std::vector<Resolver::Adjustment> &adjustments, const PlaceId self) | ||
{ | ||
PlaceId adjusted_self = self; | ||
auto ty = ctx.place_db[adjusted_self].tyty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this inside the loop to keep it up to date.
= borrow_place (adjusted_self, | ||
new TyTy::ReferenceType ( | ||
ty->get_ref (), TyTy::TyVar (ty->get_ref ()), | ||
Mutability::Imm)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, borrow place actually just needs the mutability. We should probably refactor to also take just mutability.
case Resolver::Adjustment::AdjustmentType::ERROR: | ||
case Resolver::Adjustment::AdjustmentType::INDIRECTION: | ||
case Resolver::Adjustment::AdjustmentType::UNSIZE: | ||
// FIXME: Are these adjustments needed for borrowchecker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. @philberty could you help out here.
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
TLDR: @braw-lee I cannot push to your branch, so this is my suggested change: https://github.com/jdupak/gccrs/tree/borrowck-wip |
If you have time, I suggest adding test for some convoluted testcases. |
6ca7765
to
1a157cd
Compare
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>
Fixes #2888
Depends on #2889