Skip to content

nr2.0: Separate out canonical path handling #3776

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

Jump to bottom
Merged
merged 1 commit into from
Jun 9, 2025
Merged

nr2.0: Separate out canonical path handling #3776

merged 1 commit into from
Jun 9, 2025

Conversation

powerboat9
Copy link
Collaborator

@powerboat9 powerboat9 commented May 5, 2025 •
edited
Loading

Depends on a few other PRs

@powerboat9 powerboat9 force-pushed the fix-can branch 4 times, most recently from 326ac5d to 1156370 Compare May 7, 2025 22:26
@powerboat9
Copy link
Collaborator Author

powerboat9 commented May 7, 2025 •
edited
Loading

Now only depends on #3768

@powerboat9 powerboat9 requested review from P-E-P, CohenArthur and philberty and removed request for P-E-P May 7, 2025 22:42
@powerboat9 powerboat9 force-pushed the fix-can branch 2 times, most recently from 6ed28d3 to 60d6760 Compare May 8, 2025 17:47
@powerboat9 powerboat9 marked this pull request as ready for review May 8, 2025 17:52
philberty
philberty reviewed May 13, 2025
View reviewed changes
gcc/rust/resolve/rust-name-resolution-context.cc
// this should bring us roughly to parity with nr1.0
// since nr1.0 doesn't seem to handle canonical paths for generics
// quite right anyways
return Resolver::CanonicalPath::new_seg (UNKNOWN_NODEID, "XXX");
Copy link
Member

Choose a reason for hiding this comment

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

yeah generics are a right pain to handle sometims the paths always default to their non monomorphized version sometimes not but yeah. In order to do it all "correctly" there is going to need to be some mappings between the segment and the conical path segments.. We get away with it for now because it doesnt matter, plus rust will never get a stable ABI and the hash at the end of the managled symbol.

My current feel is that if we can do what we can with canonical paths for now purely in name resolution we get like 80% the way there and its ok.

philberty
philberty reviewed May 13, 2025
View reviewed changes
gcc/rust/util/rust-canonical-path.h
@@ -57,10 +57,11 @@ class CanonicalPath
return *this;
}

static CanonicalPath new_seg (NodeId id, const std::string &path)
static CanonicalPath new_seg (NodeId id, std::string path)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure i like getting rid of the const reference here. I personally like to avoid move on std::string but if this is the style in nr2 then go for it.

Copy link
Collaborator Author

@powerboat9 powerboat9 May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was having some issue with binding to a constant lvalue reference -- in hindsight, I'm not sure why

philberty
philberty approved these changes May 13, 2025
View reviewed changes
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

LGTM

@powerboat9
nr2.0: Separate out canonical path handling
0261da0 
This should improve our canonical path handling, without requiring
further tweaks to ForeverStack. This may also help if, in the future, we
have to move canonical path calculation to later compilation phases for
proper handling of generics.

