Skip to content

Add basic struct pattern rebinding #3741

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add basic struct pattern rebinding #3741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef547ec
backend: Refactor struct pattern compilation
CohenArthur Apr 16, 2025
b834275
backend: Compile struct rebinding patterns
CohenArthur Apr 16, 2025
f7e5f66
backend: Remove checks on StructFieldIdentPattern
CohenArthur Apr 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 100 additions & 71 deletions gcc/rust/backend/rust-compile-pattern.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
#include "rust-constexpr.h"
#include "rust-compile-type.h"
#include "print-tree.h"
#include "rust-diagnostics.h"
#include "rust-hir-pattern-abstract.h"
#include "rust-hir-pattern.h"
#include "rust-system.h"
#include "rust-tyty.h"

namespace Rust {
namespace Compile {
Expand Down Expand Up @@ -254,34 +259,16 @@ CompilePatternCheckExpr::visit (HIR::StructPattern &pattern)
}
break;

case HIR::StructPatternField::ItemType::IDENT_PAT: {
HIR::StructPatternFieldIdentPat &ident
= static_cast<HIR::StructPatternFieldIdentPat &> (*field.get ());
case HIR::StructPatternField::ItemType::IDENT_PAT:
// we only support rebinding patterns at the moment, which always
// match - so do nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's wrong with the old checks?

Copy link
Member Author

Choose a reason for hiding this comment

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

they caused an invalid call to struct_field_expression which I didn't want to explore. since when compiling struct pattern matching with StructPatternFieldIdentPat (so match x { Foo { field: new_name } => {} } we would always reach a rust_unreachable(), I think this code was actually never ever reached or executed and was just wrong

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think this was meant to handle struct-like enum items, but was probably broken by 806166d. Is there any chance you could fix it, rather than remove it? I could explain any parts of the code you have questions about, or fix it myself and give you the patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

but maybe I'm wrong? happy to change it and try to fix it instead if you think this was correct

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the code is close enough to right that getting it to work should be pretty simple -- check out where match_scrutinee_expr is being modified in the if statement above, you may have to remove that for struct_field_expression to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, I'll take a look then :) didn't realize it had been modified recently, my bad! I thought I was just hitting an uncharted part of the codebase lol

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not all that recently -- I'm pretty sure I wrote it back in late 2023. You are probably right that it was never actually being executed, though I think it should function with a few tweaks


size_t offs = 0;
ok = variant->lookup_field (ident.get_identifier ().as_string (),
nullptr, &offs);
rust_assert (ok);

// we may be offsetting by + 1 here since the first field in the
// record is always the discriminator
offs += adt->is_enum ();
tree field_expr
= Backend::struct_field_expression (match_scrutinee_expr, offs,
ident.get_locus ());

tree check_expr_sub
= CompilePatternCheckExpr::Compile (ident.get_pattern (),
field_expr, ctx);
check_expr = Backend::arithmetic_or_logical_expression (
ArithmeticOrLogicalOperator::BITWISE_AND, check_expr,
check_expr_sub, ident.get_pattern ().get_locus ());
}
// this needs to change once we actually check that the field
// matches a certain value, either a literal value or a const one
break;

case HIR::StructPatternField::ItemType::IDENT: {
// ident pattern always matches - do nothing
}
case HIR::StructPatternField::ItemType::IDENT:
// ident pattern always matches - do nothing
break;
}
}
Expand Down Expand Up @@ -504,6 +491,88 @@ CompilePatternBindings::visit (HIR::TupleStructPattern &pattern)
}
}

tree
CompilePatternBindings::make_struct_access (TyTy::ADTType *adt,
TyTy::VariantDef *variant,
Identifier &ident,
int variant_index)
{
size_t offs = 0;
auto ok = variant->lookup_field (ident.as_string (), nullptr, &offs);
rust_assert (ok);

if (adt->is_enum ())
{
tree payload_accessor_union
= Backend::struct_field_expression (match_scrutinee_expr, 1,
ident.get_locus ());

tree variant_accessor
= Backend::struct_field_expression (payload_accessor_union,
variant_index, ident.get_locus ());

return Backend::struct_field_expression (variant_accessor, offs,
ident.get_locus ());
}
else
{
tree variant_accessor = match_scrutinee_expr;

return Backend::struct_field_expression (variant_accessor, offs,
ident.get_locus ());
}
}

void
CompilePatternBindings::handle_struct_pattern_ident (
HIR::StructPatternField &pat, TyTy::ADTType *adt, TyTy::VariantDef *variant,
int variant_index)
{
HIR::StructPatternFieldIdent &ident
= static_cast<HIR::StructPatternFieldIdent &> (pat);

auto identifier = ident.get_identifier ();
tree binding = make_struct_access (adt, variant, identifier, variant_index);

ctx->insert_pattern_binding (ident.get_mappings ().get_hirid (), binding);
}

