Skip to content

Add check for path patterns. #3125

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

Merged
merged 1 commit into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions gcc/rust/hir/tree/rust-hir.cc
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,44 @@ Module::as_string () const
return str + "\n";
}

std::string
Item::item_kind_string (Item::ItemKind kind)
{
switch (kind)
{
case Item::ItemKind::Static:
return "static";
case Item::ItemKind::Constant:
return "constant";
case Item::ItemKind::TypeAlias:
return "type alias";
case Item::ItemKind::Function:
return "function";
case Item::ItemKind::UseDeclaration:
return "use declaration";
case Item::ItemKind::ExternBlock:
return "extern block";
case Item::ItemKind::ExternCrate:
return "extern crate";
case Item::ItemKind::Struct:
return "struct";
case Item::ItemKind::Union:
return "union";
case Item::ItemKind::Enum:
return "enum";
case Item::ItemKind::EnumItem:
return "enum item";
case Item::ItemKind::Trait:
return "trait";
case Item::ItemKind::Impl:
return "impl";
case Item::ItemKind::Module:
return "module";
default:
rust_unreachable ();
}
}

std::string
StaticItem::as_string () const
{
Expand Down
2 changes: 2 additions & 0 deletions gcc/rust/hir/tree/rust-hir.h
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,8 @@ class Item : public Stmt, public WithOuterAttrs
Module,
};

static std::string item_kind_string (ItemKind kind);

virtual ItemKind get_item_kind () const = 0;

// Unique pointer custom clone function
Expand Down
13 changes: 11 additions & 2 deletions gcc/rust/typecheck/rust-hir-type-check-expr.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1465,6 +1465,7 @@ TypeCheckExpr::visit (HIR::MatchExpr &expr)
TyTy::BaseType *scrutinee_tyty
= TypeCheckExpr::Resolve (expr.get_scrutinee_expr ().get ());

bool saw_error = false;
std::vector<TyTy::BaseType *> kase_block_tys;
for (auto &kase : expr.get_match_cases ())
{
Expand All @@ -1475,7 +1476,10 @@ TypeCheckExpr::visit (HIR::MatchExpr &expr)
TyTy::BaseType *kase_arm_ty
= TypeCheckPattern::Resolve (pattern.get (), scrutinee_tyty);
if (kase_arm_ty->get_kind () == TyTy ::TypeKind::ERROR)
return;
{
saw_error = true;
continue;
}

TyTy::BaseType *checked_kase = unify_site (
expr.get_mappings ().get_hirid (),
Expand All @@ -1484,14 +1488,19 @@ TypeCheckExpr::visit (HIR::MatchExpr &expr)
TyTy::TyWithLocation (kase_arm_ty, pattern->get_locus ()),
expr.get_locus ());
if (checked_kase->get_kind () == TyTy::TypeKind::ERROR)
return;
{
saw_error = true;
continue;
}
}

// check the kase type
TyTy::BaseType *kase_block_ty
= TypeCheckExpr::Resolve (kase.get_expr ().get ());
kase_block_tys.push_back (kase_block_ty);
}
if (saw_error)
return;

