Reduce the amount of raw pointer the HIR API #2878

P-E-P · 2024-02-27T12:58:05Z

This draft PR is a first attempt at reducing the amount of raw pointers in the HIR API by replacing those either with references or smart pointers.

Requires #2867 to be merged first.

This PR probably requires a lot of other changes, I was trying to get something that compiles again quickly and many things may be wrong.

@philberty Is this even something you'd like to see merged ?

P-E-P · 2024-02-27T17:36:39Z

gcc/rust/ast/rust-ast-collector.cc

@@ -2570,8 +2570,7 @@ TokenCollector::visit (LetStmt &stmt)
 {
  push (Rust::Token::make (LET, stmt.get_locus ()));
  auto &pattern = stmt.get_pattern ();
-  if (pattern)
-    visit (pattern);
+  visit (pattern);


Need to keep the condition here.

philberty · 2024-09-27T11:09:09Z

I think this is a good idea, I did alot of hacky stuff in the HIR just to get stuff going but yeah this was lazy of me. If you can i would persue finishing this PR

P-E-P · 2024-09-30T08:32:09Z

It's been a long time since I last updated this PR. This was compiling fine but there was a lot of errors (mainly use after free and segfaults). So even if we manage to rebase it on master it'll still be a long way until merge.

I put you as reviewer because the HIR is mostly your work and I didn't want to bring huge change that you would disagree with.

Attempt to use references and smart pointers whenever possible. gcc/rust/ChangeLog: * backend/rust-compile-base.cc (HIRCompileBase::compile_function_body): Remove usage of get function to retrieve a raw pointer. * backend/rust-compile-base.h: Change API usage from raw pointer to a reference. * backend/rust-compile-block.cc (CompileBlock::compile): Likewise. (CompileBlock::visit): Likewise. (CompileConditionalBlocks::visit): Likewise. * backend/rust-compile-block.h: Likewise. * backend/rust-compile-expr.cc (CompileExpr::Compile): Likewise. (CompileExpr::visit): Likewise. (check_match_scrutinee): Likewise. (CompileExpr::array_value_expr): Likewise. (CompileExpr::array_copied_expr): Likewise. (CompileExpr::generate_closure_function): Likewise. (CompileExpr::generate_possible_fn_trait_call): Likewise. * backend/rust-compile-expr.h: Likewise. * backend/rust-compile-fnparam.cc (CompileFnParam::compile): Likewise. (CompileFnParam::visit): Likewise. * backend/rust-compile-fnparam.h: Likewise. * backend/rust-compile-implitem.cc (CompileTraitItem::visit): Likewise. * backend/rust-compile-intrinsic.cc (compile_fn_params): Likewise. * backend/rust-compile-item.cc (CompileItem::visit): Likewise. * backend/rust-compile-pattern.cc (CompilePatternCheckExpr::visit): Likewise. (compile_range_pattern_bound): Likewise. (CompilePatternBindings::visit): Likewise. (CompilePatternLet::visit): Likewise. * backend/rust-compile-pattern.h: Likewise. * backend/rust-compile-resolve-path.cc (ResolvePathRef::resolve): Likewise. (HIRCompileBase::query_compile): Likewise. * backend/rust-compile-stmt.cc (CompileStmt::visit): Likewise. * backend/rust-compile-struct-field-expr.cc (CompileStructExprField::Compile): Likewise. (CompileStructExprField::visit): Likewise. * backend/rust-compile-struct-field-expr.h: Likewise. * backend/rust-compile-type.cc (TyTyResolveCompile::visit): Likewise. * backend/rust-compile-var-decl.h: Likewise. * backend/rust-compile.cc: Likewise. * backend/rust-mangle-v0.cc (v0_inherent_or_trait_impl_path): Likewise. * checks/errors/borrowck/rust-bir-builder-expr-stmt.cc (ExprStmtBuilder::visit): Likewise. * checks/errors/borrowck/rust-bir-builder-lazyboolexpr.h: Likewise. * checks/errors/borrowck/rust-bir-builder-pattern.h: Likewise. * checks/errors/borrowck/rust-bir-builder-struct.h: Likewise. * checks/errors/borrowck/rust-bir-builder.h: Likewise. * checks/errors/borrowck/rust-function-collector.h: Likewise. * checks/errors/privacy/rust-privacy-reporter.cc (PrivacyReporter::check_type_privacy): Likewise. (PrivacyReporter::visit): Likewise. * checks/errors/privacy/rust-privacy-reporter.h: Likewise. * checks/errors/privacy/rust-reachability.cc (ReachabilityVisitor::visit): Likewise. * checks/errors/rust-const-checker.cc (ConstChecker::visit): Likewise. * checks/errors/rust-unsafe-checker.cc (UnsafeChecker::visit): Likewise. * checks/lints/rust-lint-marklive.cc (MarkLive::visit): Likewise. * checks/lints/rust-lint-marklive.h: Likewise. * hir/rust-hir-dump.cc (Dump::visit): Likewise. * hir/tree/rust-hir-expr.h: Likewise. * hir/tree/rust-hir-item.h: Likewise. * hir/tree/rust-hir-path.h: Likewise. * hir/tree/rust-hir-pattern.h: Likewise. * hir/tree/rust-hir-stmt.h: Likewise. * hir/tree/rust-hir-type.h: Likewise. * hir/tree/rust-hir.h: Likewise. * typecheck/rust-autoderef.cc: Likewise. * typecheck/rust-hir-dot-operator.cc (MethodResolver::select): Likewise. * typecheck/rust-hir-inherent-impl-overlap.h: Likewise. * typecheck/rust-hir-path-probe.cc (PathProbeType::process_impl_item_candidate): Likewise. (PathProbeImplTrait::process_trait_impl_items_for_candidates): Likewise. * typecheck/rust-hir-trait-resolve.cc (TraitResolver::resolve_trait): Likewise. (TraitItemReference::resolve_item): Likewise. * typecheck/rust-hir-type-check-base.cc: Likewise. * typecheck/rust-hir-type-check-base.h: Likewise. * typecheck/rust-hir-type-check-enumitem.cc (TypeCheckEnumItem::Resolve): Likewise. (TypeCheckEnumItem::visit): Likewise. * typecheck/rust-hir-type-check-enumitem.h: Likewise. * typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::Resolve): Likewise. (TypeCheckExpr::visit): Likewise. (TypeCheckExpr::resolve_fn_trait_call): Likewise. * typecheck/rust-hir-type-check-expr.h: Likewise. * typecheck/rust-hir-type-check-implitem.cc (TypeCheckTopLevelExternItem::Resolve): Likewise. (TypeCheckTopLevelExternItem::visit): Likewise. (TypeCheckImplItem::visit): Likewise. (TypeCheckImplItemWithTrait::visit): Likewise. * typecheck/rust-hir-type-check-implitem.h: Likewise. * typecheck/rust-hir-type-check-item.cc (TypeCheckItem::visit): Likewise. (TypeCheckItem::resolve_impl_item): Likewise. (TypeCheckItem::resolve_impl_block_substitutions): Likewise. (TypeCheckItem::resolve_impl_block_self): Likewise. * typecheck/rust-hir-type-check-path.cc (TypeCheckExpr::visit): Likewise. (TypeCheckExpr::resolve_segments): Likewise. * typecheck/rust-hir-type-check-pattern.cc (TypeCheckPattern::Resolve): Likewise. (TypeCheckPattern::visit): Likewise. (ClosureParamInfer::Resolve): Likewise. (ClosureParamInfer::visit): Likewise. * typecheck/rust-hir-type-check-pattern.h: Likewise. * typecheck/rust-hir-type-check-stmt.cc (TypeCheckStmt::Resolve): Likewise. (TypeCheckStmt::visit): Likewise. * typecheck/rust-hir-type-check-stmt.h: Likewise. * typecheck/rust-hir-type-check-struct-field.h: Likewise. * typecheck/rust-hir-type-check-struct.cc (TypeCheckStructExpr::TypeCheckStructExpr): Likewise. (TypeCheckStructExpr::Resolve): Likewise. (TypeCheckStructExpr::resolve): Likewise. (TypeCheckStructExpr::visit): Likewise. * typecheck/rust-hir-type-check-type.cc (TypeCheckResolveGenericArguments::resolve): Likewise. (TypeCheckType::Resolve): Likewise. (TypeCheckType::visit): Likewise. (TypeCheckType::resolve_root_path): Likewise. (TypeResolveGenericParam::Resolve): Likewise. (TypeResolveGenericParam::visit): Likewise. (ResolveWhereClauseItem::visit): Likewise. * typecheck/rust-hir-type-check-type.h: Likewise. * typecheck/rust-hir-type-check.cc (TraitItemReference::get_type_from_fn): Likewise. * typecheck/rust-hir-type-check.h: Likewise. * typecheck/rust-type-util.cc (query_type): Likewise. * typecheck/rust-typecheck-context.cc (TypeCheckContextItem::TypeCheckContextItem): Likewise. * typecheck/rust-tyty-bounds.cc (TypeBoundsProbe::scan): Likewise. (TypeCheckBase::get_predicate_from_bound): Likewise. * typecheck/rust-tyty-call.cc (TypeCheckCallExpr::visit): Likewise. (TypeCheckMethodCallExpr::go): Likewise. (TypeCheckMethodCallExpr::check): Likewise. * typecheck/rust-tyty-subst.cc: Likewise. * typecheck/rust-tyty.cc (BaseType::monomorphized_clone): Likewise. (VariantDef::VariantDef): Remove copy constructor. (VariantDef::operator=): Change to move operator. (VariantDef::get_discriminant): Replace return type to a reference instead of a reference to a unique pointer. (VariantDef::clone): Change to references. (VariantDef::monomorphized_clone): Likewise. (FnType::as_string): Likewise. (FnType::clone): Likewise. * typecheck/rust-tyty.h: Likewise. * util/rust-hir-map.cc (Mappings::insert_hir_impl_block): Likewise. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

