test(supported_langs): check that commutative parents' children all have signatures defined #507

I'm surprised by the three components of this signature. I would expect that it's not possible to declare two records with the same name, so vec![Field("name")] would be enough, no?

I don't really know if I'm being honest. I took this from method_declaration, which does need it because of overloading, but I guess structs/records/enums don't?

Yes, not only they don't need it, but adding it will prevent the signatures to be equal if the records have different fields.

For instance, in a case like this:

public record Person (String name, String address) {}

public record Person (String firstName, String lastName, String address) {}

we do want to flag those as being duplicates, because the Java compiler will not accept that.
With the signature definition you have, those two declarations would get different signatures, so we wouldn't detect that they are in conflict.

You're totally right, I'll go fix it

looks like this:

impl Foo
where
    Foo: Copy,
{
    fn foo() -> f64 {}
    const TRUE: bool = true;
}

parses like this:

└source_file Commutative
  └impl_item
    ├impl
    ├type: identifier Foo
    ├where_clause Commutative
    │ ├where
    │ ├where_predicate Signature [[Foo], [: Copy]] // this is WIP, ignore for now
    │ │ └ ...
    │ └,
    └body: declaration_list Commutative
      ├{
      ├function_item Signature [[foo]]
      │ └ ...
      ├const_item Signature [[TRUE]]
      │ └ ...
      └}

Firstly, this can very well be marked as a commutative child, since impl blocks are reorderable.

Secondly, if we do add them as a commutative child, what would their signature be?

Theoretically, nothing stops one from having two impl blocks for the same type in the same file -- though normally one would either:

• create two blocks only if they have different where-bounds (e.g. HashMap::new has no bounds, but HashMap::insert requires the key to be hashable, so you put them in different blocks)
• create two blocks with the same (missing) bounds, and get a Clippy warning (though not by default apparently)

So in order to detect identical impl blocks, we would need to consider all of:

• item that the impl is for
• where bounds
• all the items defined inside? this one sounds especially daunting
• apparently, impl Trait for Foo is also an impl_item?.. In that case, we'd also need to look at the type field, where Trait is stored in this case. Although in that case, having two impl X for Y for the same (X,Y) is disallowed

That's a tricky case indeed. I guess we could change the grammar so that it parses impl Trait for Foo as a different node type. Then, for that node type, we could define a signature which bundles up:

• the trait
• the type
• the where_clause if any

And those all together should be unique in a given file, no?

For the general impl Foo { … } case, we could perhaps also just use vec![vec![]] as signature… (and maybe it's also a sensible temporary solution for impl Trait for Foo in the meantime)

I guess we could change the grammar so that it parses impl Trait for Foo as a different node type

I mean I understand why the grammar might not want to concern itself with that -- highlighting and stuff don't really change just because of the Trait for part. But if it would be possible to reuse "impl_body" in both impl_item and "impl_trait_item", then it could probably work I think. Though interestingly enough, not even the compiler distinguishes between the two, neither in the AST nor in the HIR.

All of this makes me think that maybe we should just restrict ourselves to vec![vec![]] after all..

this is similar to impl_item, in that one can have multiple extern "C" { ... } items in the source code, and the only real way to find out if they are identical is to go through all the functions defined inside the block (and the foreign ABI convention ("C" here) as well, of course)

Then maybe use vec![vec![]] as signature? It means the whole node is treated as a signature, which would at least prevent isomorphic elements from being added.

makes sense!

here's an example of its use:

the where clause in this:

fn foo<T>()
where
    T: Sized,
    T: Send,
{
}

gets parsed as this:

└where_clause Commutative
  ├where
  ├where_predicate
  │ ├left: identifier T
  │ └bounds: trait_bounds Commutative
  │   ├:
  │   └identifier Sized Signature [[Sized]]
  ├,
  ├where_predicate
  │ ├left: identifier T
  │ └bounds: trait_bounds Commutative
  │   ├:
  │   └identifier Send Signature [[Send]]
  └,

and we're asked to provide signatures for the where_predicates.

I initially went with the approach of left + (list of all identifiers in bound). But then I saw that use_declaration uses the simpler approach, where it creates the signature from just argument, which is everything that follows use. And so I thought where_predicate could do the same for the right half of its signature.

Although I'm not quite sure that the argument thing is safe to do in the first place? I think the following two use declarations:

use foo::{Bar, Baz};
use foo::{
    Bar,
    Baz,
};

would technically have different arguments (and thus different signatures) because of the different formatting? At least Ast::ascii_tree does show the signatures as with the formatting respected

What you did looks good to me. It's true that it will fail to identify bounds that are slightly different, but it's the best we can do given the system we have.

Concerning your example about use statements above, the deciding factor that would make the signatures different is the trailing comma (the formatting differences shouldn't make a difference since nodes in signatures are treated up to isomorphism).