feat(ast): add ContentEq trait.

[rzvxa]

Part of #5283

[rzvxa]

• feat(ast): add ContentEq trait. #5427 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rzvxa and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #5427 will not alter performance

_{Comparing 09-03-feat_ast_add_contenteq_trait (23285f4) with main (c782916)}

Summary

✅ 28 untouched benchmarks

[graphite-app]

Merge activity

• Sep 4, 7:52 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Sep 4, 7:52 AM EDT: Boshen added this pull request to the Graphite merge queue.
• Sep 4, 7:57 AM EDT: Boshen merged this pull request with the Graphite merge queue.

[overlookmotel]

I'm late to the party and this has already been merged, but...

As well as specific comments below, a broader thought:

Currently ContentEq is defined as pub trait ContentEq<Rhs: ?Sized = Self>. So it allows comparing one type to another type.

I wonder if this makes it broader than it needs to be?

The original problem we needed to solve was something specific: Compare AST nodes while missing out certain fields (span, scope_id etc). So I had imagined the scope of ContentEq being:

1. Comparing AST nodes.
2. Comparing an AST node to another node of the same type.

In this use case, "content equality" means the nodes have equal "core content", ignoring "context" - "context" being where it sits in AST or source text.

But in this implementation, you've made ContentEq a much broader thing that could be applied to any type, and allows comparing to any type (including other types). The meaning of "content equality" is implementation-defined, and can be whatever the implementation wants it to.

There are positives and negatives to this:

• Positive: More flexible.
• Negative: Potentially confusing. When it's this broad, what does "content equality" even mean?

To judge the balance between the pluses and minuses, some questions:

1. What other uses of "content equality" can we conceive of needing?
2. Would those equality relations best be considered "content equality", or would they be better modelled as some other equality relation? e.g. "scope equality" which would have a separate ScopeEq trait (I have no idea what "scope equality" would mean, just a spurious example)

In my personal opinion, if we don't have good answers to these questions, making ContentEq broader than the original use case might be considered "premature abstraction".

We need people to understand the difference between == and content_eq. But that's harder to do if we don't have a solid definition of "content equality".

What do you think @rzvxa?

[overlookmotel]

Could we make this a list of types to ignore, rather than field name + type pairs?

I assume we should be ignoring any Span (regardless of what field it appears in). If we add any new fields which contain a Span to AST types in future, we'll have to remember to add them here too.

Boshen had to remember to do that in his amends to #5327 (and he did remember), but if this PR wasn't fresh in the mind, it would have been very easy to forget.

[rzvxa]

I wasn't sure about it since our types aren't context-aware so for less common type names you can have code like this:

// locally used type name.
type ReferenceId = usize;
#[ast]
#[generate_derive(ContentEq)]
struct X {
    span: Span,
    foo: ReferenceId,
}

Something like ReferenceId isn't that crazy to be used by someone who isn't aware of this and would mess up their equality checks for good(it is always true in this example).

What if we ignore the Span type name but use the name + type combo for the rest? Or we can actually look at the use statements and resolve the type names, I think it would also be a welcomed change in the oxc_regular_expression crate, Atom in regex means something else, and our atom is usually used there as use oxc_span::Atom as SpanAtom, But I had to change it back to Atom in the ast file to make the codegen recognize it.

---

Edit:

Now that I'm looking at this with a clear mind I see name + type combo is useless, for a type name such as ReferenceId it would be common to choose a name like reference_id as its field identifier. So it doesn't solve anything, It only lowers the surface area of the issue. I'll scrap this idea entirely and go back to checking type names, My point about making it type-aware stays.

[rzvxa]

I'll submit a follow-up PR addressing these issues and simplifying the implementation.

[overlookmotel]

#5468 narrows the scope of ContentEq as discussed above.