if (kase_block_tys.size () == 0)
{
Expand Down
118 changes: 93 additions & 25 deletions gcc/rust/typecheck/rust-hir-type-check-pattern.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,32 +43,100 @@ TypeCheckPattern::Resolve (HIR::Pattern *pattern, TyTy::BaseType *parent)
void
TypeCheckPattern::visit (HIR::PathInExpression &pattern)
{
infered = TypeCheckExpr::Resolve (&pattern);

/*
* We are compiling a PathInExpression, which can't be a Struct or Tuple
* pattern. We should check that the declaration we are referencing IS NOT a
* struct pattern or tuple with values.
*/
// Pattern must be enum variants, sturcts, constants, or associated constansts
Copy link
Contributor

Choose a reason for hiding this comment

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

sturcts and constansts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :)

Comment on lines -48 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this original comment incorrect? I thought PathInExpression could only reference a unit variant (not tuple variant or struct variant).

Copy link
Contributor

Choose a reason for hiding this comment

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

Or would you get a Hir::PathInExpression for unit structs, aka
struct Unit;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh the original message is okay but it lacks constants and associated constants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just split this comment to one here and one in line 100

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my only problem is that instead of
" Pattern must be enum variants, sturcts, constants, or associated constansts"
It should be
" Pattern must be enum variants, unit structs, constants, or associated constants"
right?

Your new comment is more concise which is nice

TyTy::BaseType *pattern_ty = TypeCheckExpr::Resolve (&pattern);

NodeId ref_node_id = UNKNOWN_NODEID;
bool maybe_item = false;
maybe_item
|= resolver->lookup_resolved_name (pattern.get_mappings ().get_nodeid (),
&ref_node_id);
maybe_item
|= resolver->lookup_resolved_type (pattern.get_mappings ().get_nodeid (),
&ref_node_id);
Comment on lines +51 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there the same line twice? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First one checks variable names and second one checks type names. E.g. first for static items, second for type aliases

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave this one to the judgement of the code maintainers. I'm not familiar with how this code works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you dont recognize different methods are called

Copy link
Contributor

Choose a reason for hiding this comment

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

oh lol. I'm sorry. I spent so long reading that code and didn't notice

bool path_is_const_item = false;

if (maybe_item)
{
Comment on lines +59 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, lookup_resolved_type will fail if we need to do type inferencing. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK resolving (variable names and type names) is done before lowering AST into HIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section in rustc guide might be useful.
https://rustc-dev-guide.rust-lang.org/overview.html#lexing-and-parsing

tl::optional<HirId> definition_id
= mappings.lookup_node_to_hir (ref_node_id);
rust_assert (definition_id.has_value ());
HirId def_id = definition_id.value ();

tl::optional<HIR::Item *> hir_item = mappings.lookup_hir_item (def_id);
// If the path refrerences an item, it must be constants or structs.
if (hir_item.has_value ())
{
HIR::Item *item = hir_item.value ();
if (item->get_item_kind () == HIR::Item::ItemKind::Constant)
{
path_is_const_item = true;
}
else if (item->get_item_kind () != HIR::Item::ItemKind::Struct)
{
HIR::Item *item = hir_item.value ();
std::string item_kind
= HIR::Item::item_kind_string (item->get_item_kind ());

std::string path_buf;
for (size_t i = 0; i < pattern.get_segments ().size (); i++)
{
HIR::PathExprSegment &seg = pattern.get_segments ().at (i);
path_buf += seg.as_string ();
if (i != pattern.get_segments ().size () - 1)
path_buf += "::";
}

rich_location rich_locus (
line_table, pattern.get_final_segment ().get_locus ());
rich_locus.add_fixit_replace (
"not a unit struct, unit variant or constatnt");
rust_error_at (rich_locus, ErrorCode::E0532,
"expected unit struct, unit variant or constant, "
"found %s %<%s%>",
item_kind.c_str (), path_buf.c_str ());
return;
Comment on lines +90 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better error message :)

}
}
}

