oxc-project / backlog

backlog for collborators only
1 stars 0 forks source link

Context aware types in `ast_tools` schema #107

Open rzvxa opened 2 months ago

rzvxa commented 2 months ago

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.

_Originally posted by @rzvxa in https://github.com/oxc-project/oxc/pull/5427#discussion_r1744774790_

overlookmotel commented 2 months ago

I agree that in an ideal world we'd resolve type names. But do you think that's feasible in an reasonable amount of time?

And how deep do we go? Trace the chain of use statements all the way back to Cargo.toml? e.g. do we support resolving use super::X / use crate::X / use local_module::X? And how do we deal with e.g. use oxc_ast::ast::*?

This seems pretty fiendish - we'd be essentially reimplementing a chunk of rustc.

Is there more limited level of resolution we could do which covers 95% without being quite so fiendish?

Or is there an existing crate for this?

overlookmotel commented 2 months ago

Or... could we prevent the problem you're pointing out by just banning defining/importing types with same names as #[ast] types or other types which appear in AST (e.g. ReferenceId)? That's not quite as satisfying a solution, but would be much simpler than implementing a full resolution algorithm.

rzvxa commented 2 months ago

I was more concerned about our restrictions on aliased use statements. Everywhere in the oxc_regular_expression, Atom is called SpanAtom so it can be confusing for the end users of that crate. Supporting the SpanAtom in these files would be an awesome feature. But I agree disallowing these names seems like an easier approach. We can just output some const assertions in the #[ast] expansion to make sure these type names point to the right thing.

I'll create an issue for tracking this suggestion in the main repo - as I don't see that part being hard to implement - but let's keep this one open in the backlog.

I think we can do a simple hack for it instead of doing the whole type-system thing. If we resolve only use aliases we can generate custom assertions and include them in the #[ast] macro to make sure SpanAtom is indeed Span. Codegen itself doesn't care to output a valid AST from typedefs so it can just rename these aliases as soon as they are resolved and keep the alias list in a separate vector.