Rust: refactor pre_emit! and post_emit! to a trait
#19851
Conversation
redsun82
force-pushed
the
redsun82/rust-emission-trait
branch
from
e15e81e to
8443aaa
Compare
redsun82
force-pushed
the
redsun82/rust-emission-trait
branch
from
8443aaa to
d0c7550
Compare
There was a problem hiding this comment.
Pull Request Overview
Refactors the existing macro-based pre_emit!/post_emit! logic into a trait-driven approach, improving IDE support and reducing hidden macro magic. Key changes include:
- Introduce
HasTrapClassandEmission<T>traits inmappings.rsand implement them for all AST nodes inbase.rs. - Remove
pre_emit!/post_emit!macros and update theast-generatortemplates to emitself.pre_emit(node)andself.post_emit(node, label)calls. - Extend the AST generator to emit
HasTrapClassimpls and guard template sections with a newhas_special_emissionflag.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/extractor/src/translate/mappings.rs | Add HasTrapClass and Emission traits |
| rust/extractor/src/translate/base.rs | Remove macros, implement Emission<T> for AST nodes, add emit_else_branch |
| rust/ast-generator/templates/{pre_emission,post_emission}.mustache | Generate trait-based pre/post emission calls |
| rust/ast-generator/templates/trap_class_mapping.mustache | Emit HasTrapClass impls for nodes with special emission |
| rust/ast-generator/templates/extractor.mustache | Remove macro imports, update imports and include new template partials |
| rust/ast-generator/src/main.rs | Add has_special_emission helper and propagate flag |
Comments suppressed due to low confidence (1)
rust/extractor/src/translate/mappings.rs:6
- [nitpick] Consider adding a doc comment to explain the purpose of the
HasTrapClasstrait and its associatedTrapClasstype for future maintainers.
pub(crate) trait HasTrapClass: AstNode {
| @@ -1,7 +1,20 @@ | |||
| use crate::trap::{Label, TrapClass}; | |||
| use ra_ap_hir::{Enum, Function, HasContainer, Module, Semantics, Struct, Trait, Union}; | |||
| use ra_ap_ide_db::RootDatabase; | |||
| use ra_ap_syntax::{AstNode, ast, ast::RangeItem}; | |||
There was a problem hiding this comment.
The import ast::RangeItem is unused in this file. Consider removing it to clean up unused dependencies.
| use ra_ap_syntax::{AstNode, ast, ast::RangeItem}; | |
| use ra_ap_syntax::{AstNode, ast}; |
Copilot uses AI. Check for mistakes.
| @@ -52,6 +52,32 @@ fn property_name(type_name: &str, field_name: &str) -> String { | |||
| name.to_owned() | |||
| } | |||
|
|
|||
| fn has_special_emission(type_name: &str) -> bool { | |||
There was a problem hiding this comment.
We could also move this list into base.rs as a call to a macro that generates empty impl Emissions for the types that do not require special emission. That way you can keep the configuration out of the generator. Something like
no_special_emission!(AssocItem, ExternItem,...);
There was a problem hiding this comment.
the absence of special emission is on the complementary set of that list, so it would need to explicitly list all nodes/enums except the ones listed here, which seems impractical.
We could move this entirely in base.rs if we had impl specialization and we could add a blanket implementation with special cases. I wouldn't hold my breath waiting for that to become stable anytime soon though (it's not seeing a lot of activity). We could use that with a nightly toolchain, but I'd avoid that.
This refactors the macro-based magic of
pre_emitandpost_emitto use a trait onTranslator, with the corresponding ast node as generic parameter. This needs to be explicitly requested for an AST node in theast-generatorcode (changing thehas_special_emissionfunction).This is safer, less error-prone, and nicer to the IDE.