Implement readonly checker on HIR by sakupan102 · Pull Request #3881 · Rust-GCC/gccrs

sakupan102 · 2025-07-07T13:13:19Z

After this PR is closed, I will delete the old read-only check and switch to the new one.

philberty · 2025-07-07T18:23:11Z

your probably missing the using statement in the visitor

CohenArthur · 2025-07-08T09:11:03Z

@@ -0,0 +1,150 @@
+// Copyright (C) 2020-2025 Free Software Foundation, Inc.


Suggested change

// Copyright (C) 2020-2025 Free Software Foundation, Inc.

// Copyright (C) 2025 Free Software Foundation, Inc.

we can just use 2025 here since it is a new file :)

CohenArthur · 2025-07-08T09:11:19Z

@@ -0,0 +1,51 @@
+// Copyright (C) 2020-2025 Free Software Foundation, Inc.


Suggested change

// Copyright (C) 2020-2025 Free Software Foundation, Inc.

// Copyright (C) 2025 Free Software Foundation, Inc.

CohenArthur · 2025-07-08T09:16:50Z

+    {
+      auto ident_pattern
+	= static_cast<HIR::IdentifierPattern *> (*maybe_pattern);
+      if (!ident_pattern->is_mut () && assignment_map[*hir_id] > 1)


do we want to check if it is greater than 0 here?

I would like to check if the value is greater than 1.
In this context, assignment_map[hir_id] == 1 means the variable has been initialized, while assignment_map[hir_id] == 0 means it has only been declared.
If the assignment count is greater than 1, the variable has been assigned more than once, so I want to emit an error if the variable is not mutable.

CohenArthur · 2025-07-08T09:17:32Z

+	   == HIR::Item::ItemKind::Static)
+    {
+      auto static_item = static_cast<HIR::StaticItem *> (*maybe_static_item);
+      if (!static_item->is_mut () && assignment_map[*hir_id] > 1)


CohenArthur · 2025-07-08T09:18:23Z

+namespace Rust {
+namespace HIR {
+
+static std::map<HirId, int> assignment_map = {};


if we are only interested in whether we've seen the HirId or not, we can use a set here

Suggested change

static std::map<HirId, int> assignment_map = {};

static std::set<HirId> assignment_map = {};

I agree that if you need the count/number of times we've seen the HirId, then a map is better 👍

powerboat9 · 2025-07-21T17:59:24Z

You can skip the #include "options.h" and assume flag_name_resolution_2_0 is true -- name resolution 1.0 is in the middle of being removed, so your code should only have to work with name resolution 2.0

CohenArthur · 2025-07-29T06:46:45Z

 DefaultHIRVisitor::walk (IfExprConseqElse &expr)
 {
-  reinterpret_cast<IfExpr &> (expr).accept_vis (*this);
+  reinterpret_cast<IfExpr &> (expr).IfExpr::accept_vis (*this);


you don't need the reinterpret_cast here:

Suggested change

reinterpret_cast<IfExpr &> (expr).IfExpr::accept_vis (*this);

expr.IfExpr::accept_vis (*this);

CohenArthur · 2025-07-29T06:47:11Z

 DefaultHIRVisitor::walk (EnumItemTuple &item_tuple)
 {
-  reinterpret_cast<EnumItem &> (item_tuple).accept_vis (*this);
+  reinterpret_cast<EnumItem &> (item_tuple).EnumItem::accept_vis (*this);


likewise and other uses in this file

CohenArthur · 2025-07-29T06:48:07Z

+  enum lvalue_use
+  {
+    lv_assign,
+    lv_increment,
+    lv_decrement,
+  };


Suggested change

enum lvalue_use

{

lv_assign,

lv_increment,

lv_decrement,

};

enum class lvalue_use

{

assign,

increment,

decrement,

};

this way we get a bit stronger namespacing

oops, meant enum class thank you @P-E-P

gcc/rust/ChangeLog: * hir/tree/rust-hir-visitor.cc (DefaultHIRVisitor::walk): Add check before calling `get_trait_ref()` Signed-off-by: Ryutaro Okada <1015ryu88@gmail.com>

gcc/rust/ChangeLog: * hir/tree/rust-hir-visitor.cc (DefaultHIRVisitor::walk): Call base class's accept_vis method Signed-off-by: Ryutaro Okada <1015ryu88@gmail.com>

gcc/rust/ChangeLog: * Make-lang.in (rust-readonly-check2.cc): Add read-only check on HIR * checks/errors/rust-readonly-check2.cc (ReadonlyChecker): Add read-only check on HIR * checks/errors/rust-readonly-check2.h (ReadonlyChecker): Add read-only check on HIR Signed-off-by: Ryutaro Okada <1015ryu88@gmail.com>

CohenArthur

I think this looks good!

sakupan102 marked this pull request as draft July 7, 2025 13:13

CohenArthur reviewed Jul 8, 2025

View reviewed changes

sakupan102 force-pushed the readonly-check branch from 5681f7f to ec0556a Compare July 21, 2025 16:43

sakupan102 marked this pull request as ready for review July 21, 2025 16:44

sakupan102 requested a review from CohenArthur July 22, 2025 01:35

sakupan102 force-pushed the readonly-check branch from ec0556a to bcf2748 Compare July 24, 2025 14:35

CohenArthur reviewed Jul 29, 2025

View reviewed changes

sakupan102 added 3 commits July 31, 2025 04:44

Add check before calling get_trait_ref()

958af92

gcc/rust/ChangeLog: * hir/tree/rust-hir-visitor.cc (DefaultHIRVisitor::walk): Add check before calling `get_trait_ref()` Signed-off-by: Ryutaro Okada <1015ryu88@gmail.com>

Call base class's accept_vis method

61c1069

gcc/rust/ChangeLog: * hir/tree/rust-hir-visitor.cc (DefaultHIRVisitor::walk): Call base class's accept_vis method Signed-off-by: Ryutaro Okada <1015ryu88@gmail.com>

sakupan102 force-pushed the readonly-check branch from bcf2748 to 8b90cca Compare July 31, 2025 11:50

sakupan102 requested a review from CohenArthur July 31, 2025 11:50

CohenArthur reviewed Jul 31, 2025

View reviewed changes

CohenArthur approved these changes Jul 31, 2025

View reviewed changes

CohenArthur added this pull request to the merge queue Aug 4, 2025

Merged via the queue into Rust-GCC:master with commit fa88250 Aug 4, 2025
13 checks passed

		@@ -0,0 +1,150 @@
		// Copyright (C) 2020-2025 Free Software Foundation, Inc.

	// Copyright (C) 2020-2025 Free Software Foundation, Inc.
	// Copyright (C) 2025 Free Software Foundation, Inc.

		@@ -0,0 +1,51 @@
		// Copyright (C) 2020-2025 Free Software Foundation, Inc.

	static std::map<HirId, int> assignment_map = {};
	static std::set<HirId> assignment_map = {};

	reinterpret_cast<IfExpr &> (expr).IfExpr::accept_vis (*this);
	expr.IfExpr::accept_vis (*this);

Uh oh!

Conversation

sakupan102 commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philberty commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakupan102 Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powerboat9 commented Jul 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CohenArthur Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CohenArthur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sakupan102 commented Jul 7, 2025 •

edited

Loading

sakupan102 Jul 9, 2025 •

edited

Loading

CohenArthur Jul 29, 2025 •

edited

Loading