gcc/rust/ChangeLog: * backend/rust-compile-base.cc: * backend/rust-compile-expr.cc (CompileExpr::visit): * backend/rust-compile-intrinsic.cc (compile_fn_params): (transmute_handler): (assume_handler): * backend/rust-compile-type.cc (TyTyResolveCompile::visit): * checks/errors/borrowck/rust-bir-builder-expr-stmt.cc (ExprStmtBuilder::visit): * checks/errors/privacy/rust-privacy-reporter.cc (PrivacyReporter::visit): * checks/errors/rust-unsafe-checker.cc (UnsafeChecker::visit): * hir/tree/rust-hir-expr.h: * hir/tree/rust-hir-item.h: * hir/tree/rust-hir.h: * typecheck/rust-hir-dot-operator.cc (MethodResolver::Select): * typecheck/rust-hir-type-check-expr.cc: * typecheck/rust-hir-type-check-implitem.cc (TypeCheckTopLevelExternItem::visit): (TypeCheckImplItem::visit): * typecheck/rust-hir-type-check-item.cc (TypeCheckItem::visit): * typecheck/rust-hir-type-check.cc (TraitItemReference::get_type_from_fn): * typecheck/rust-tyty-call.cc (TypeCheckCallExpr::visit): (TypeCheckMethodCallExpr::check): * typecheck/rust-tyty-cmp.h: * typecheck/rust-tyty.cc (BaseType::monomorphized_clone): (BaseType::is_concrete): (FnType::as_string): (FnType::is_equal): (FnType::clone): (FnType::handle_substitions): * typecheck/rust-tyty.h (class FnParam): * typecheck/rust-unify.cc (UnifyRules::expect_fndef): (UnifyRules::expect_fnptr): Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

