Skip to content

AST: Add reconstruct_type() method #3799

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CohenArthur
Copy link
Member

  • ast: reconstruct: Add base for reconstructing and asserting different IDs

  • ast: Add reconstruct_type() method

  • ast: builder: Remove ASTTypeBuilder

    This is the first step of adding the reconstruct_*() methods as discussed on Zulip. The goal is to have something similar to clone_*(), but each time reconstructing the node and assigning it a different NodeID - and modifying clone_*() where necessary to ensure the node ID stays the same. Subsequently, this also deletes the ASTTypeBuilder visitor as it is replaced by simple calls to .reconstruct_type() instead.

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.

nice use of a template

reconstruct_vec (const std::vector<std::unique_ptr<T>> &to_reconstruct,
F method)
{
std::vector<std::unique_ptr<T>> reconstructed;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would make sense to reserve space in reconstructed, since we should have to_reconstruct.size () == reconstructed.size ()

Copy link
Collaborator

Choose a reason for hiding this comment

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

^^^

@powerboat9
Copy link
Collaborator

TLDR, it looks like you can override functions with functions that return a different type, if you're not overriding a virtual function

https://godbolt.org/z/TW1rjbMba

#include <memory>

class A
{
    public:
    std::unique_ptr<A> clone()
    {
        return std::make_unique<A> ();
    }
};

class B: public A
{
    public:
    std::unique_ptr<B> clone()
    {
        return std::make_unique<B> ();
    }
};

int main() {
    A w;
    std::unique_ptr<A> x = w.clone ();
    B y;
    std::unique_ptr<A> z = y.clone ();
}

So clone_* methods shouldn't need different names, especially since std::unique_ptr has a constructor that can do upcasting

@CohenArthur CohenArthur force-pushed the reconstruct-type branch 2 times, most recently from 8426e95 to caf27ba Compare May 22, 2025 13:40
… IDs

gcc/rust/ChangeLog:

	* ast/rust-ast.h (reconstruct): New function for calling the `reconstruct_*_impl` method
	and asserting that the new NodeId is different, and then wrap it in a unique_ptr<T>.
	(reconstruct_vec): Likewise, but for vectors of unique_ptr<T>
gcc/rust/ChangeLog:

	* ast/rust-ast.h: Add reconstruct_type() and reconstruct_type_impl()
	* ast/rust-type.h: Implement them.
	* ast/rust-macro.h: Likewise.
	* ast/rust-path.h: Likewise.
gcc/rust/ChangeLog:

	* Make-lang.in: Remove object file for ASTTypeBuilder.
	* ast/rust-ast-builder.h: Remove function.
	* ast/rust-ast-builder.cc (Builder::new_type): Likewise.
	(Builder::new_const_param): Use reconstruct_type() instead.
	(Builder::new_generic_args): Likewise.
	* expand/rust-derive-default.cc (DeriveDefault::visit_struct): Likewise.
	(DeriveDefault::visit_tuple): Likewise.
	* expand/rust-derive-eq.cc (DeriveEq::visit_tuple): Likewise.
	(DeriveEq::visit_struct): Likewise.
	(DeriveEq::visit_enum): Likewise.
	(DeriveEq::visit_union): Likewise.
	* ast/rust-ast-builder-type.cc: Removed.
	* ast/rust-ast-builder-type.h: Removed.
@CohenArthur CohenArthur requested a review from powerboat9 May 23, 2025 14:39
reconstruct_vec (const std::vector<std::unique_ptr<T>> &to_reconstruct,
F method)
{
std::vector<std::unique_ptr<T>> reconstructed;
Copy link
Collaborator

Choose a reason for hiding this comment

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

^^^

std::vector<std::unique_ptr<T>> reconstructed;

for (const auto &elt : to_reconstruct)
reconstructed.emplace_back (std::unique_ptr<T> (elt->reconstruct_impl ()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably use reconstruct_base here, for the rust_assert

Copy link
Member Author

Choose a reason for hiding this comment

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

this doesn't work for macro invocations, as reconstruct returns a std::unique_ptr<Type> but we want to store a vec<ptr<MacroInvocation>> for the pending eager macro invocations. I think in general this is a more generic version

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can declare MacroInvocation::reconstruct returning a std::unique_ptr<MacroInvocation>

@powerboat9
Copy link
Collaborator

The "^^^" comment was supposed to be a reply to #3799 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants