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

Context aware types in ast_tools schema #107

Open
rzvxa opened this issue Sep 6, 2024 · 3 comments
Open

Context aware types in ast_tools schema #107

rzvxa opened this issue Sep 6, 2024 · 3 comments

Comments

@rzvxa
Copy link
Collaborator

rzvxa commented Sep 6, 2024

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 oxc-project/oxc#5427 (comment)

@overlookmotel
Copy link

overlookmotel commented Sep 7, 2024

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
Copy link

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
Copy link
Collaborator Author

rzvxa commented Sep 7, 2024

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.

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

No branches or pull requests

2 participants