gcc/rust/ChangeLog:

	* backend/rust-compile-base.cc
	(HIRCompileBase::compile_function): Since canonical paths
	returned from nr2.0 now include the crate name, avoid prepending
	the crate name again.

	* backend/rust-compile-implitem.cc (CompileTraitItem::visit):
	Use NameResolutionContext::to_canonical_path instead of
	ForeverStack::to_canonical_path.
	* backend/rust-compile-item.cc (CompileItem::visit): Likewise.
	* typecheck/rust-hir-type-check-enumitem.cc
	(TypeCheckEnumItem::visit): Likewise.
	* typecheck/rust-hir-type-check-implitem.cc
	(TypeCheckImplItem::visit): Likewise.
	* typecheck/rust-hir-type-check-item.cc (TypeCheckItem::visit):
	Likewise.
	* typecheck/rust-hir-type-check.cc
	(TraitItemReference::get_type_from_fn): Likewise.

	* resolve/rust-default-resolver.cc (DefaultResolver::visit): Add
	Crate and EnumItem instance visitors, handle canonical path
	context scoping.
	* resolve/rust-default-resolver.h (DefaultResolver::visit): Add
	Crate and EnumItem instance visitors.
	* resolve/rust-early-name-resolver-2.0.cc (Early::go): Visit
	instances of Crate using the virtual member function visit.
	* resolve/rust-forever-stack.h
	(ForeverStack::to_canonical_path): Remove function declaration.
	* resolve/rust-forever-stack.hxx
	(ForeverStack::to_canonical_path): Remove function definition.
	* resolve/rust-late-name-resolver-2.0.cc (Late::go): Visit
	instances of Crate using the virtual member function visit.
	* resolve/rust-name-resolution-context.cc
	(CanonicalPathRecordCrateRoot::as_path): New function definition.
	(CanonicalPathRecordNormal::as_path): Likewise.
	(CanonicalPathRecordLookup::as_path): Likewise.
	(CanonicalPathRecordImpl::as_path): Likewise.
	(CanonicalPathRecordTraitImpl::as_path): Likewise.
	(NameResolutionContext::NameResolutionContext): Initialize
	member variable canonical_ctx.
	* resolve/rust-name-resolution-context.h: Include "rust-item.h".
	(class NameResolutionContext): Forward declare class.
	(class CanonicalPathRecord): New class.
	(class CanonicalPathRecordWithParent): Likewise.
	(class CanonicalPathRecordCrateRoot): Likewise.
	(class CanonicalPathRecordNormal): Likewise.
	(class CanonicalPathRecordLookup): Likewise.
	(class CanonicalPathRecordImpl): Likewise.
	(class CanonicalPathRecordTraitImpl): Likewise.
	(class CanonicalPathCtx): Likewise.
	(NameResolutionContext::canonical_ctx): New member variable.
	(NameResolutionContext::to_canonical_path): New member function.
	* resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::go):
	Visit instances of Crate with the virtual member function visit.
	(TopLevel::visit): Handle canonical path context scoping for
	external crates, use DefaultResolver::visit when visiting
	instances of StructStruct.
	* util/rust-canonical-path.h (CanonicalPath::new_seg): Take path
	parameter by-value, as a duplicate instance will be constructed
	regardless.

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: Remove canonical_paths1.rs.
@powerboat9
Copy link
Collaborator Author

I've made a few tweaks to improve the CanonicalPathRecord tree structure and ExternCrate handling (although I'll be sticking better support for the latter in an upcoming PR)

@powerboat9
Copy link
Collaborator Author

@CohenArthur

CohenArthur
CohenArthur approved these changes Jun 9, 2025
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

so, I really like the features that this PR has but I'm not too comfortable with the implementation. I think it is very complicated and gets a bit overwhelming.

I think we should merge this and I think this is great work. At the same time, I think we should have a look at improving it - but I'm not quite sure how. Maybe adding some template functions and using the pointer to member operator would clean this up, e.g. to avoid always calling .get_node_id() and passing this in lambdas. This could maybe help clear up the code and remove all those inner_fn_1 and inner_fn_2 and sometimes inner_fn_3 lambdas that make this harder to understand.

Ideally for most visitors we should have just one lambda which takes care of calling DefautResolver::visit and ctx.canonical_ctx.scope if that makes sense. e.g. I think this is already slightly more readable even though I'm still not a super fan, as it puts less clutter in the visitor's outermost scope.

void
DefaultResolver::visit (AST::Module &module)
{
  auto item_fn = [this, &module] () {
    auto item_fn
      = [this, &module] () { AST::DefaultASTVisitor::visit (module); };

    ctx.canonical_ctx.scope (module.get_node_id (), module.get_name (),
			     std::move (item_fn));
  };

  ctx.scoped (Rib::Kind::Module, module.get_node_id (), item_fn,
	      module.get_name ());
}

gcc/rust/resolve/rust-default-resolver.cc
Comment on lines +152 to +153
ctx.canonical_ctx.scope_impl (impl, std::move (inner_fn_2));
};
Copy link
Member

Choose a reason for hiding this comment

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

why do we use .scope_impl(() instead of .scope() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the canonical path of an inherent impl block is dependent on its impl type

@CohenArthur CohenArthur added this pull request to the merge queue Jun 9, 2025
Merged via the queue into Rust-GCC:master with commit c8153f9 Jun 9, 2025
12 checks passed
@powerboat9 powerboat9 deleted the fix-can branch June 9, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philberty philberty philberty approved these changes

@CohenArthur CohenArthur CohenArthur approved these changes

@P-E-P P-E-P Awaiting requested review from P-E-P

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@powerboat9 @philberty @CohenArthur