void
CompilePatternBindings::handle_struct_pattern_ident_pat (
HIR::StructPatternField &pat, TyTy::ADTType *adt, TyTy::VariantDef *variant,
int variant_index)
{
auto &pattern = static_cast<HIR::StructPatternFieldIdentPat &> (pat);

switch (pattern.get_pattern ().get_pattern_type ())
{
case HIR::Pattern::IDENTIFIER: {
auto &id
= static_cast<HIR::IdentifierPattern &> (pattern.get_pattern ());

CompilePatternBindings::Compile (id, match_scrutinee_expr, ctx);
}
break;
default:
rust_sorry_at (pat.get_locus (),
"cannot handle non-identifier struct patterns");
return;
}

auto ident = pattern.get_identifier ();
tree binding = make_struct_access (adt, variant, ident, variant_index);

ctx->insert_pattern_binding (
pattern.get_pattern ().get_mappings ().get_hirid (), binding);
}

void
CompilePatternBindings::handle_struct_pattern_tuple_pat (
HIR::StructPatternField &pat)
{
rust_unreachable ();
}

void
CompilePatternBindings::visit (HIR::StructPattern &pattern)
{
Expand Down Expand Up @@ -539,54 +608,14 @@ CompilePatternBindings::visit (HIR::StructPattern &pattern)
{
switch (field->get_item_type ())
{
case HIR::StructPatternField::ItemType::TUPLE_PAT: {
// TODO
rust_unreachable ();
}
case HIR::StructPatternField::ItemType::TUPLE_PAT:
handle_struct_pattern_tuple_pat (*field);
break;

case HIR::StructPatternField::ItemType::IDENT_PAT: {
// TODO
rust_unreachable ();
}
case HIR::StructPatternField::ItemType::IDENT_PAT:
handle_struct_pattern_ident_pat (*field, adt, variant, variant_index);
break;

case HIR::StructPatternField::ItemType::IDENT: {
HIR::StructPatternFieldIdent &ident
= static_cast<HIR::StructPatternFieldIdent &> (*field.get ());

size_t offs = 0;
ok = variant->lookup_field (ident.get_identifier ().as_string (),
nullptr, &offs);
rust_assert (ok);

tree binding = error_mark_node;
if (adt->is_enum ())
{
tree payload_accessor_union
= Backend::struct_field_expression (match_scrutinee_expr, 1,
ident.get_locus ());

tree variant_accessor
= Backend::struct_field_expression (payload_accessor_union,
variant_index,
ident.get_locus ());

binding
= Backend::struct_field_expression (variant_accessor, offs,
ident.get_locus ());
}
else
{
tree variant_accessor = match_scrutinee_expr;
binding
= Backend::struct_field_expression (variant_accessor, offs,
ident.get_locus ());
}

ctx->insert_pattern_binding (ident.get_mappings ().get_hirid (),
binding);
}
case HIR::StructPatternField::ItemType::IDENT:
handle_struct_pattern_ident (*field, adt, variant, variant_index);
break;
}
}
Expand Down
15 changes: 15 additions & 0 deletions gcc/rust/backend/rust-compile-pattern.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
// <http://www.gnu.org/licenses/>.

#include "rust-compile-base.h"
#include "rust-hir-pattern.h"
#include "rust-hir-visitor.h"
#include "rust-tyty.h"

namespace Rust {
namespace Compile {
Expand Down Expand Up @@ -78,6 +80,19 @@ class CompilePatternBindings : public HIRCompileBase,
pattern.accept_vis (compiler);
}

tree make_struct_access (TyTy::ADTType *adt, TyTy::VariantDef *variant,
Identifier &ident, int variant_index);

void handle_struct_pattern_ident (HIR::StructPatternField &pat,
TyTy::ADTType *adt,
TyTy::VariantDef *variant,
int variant_index);
void handle_struct_pattern_ident_pat (HIR::StructPatternField &pat,
TyTy::ADTType *adt,
TyTy::VariantDef *variant,
int variant_index);
void handle_struct_pattern_tuple_pat (HIR::StructPatternField &pat);

Copy link
Collaborator

Choose a reason for hiding this comment

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

These should probably be private

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch thanks

void visit (HIR::StructPattern &pattern) override;
void visit (HIR::TupleStructPattern &pattern) override;
void visit (HIR::ReferencePattern &pattern) override;
Expand Down
19 changes: 19 additions & 0 deletions gcc/testsuite/rust/execute/torture/struct_pattern1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
struct A {
// the two warnings are invalid but this should be fixed by our lint rework
// with this year's GSoC so ok for now
a: i32, // { dg-warning "never read" }
b: i32, // { dg-warning "never read" }
}

fn main() -> i32 {
let a = A { a: 15, b: 14 };

let result = match a {
A {
a: self_a,
b: self_b,
} => self_a + self_b,
};

result - 29
}