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

[GuillaumeGomez]

Fixes #2154.

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

[GuillaumeGomez]

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]

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]

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]

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]

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