Skip to content
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

ast: Check if the #[derive] attribute is used on a compatible item #2904

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

Conversation

GuillaumeGomez
Copy link
Contributor

Fixes #2154.

Second commit is a small typo I encountered in the function I was working on.

@GuillaumeGomez GuillaumeGomez changed the title Check derive compatibility ast: Check if the #[derive] attribute is used on a compatible item Mar 5, 2024
@GuillaumeGomez GuillaumeGomez force-pushed the check-derive-compatibility branch 4 times, most recently from b5d02ac to 21079c3 Compare March 6, 2024 20:49
@CohenArthur CohenArthur self-assigned this Mar 7, 2024
gcc/rust/ChangeLog:

	* ast/rust-ast.h: Create new `Item::Kind` enum and new `Item::get_item_kind` method
	* ast/rust-item.h: Implement `Item::get_item_kind` method
	* ast/rust-macro.h: Implement `Item::get_item_kind` method
	* expand/rust-derive.cc (DeriveVisitor::derive): Check that `#[derive]` is applied
	on compatible item
	* expand/rust-expand-visitor.cc: Check if returned item is null before putting it
	into the item list

gcc/testsuite/ChangeLog:

	* rust/compile/derive_macro9.rs: New test.
gcc/rust/ChangeLog:

	* expand/rust-derive.cc (DeriveVisitor::derive): Fix typo
@GuillaumeGomez
Copy link
Contributor Author

So, seems like I'm stuck: I think the attribute visitor is overloading the methods wrongly, making them not go through the code recursively. If so, the simplest way would be for the original ASTVisitor to have two sets of methods:

  1. the current one which can be overloaded
  2. a new set which would call each visit method as it should and that could not be overloaded

Again, maybe I'm completely misunderstanding how it works and sorry for the noise.

@P-E-P
Copy link
Member

P-E-P commented Mar 13, 2024

So, seems like I'm stuck: I think the attribute visitor is overloading the methods wrongly, making them not go through the code recursively. If so, the simplest way would be for the original ASTVisitor to have two sets of methods:

1. the current one which can be overloaded

2. a new set which would call each visit method as it should and that could not be overloaded

Not entirely sure I understand what you mean. Right now we use ASTVisitor to provide the visitor interface, as well as DefaultASTVisitor to automatically recurse into the nodes. The latter can be overloaded on desired nodes to either stop the recursion, change the behavior or process whatever you want. Would this description fix your problem ?

Again, maybe I'm completely misunderstanding how it works and sorry for the noise.

don't worry, I'm glad we've got people challenging the implementation

@GuillaumeGomez
Copy link
Contributor Author

Let me try to explain a bit more: currently, if we want to recurse through the whole AST, we need to call the right methods in every implementations of the ASTVisitor, which doesn't seem great. Instead, what I was suggesting was two have two sets of methods: the current one which can be overloaded, and another one which cannot (because it's final).

The second set of methods will be in charge of going through the AST whereas the first one will only be used when you're interested into a particular item.

Now about stopping the recursion, we can make the overloadable methods return a bool. If this bool is true, it means "ok you can continue to recurse`. Example:

class ASTVisitor
{
public:
  virtual void walk (AST::Crate &crate);

protected:
  virtual void walk (AST::StructStruct &struct_item) final {
      if visit (struct_item) {
        visit_outer_attrs (struct_item);
        visit (struct_item.get_visibility ());
        for (auto &generic : struct_item.get_generic_params ())
          visit (generic);
        if (struct_item.has_where_clause ())
          visit (struct_item.get_where_clause ());
        for (auto &field : struct_item.get_fields ())
          visit (field);
      }
  }

  virtual bool visit (AST::StructStruct &struct_item) override;

No need to worry about overloading incorrectly visit methods since they are not "in charge" of going through the AST anymore.

@P-E-P
Copy link
Member

P-E-P commented Mar 14, 2024

No need to worry about overloading incorrectly visit methods since they are not "in charge" of going through the AST anymore.

Ok, It's clearer. Usually we call DefaultVisitor::visit (or whatever visitor your visitor inherits) in our overload instead of walking the ast by hand. This allow us to select either postfix/prefix walk for our given item depending on the kind of processing we wish to achieve without worrying on which subitem should be visited or not (and missing a call!). This allows us to stack multiple visitors on top (eg. ContextVisitor) of each others. This is not enforced everywhere since the default visitor thing is kinda new.

I recognize having two different functions instead of overly relying on overloading would be clearer though.

@GuillaumeGomez
Copy link
Contributor Author

If it sounds good to you, I can implement it.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 20, 2024
@P-E-P P-E-P mentioned this pull request Sep 12, 2024
4 tasks
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.

Report invalid uses of #[derive]
3 participants