gcc/rust/ChangeLog: * checks/lints/rust-lint-marklive.h: * hir/rust-ast-lower-expr.cc (ASTLoweringExpr::visit): * hir/rust-hir-dump.cc (Dump::visit): * hir/tree/rust-hir-expr.h (RUST_HIR_EXPR_H): (class ArrayElemsValues): (class TupleExpr): (struct StructBase): (class StructExprStructFields): (class CallExpr): (class MethodCallExpr): (class WhileLetLoopExpr): (class IfLetExpr): * hir/tree/rust-hir-path.h: * hir/tree/rust-hir-type.h: * hir/tree/rust-hir.cc (TypePath::to_trait_bound): (StructExprStructFields::as_string): * hir/tree/rust-hir.h: * typecheck/rust-hir-type-check-struct.cc (TypeCheckStructExpr::resolve): * typecheck/rust-tyty-bounds.cc: Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

gcc/rust/ChangeLog: * checks/errors/rust-unsafe-checker.h: Function overrides and produce a warning on some compilers, properly tagging it as override silent it. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

Refactor the hir tree files to remove raw pointer usage and most forward declarations. Move implementation out of headers and split headers into smaller and more manageable units. gcc/rust/ChangeLog: * Make-lang.in: Add new files. * hir/tree/rust-hir-item.h: Move Item definition and remove implementations to their corresponding cc file. * hir/tree/rust-hir-expr.h: Move implementation to the corresponding cc file. * hir/tree/rust-hir-path.h: Likewise. * hir/tree/rust-hir-pattern.h: Likewise. * hir/tree/rust-hir-stmt.h: Likewise. * hir/tree/rust-hir-type.h: Likewise. * hir/tree/rust-hir-visitor.h: Likewise. * hir/tree/rust-hir.h: Likewise. * hir/tree/rust-hir.cc (Crate::Crate): Add implementations from Crate and remove ConstGenericParam implementations to move them to their own file. * hir/tree/rust-hir-attrs.h: New file. * hir/tree/rust-hir-bound-abstract.h: New file. * hir/tree/rust-hir-bound.h: New file. * hir/tree/rust-hir-expr-abstract.h: New file. * hir/tree/rust-hir-expr.cc: New file. * hir/tree/rust-hir-generic-param.cc: New file. * hir/tree/rust-hir-generic-param.h: New file. * hir/tree/rust-hir-item.cc: New file. * hir/tree/rust-hir-literal.h: New file. * hir/tree/rust-hir-node.h: New file. * hir/tree/rust-hir-path.cc: New file. * hir/tree/rust-hir-pattern-abstract.h: New file. * hir/tree/rust-hir-simple-path.h: New file. * hir/tree/rust-hir-stmt.cc: New file. * hir/tree/rust-hir-trait-bound.h: New file. * hir/tree/rust-hir-type-abstract.cc: New file. * hir/tree/rust-hir-type-abstract.h: New file. * hir/tree/rust-hir-type-no-bounds.h: New file. * hir/tree/rust-hir-type.cc: New file. * hir/tree/rust-hir-visibility.h: New file. * hir/tree/rust-hir-visitable.h: New file. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