rust_assert (infered->get_kind () == TyTy::TypeKind::ADT);
TyTy::ADTType *adt = static_cast<TyTy::ADTType *> (infered);
// If the path is a constructor, it must be a unit struct or unit variants.
if (!path_is_const_item && pattern_ty->get_kind () == TyTy::TypeKind::ADT)
{
TyTy::ADTType *adt = static_cast<TyTy::ADTType *> (pattern_ty);
rust_assert (adt->get_variants ().size () > 0);

HirId variant_id = UNKNOWN_HIRID;
bool ok
= context->lookup_variant_definition (pattern.get_mappings ().get_hirid (),
&variant_id);
rust_assert (ok);
TyTy::VariantDef *variant = adt->get_variants ().at (0);
if (adt->is_enum ())
{
HirId variant_id = UNKNOWN_HIRID;
bool ok = context->lookup_variant_definition (
pattern.get_mappings ().get_hirid (), &variant_id);
rust_assert (ok);

TyTy::VariantDef *variant = nullptr;
ok = adt->lookup_variant_by_id (variant_id, &variant);
ok = adt->lookup_variant_by_id (variant_id, &variant);
rust_assert (ok);
}

TyTy::VariantDef::VariantType vty = variant->get_variant_type ();
if (variant->get_variant_type () != TyTy::VariantDef::VariantType::NUM)
{
std::string variant_type = TyTy::VariantDef::variant_type_string (
variant->get_variant_type ());

rich_location rich_locus (line_table,
pattern.get_final_segment ().get_locus ());
rich_locus.add_fixit_replace (
"not a unit struct, unit variant or constatnt");
rust_error_at (rich_locus, ErrorCode::E0532,
"expected unit struct, unit variant or constant, "
"found %s variant %<%s::%s%>",
variant_type.c_str (), adt->get_name ().c_str (),
variant->get_identifier ().c_str ());
return;
}

if (vty != TyTy::VariantDef::VariantType::NUM)
rust_error_at (
pattern.get_final_segment ().get_locus (), ErrorCode::E0532,
"expected unit struct, unit variant or constant, found tuple variant");
infered = pattern_ty;
}
}

void
Expand Down Expand Up @@ -100,8 +168,8 @@ TypeCheckPattern::visit (HIR::TupleStructPattern &pattern)
rust_assert (ok);
}

// error[E0532]: expected tuple struct or tuple variant, found struct variant
// `Foo::D`, E0532 by rustc 1.49.0 , E0164 by rustc 1.71.0
// error[E0532]: expected tuple struct or tuple variant, found struct
// variant `Foo::D`, E0532 by rustc 1.49.0 , E0164 by rustc 1.71.0
if (variant->get_variant_type () != TyTy::VariantDef::VariantType::TUPLE)
{
std::string variant_type
Expand Down Expand Up @@ -203,8 +271,8 @@ TypeCheckPattern::visit (HIR::StructPattern &pattern)
rust_assert (ok);
}

// error[E0532]: expected tuple struct or tuple variant, found struct variant
// `Foo::D`
// error[E0532]: expected tuple struct or tuple variant, found struct
// variant `Foo::D`
if (variant->get_variant_type () != TyTy::VariantDef::VariantType::STRUCT)
{
std::string variant_type
Expand Down
2 changes: 1 addition & 1 deletion gcc/testsuite/rust/compile/issue-2324-2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ enum State {
fn print_on_failure(state: &State) {
match *state {
State::Succeeded => (),
State::Failed => (), // { dg-error "expected unit struct, unit variant or constant, found tuple variant" }
State::Failed => (), // { dg-error "expected unit struct, unit variant or constant, found struct variant" }
_ => ()
}
}
Expand Down
30 changes: 30 additions & 0 deletions gcc/testsuite/rust/compile/match9.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
enum E {
A(),
B,
}

const CONST_E: E = E::A();

static static_e: E = E::A();

type type_alias = E;

fn f(e: E) {
match e {
E::A => {}
// { dg-error "expected unit struct, unit variant or constant, found tuple variant .E::A." "" { target *-*-* } .-1 }
E::B => {}
crate::CONST_E => {}
crate::type_alias => {}
// { dg-error "expected unit struct, unit variant or constant, found type alias .crate::type_alias." "" { target *-*-* } .-1 }
crate::E => {}
// { dg-error "expected unit struct, unit variant or constant, found enum .crate::E." "" { target *-*-* } .-1 }
crate::static_e => {}
// { dg-error "expected unit struct, unit variant or constant, found static .crate::static_e." "" { target *-*-* } .-1 }
crate::f => {}
// { dg-error "expected unit struct, unit variant or constant, found function .crate::f." "" { target *-*-* } .-1 }
_ => {}
}
}

fn main() {}
Loading