Add check for path patterns. #3125

tamaroning · 2024-08-10T02:48:41Z

Addresses #2553 #2094
Fix some incorrect error messages introduced in #3118

gcc/rust/ChangeLog:

	* hir/tree/rust-hir.cc (Item::item_kind_string): New function.
	* hir/tree/rust-hir.h: New function.
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::visit):
		Modify to check all arms in match expressions even if some of them
		has errors.
	* typecheck/rust-hir-type-check-pattern.cc (TypeCheckPattern::visit):
		Add and fix check for path patterns.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-2324-2.rs: Fix error message.
	* rust/compile/match8.rs: New test.

tamaroning · 2024-08-10T06:55:02Z

Let me resolve conflicts.

liamnaddell · 2024-08-10T13:39:37Z

gcc/rust/typecheck/rust-hir-type-check-pattern.cc

-   * 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


sturcts and constansts

liamnaddell · 2024-08-10T13:40:27Z

gcc/rust/typecheck/rust-hir-type-check-pattern.cc

-  /*
-   * 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


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

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

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

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

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

liamnaddell · 2024-08-10T13:42:07Z

gcc/rust/typecheck/rust-hir-type-check-pattern.cc

+  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);


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

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

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

Maybe you dont recognize different methods are called

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

liamnaddell · 2024-08-10T13:44:33Z

gcc/rust/typecheck/rust-hir-type-check-pattern.cc

+	      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;


Much better error message :)

liamnaddell · 2024-08-10T13:53:58Z

gcc/rust/typecheck/rust-hir-type-check-pattern.cc

+  if (maybe_item)
+    {


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

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

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

liamnaddell · 2024-08-10T13:58:08Z

gcc/testsuite/rust/compile/match8.rs

+
+static static_e: E = E::A();
+
+struct S;


Shouldn't we also add a success test for unit structs?

Yes, i also found cases for constants are missing:)

liamnaddell · 2024-08-10T13:58:16Z

gcc/testsuite/rust/compile/match8.rs

+        crate::type_alias => {}
+        // { dg-error "expected unit struct, unit variant or constant, found type alias .crate::type_alias." "" { target *-*-* } .-1 }


Great test!

tamaroning · 2024-08-12T10:49:15Z

I did not add tests for struct because HIR path prober for structs has not been implemented yet. Thus typecheck for struct path pattern (e.g. crate::S => {}) seems to fail.
tests for them will be added once it is implemented.

https://github.com/tamaroning/gccrs/blob/96ec7ca1e096e8ba42632b76410d5cbf978b94cb/gcc/rust/typecheck/rust-hir-path-probe.h#L29-L44

P-E-P

LGTM, could you resolve conflicts ? I will merge upon resolution

gcc/rust/ChangeLog: * hir/tree/rust-hir.cc (Item::item_kind_string): New function. * hir/tree/rust-hir.h: New function. * typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::visit): Modify to check all arms in match expressions even if some of them has errors. * typecheck/rust-hir-type-check-pattern.cc (TypeCheckPattern::visit): Add and fix check for path patterns. gcc/testsuite/ChangeLog: * rust/compile/issue-2324-2.rs: Fix error message. * rust/compile/match9.rs: New test. Signed-off-by: Raiki Tamura <[email protected]>

tamaroning · 2024-08-28T14:30:32Z

Thanks for review. Done :)

tamaroning mentioned this pull request Aug 10, 2024

Add error message for E0532 #3118

Merged

tamaroning force-pushed the feat/e0532-path-pattern branch from c1fde94 to 9e1e0c7 Compare August 10, 2024 07:05

liamnaddell reviewed Aug 10, 2024

View reviewed changes

tamaroning force-pushed the feat/e0532-path-pattern branch from 9e1e0c7 to 96ec7ca Compare August 12, 2024 10:42

P-E-P approved these changes Aug 28, 2024

View reviewed changes

tamaroning force-pushed the feat/e0532-path-pattern branch from 96ec7ca to a62e4d7 Compare August 28, 2024 14:26

CohenArthur added this pull request to the merge queue Aug 28, 2024

Merged via the queue into Rust-GCC:master with commit 5da381f Aug 28, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for path patterns. #3125

Add check for path patterns. #3125

tamaroning commented Aug 10, 2024 •

edited

Loading

tamaroning commented Aug 10, 2024

liamnaddell Aug 10, 2024

tamaroning Aug 10, 2024

liamnaddell Aug 10, 2024

liamnaddell Aug 10, 2024

tamaroning Aug 10, 2024

tamaroning Aug 10, 2024

liamnaddell Aug 10, 2024

liamnaddell Aug 10, 2024

tamaroning Aug 10, 2024

liamnaddell Aug 10, 2024

tamaroning Aug 10, 2024

liamnaddell Aug 10, 2024

liamnaddell Aug 10, 2024

liamnaddell Aug 10, 2024

tamaroning Aug 10, 2024

tamaroning Aug 10, 2024

liamnaddell Aug 10, 2024

tamaroning Aug 10, 2024

liamnaddell Aug 10, 2024

tamaroning commented Aug 12, 2024 •

edited

Loading

P-E-P left a comment •

edited

Loading

tamaroning commented Aug 28, 2024

		crate::type_alias => {}
		// { dg-error "expected unit struct, unit variant or constant, found type alias .crate::type_alias." "" { target --* } .-1 }

Add check for path patterns. #3125

Add check for path patterns. #3125

Conversation

tamaroning commented Aug 10, 2024 • edited Loading

tamaroning commented Aug 10, 2024

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

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

tamaroning commented Aug 12, 2024 • edited Loading

P-E-P left a comment • edited Loading

Choose a reason for hiding this comment

tamaroning commented Aug 28, 2024

tamaroning commented Aug 10, 2024 •

edited

Loading

tamaroning commented Aug 12, 2024 •

edited

Loading

P-E-P left a comment •

edited

Loading