P-E-P · 2024-10-21T14:54:03Z

This does not look good, I should have refactored the hir files before attempting to remove the raw pointers.

A variant being moved lead to a null being created and a segfault later down the line. gcc/rust/ChangeLog: * backend/rust-compile-expr.cc (CompileExpr::visit): Call getter instead of size function. * checks/errors/privacy/rust-privacy-reporter.cc (PrivacyReporter::visit): Only check privacy if the type is present. * hir/rust-ast-lower-stmt.cc (ASTLoweringStmt::visit): Use an optional. * hir/tree/rust-hir-generic-param.h: Assert type before getting it. * hir/tree/rust-hir-item.h: Assert pointers before dereference, fix has_type condition. * hir/tree/rust-hir-path.h: Add more assertions. * hir/tree/rust-hir-stmt.cc: Change constructor with optionals. * hir/tree/rust-hir-stmt.h: Use optionals over smart pointers to emphasize these fields might be missing. * hir/tree/rust-hir.cc (LetStmt::as_string): Use getters. * typecheck/rust-hir-type-check-expr.cc: Clone structures to prevent parent's fields from being nulled by the move operation. * typecheck/rust-hir-type-check-item.cc (TypeCheckItem::visit): Use optionals. * typecheck/rust-tyty.cc: Likewise. * typecheck/rust-tyty.h: Likewise. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

