-
-
Notifications
You must be signed in to change notification settings - Fork 496
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
fix(ast_macros): raise compile error on invalid generate_derive
input.
#4766
fix(ast_macros): raise compile error on invalid generate_derive
input.
#4766
Conversation
34fe924
to
62ee689
Compare
8440cdf
to
862fe61
Compare
62ee689
to
4125aad
Compare
4125aad
to
a61a01f
Compare
CodSpeed Performance ReportMerging #4766 will not alter performanceComparing Summary
|
a61a01f
to
6f4c6bf
Compare
862fe61
to
60b6cf9
Compare
6f4c6bf
to
15c92ab
Compare
15c92ab
to
3bff36e
Compare
60b6cf9
to
e711be3
Compare
3bff36e
to
2b7f89e
Compare
2b7f89e
to
df19742
Compare
df19742
to
2c0b146
Compare
ca2b441
to
3850558
Compare
This is absolutely ingenious! You can fool it with: use oxc_span::GetSpan as GetSpanReal;
trait GetSpan: GetSpanReal {}
#[ast]
#[generate_derive(GetSpan)]
struct Foo { /* blah */ } But I can't see any way to catch that one, and anyone who does such a bizarre thing probably deserves everything they get! I've pushed 2 more commits.
If you're happy with those changes, please go ahead and merge. |
Crap, You are right; And I can't figure out how exactly we can prevent it. If I can think of one I'll follow it up with another PR. |
Merge activity
|
…ut. (#4766) It checks 2 things. 1) The input is a supported derive 2) The given identifier is the same as the fully qualified target trait. The latter makes sure that the trait for derive is included in the scope. Part of #4704 Here's an expanded example of how we assert traits: ```rust const _:() = { { trait AssertionTrait: ::oxc_allocator::CloneIn<'static> {} impl<T: CloneIn<'static>> AssertionTrait for T {} }; }; ``` It makes sure `CloneIn` is the same as `::oxc_allocator::CloneIn` and more importantly requires the user to include the trait if they wish to use it with `generate_derive`. It also provides LSP jump to definition.
3850558
to
f5eeebd
Compare
Just to say, this is a tiny point. For all practical purposes, this works. And I noticed this morning that "jump to definition" now works too, which is brilliant. |
Yeah, the jump to definition is a nice plus. |
## [0.24.1] - 2024-08-10 ### Features - b3c3125 linter: Overhaul unicorn/no-useless-spread (#4791) (DonIsaac) - c519295 minifier: Add `InjectGlobalVariables` plugin (`@rollup/plugin-inject`) (#4759) (Boshen) ### Bug Fixes - fff9da3 ast, ast_codegen: Use `generate_derive` instead of visitable for generating span derives. (#4747) (rzvxa) - f5eeebd ast_macros: Raise compile error on invalid `generate_derive` input. (#4766) (rzvxa) - 4d0b40a napi/transform: Fix wrong isolated declarations emit (Boshen) ### Refactor - daa0b2e ast: `oxc_ast` crate re-export AST types from other crates (#4773) (overlookmotel) - d4a3be8 ast_codegen: Line breaks between types in layout assertions (#4781) (overlookmotel) - dbb5f4c ast_codegen: Remove unnecessary imports from generated files (#4774) (overlookmotel) - 7ea058d ast_codegen: Replace Windows-style line breaks with Unix-style (#4769) (overlookmotel) - 2dea0ca ast_codegen: Consistent import order (#4761) (overlookmotel) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
It checks 2 things. 1) The input is a supported derive 2) The given identifier is the same as the fully qualified target trait.
The latter makes sure that the trait for derive is included in the scope.
Part of oxc-project/backlog#124
Here's an expanded example of how we assert traits:
It makes sure
CloneIn
is the same as::oxc_allocator::CloneIn
and more importantly requires the user to include the trait if they wish to use it withgenerate_derive
.It also provides LSP jump to definition.