gcc/rust/ChangeLog: * hir/rust-ast-lower-type.cc (ASTLowerGenericParam::visit): Forward an optional to the constructor. * hir/tree/rust-hir-item.cc (TypeParam::TypeParam): Use an optional in the constructor. (TypeParam::operator=): Ensure the TypeParam has a type properly. (TypeParam::get_type_mappings): Likewise. * hir/tree/rust-hir-item.h: Wrap the type smart pointer into an optional. * hir/tree/rust-hir.cc (TypeParam::as_string): Unwrap optional type correctly. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

P-E-P · 2024-11-12T10:18:24Z

Putting this as ready as it will be horrible and long to review, it still requires a few fixes and a rebase though.

CohenArthur

Thank you so much for doing all that work, amazing job

CohenArthur · 2024-11-12T12:06:19Z

gcc/rust/checks/errors/borrowck/rust-bir-builder-pattern.h

@@ -90,7 +90,7 @@ class PatternBindingBuilder : protected AbstractBuilder,
  void visit (HIR::PathInExpression &expression) override {}
  void visit (HIR::QualifiedPathInExpression &expression) override {}
  void visit (HIR::RangePattern &pattern) override {}
-};
+}; // namespace Rust


I think this needs to be removed? probably an automatic addition by clang-format due to mismatched brackets

CohenArthur · 2024-11-12T12:11:00Z

gcc/rust/checks/errors/rust-unsafe-checker.h

@@ -115,7 +115,7 @@ class UnsafeChecker : public HIRFullVisitor
  virtual void visit (MatchExpr &expr) override;
  virtual void visit (AwaitExpr &expr) override;
  virtual void visit (AsyncBlockExpr &expr) override;
-  virtual void visit (InlineAsm &expr);
+  virtual void visit (InlineAsm &expr) override;


I think that's part of #3244 so this needs a rebase I think?

CohenArthur · 2024-11-12T12:12:17Z

gcc/rust/hir/rust-ast-lower-expr.cc

  if (struct_expr.has_struct_base ())
    {
      HIR::Expr *translated_base = ASTLoweringExpr::translate (
 	struct_expr.get_struct_base ().get_base_struct ());
-      base = new HIR::StructBase (std::unique_ptr<HIR::Expr> (translated_base));
+      base = tl::optional (Rust::make_unique<StructBase> (


do we need to wrap this in a tl::optional ( constructor call here? I though the conversion would happen automatically

CohenArthur · 2024-11-12T12:15:06Z

gcc/rust/hir/rust-ast-lower-stmt.cc

+  auto type
+    = stmt.has_type () ? tl::optional<std::unique_ptr<Type>> (
+	std::unique_ptr<Type> (ASTLoweringType::translate (stmt.get_type ())))
+		       : tl::nullopt;
+  auto init_expression
+    = stmt.has_init_expr ()
+	? tl::optional<std::unique_ptr<Expr>> (std::unique_ptr<HIR::Expr> (
+	  ASTLoweringExpr::translate (stmt.get_init_expr ())))
+	: tl::nullopt;


those calls are really rough. If it works I'd switch it to this, or at least remove the auto and put the tl::optional on the left side.

Suggested change

auto type

= stmt.has_type () ? tl::optional<std::unique_ptr<Type>> (

std::unique_ptr<Type> (ASTLoweringType::translate (stmt.get_type ())))

: tl::nullopt;

auto init_expression

= stmt.has_init_expr ()

? tl::optional<std::unique_ptr<Expr>> (std::unique_ptr<HIR::Expr> (

ASTLoweringExpr::translate (stmt.get_init_expr ())))

: tl::nullopt;

tl::optional<std::unique_ptr<Type>> type

= stmt.has_type () ? std::unique_ptr<Type> (ASTLoweringType::translate (stmt.get_type ()))

: tl::nullopt;

tl::optional<std::unique_ptr<HIR::Expr>> init_expression

= stmt.has_init_expr ()

? std::unique_ptr<HIR::Expr> (

ASTLoweringExpr::translate (stmt.get_init_expr ()))

: tl::nullopt;

P-E-P added the cleanup label Feb 27, 2024

P-E-P added this to the GCC 14.1 release milestone Feb 27, 2024

P-E-P requested a review from philberty February 27, 2024 12:58

P-E-P self-assigned this Feb 27, 2024

P-E-P force-pushed the no-more-ref-to-sp-hir branch from 7d7a2f0 to bd0cd09 Compare February 27, 2024 17:34

P-E-P commented Feb 27, 2024

View reviewed changes

P-E-P force-pushed the no-more-ref-to-sp-hir branch from 9990268 to 8765a9c Compare March 18, 2024 16:23

CohenArthur modified the milestones: GCC 14.1 release, GCC 15.1 release Apr 15, 2024

CohenArthur removed this from the GCC 15.1 milestone Jun 14, 2024

P-E-P force-pushed the no-more-ref-to-sp-hir branch from 8765a9c to 19e3d3d Compare June 28, 2024 12:34

P-E-P added 2 commits October 14, 2024 18:21

P-E-P force-pushed the no-more-ref-to-sp-hir branch 2 times, most recently from 322881b to 9e0c8a0 Compare October 15, 2024 15:31

P-E-P force-pushed the no-more-ref-to-sp-hir branch from 9e0c8a0 to 24de06b Compare October 16, 2024 09:28

P-E-P added 2 commits October 21, 2024 16:39

Tag function as override.

0370883

gcc/rust/ChangeLog: * checks/errors/rust-unsafe-checker.h: Function overrides and produce a warning on some compilers, properly tagging it as override silent it. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>

P-E-P force-pushed the no-more-ref-to-sp-hir branch from 285dfde to 93d5288 Compare November 6, 2024 15:12

P-E-P marked this pull request as ready for review November 12, 2024 10:16

CohenArthur approved these changes Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the amount of raw pointer the HIR API #2878

Reduce the amount of raw pointer the HIR API #2878

P-E-P commented Feb 27, 2024 •

edited

Loading

P-E-P Feb 27, 2024

philberty commented Sep 27, 2024

P-E-P commented Sep 30, 2024 •

edited

Loading

P-E-P commented Oct 21, 2024

P-E-P commented Nov 12, 2024

CohenArthur left a comment

CohenArthur Nov 12, 2024

CohenArthur Nov 12, 2024

CohenArthur Nov 12, 2024

CohenArthur Nov 12, 2024

Reduce the amount of raw pointer the HIR API #2878

Are you sure you want to change the base?

Reduce the amount of raw pointer the HIR API #2878

Conversation

P-E-P commented Feb 27, 2024 • edited Loading

P-E-P Feb 27, 2024

Choose a reason for hiding this comment

philberty commented Sep 27, 2024

P-E-P commented Sep 30, 2024 • edited Loading

P-E-P commented Oct 21, 2024

P-E-P commented Nov 12, 2024

CohenArthur left a comment

Choose a reason for hiding this comment

CohenArthur Nov 12, 2024

Choose a reason for hiding this comment

CohenArthur Nov 12, 2024

Choose a reason for hiding this comment

CohenArthur Nov 12, 2024

Choose a reason for hiding this comment

CohenArthur Nov 12, 2024

Choose a reason for hiding this comment

P-E-P commented Feb 27, 2024 •

edited

Loading

P-E-P commented Sep 30, 2024 •

edited

Loading