Skip to content

bsn! macro code quality improvements: codegen, error handling, tests, docs#23561

Open
NicoZweifel wants to merge 15 commits intobevyengine:mainfrom
NicoZweifel:bsn-fmt-&&-error-handling
Open

bsn! macro code quality improvements: codegen, error handling, tests, docs#23561
NicoZweifel wants to merge 15 commits intobevyengine:mainfrom
NicoZweifel:bsn-fmt-&&-error-handling

Conversation

@NicoZweifel
Copy link
Copy Markdown
Contributor

@NicoZweifel NicoZweifel commented Mar 28, 2026 •
edited
Loading

Objective

This originated from playing around with the bsn! macro to find out how well the lsp integration works during actual usage when typing new fields etc.

It started by adding new_spanned to report errors. I restructured as I saw fit for now as some of the locations were quite heavily indented, although I'm not opposed to doing it differently. I do think it is an improvement overall.

Changes

  • reduces nesting and indents significantly.
  • remove/cleanup some dead/unused code, see comments.
  • adds BsnCodegenCtx to clean up the function signatures and to allow accumulation of errors
  • adds usage of syn::Result return types, allowing for optional short circuit behavior.
  • adds error handling to the parsing like duplicate detection etc.
  • small tweaks here and there to fail gracefully/continue parsing.
  • improve/fix parsing in is_const, add tests.
  • add a BsnTokenStream trait

Notes

  • the PR is not about fmt support for the macro content.
  • it's not perfect, but I think it's a good base and much better than what we have now from a devex pov.
  • uses proc tests for both is_const and path_type, making them real unit tests that follow AAA without taking more space to do it, we can also type them all out if that's not alright but I think it's okay here for these tests.

Testing

cargo test
cargo bench
cargo run --example bsn

image

@NicoZweifel NicoZweifel changed the title Bsn fmt, errors and tests format Bsn gen macro code, add errors and tests Mar 28, 2026
@NicoZweifel NicoZweifel changed the title format Bsn gen macro code, add errors and tests format Bsn macro code, add errors and tests Mar 28, 2026
@NicoZweifel NicoZweifel changed the title format Bsn macro code, add errors and tests Bsn codegen fmt, add errors and tests Mar 28, 2026
@NicoZweifel NicoZweifel changed the title Bsn codegen fmt, add errors and tests Bsn codegen structure/indents, add errors and tests Mar 28, 2026
@Jondolf Jondolf added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Scenes Serialized ECS data stored on the disk S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Macros Code that generates Rust code labels Mar 28, 2026
NicoZweifel
NicoZweifel commented Mar 28, 2026
View reviewed changes
@Jondolf Jondolf added this to the 0.19 milestone Mar 28, 2026
NicoZweifel
NicoZweifel commented Mar 28, 2026
View reviewed changes
crates/bevy_scene2/macros/src/bsn/codegen.rs
function,
args,
}) => {
// NOTE: The odd turbofish line break below avoids breaking rustfmt
Copy link
Copy Markdown
Contributor Author

@NicoZweifel NicoZweifel Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I misunderstand but this didn't do anything for me so I removed it.

NicoZweifel
NicoZweifel commented Mar 28, 2026
View reviewed changes
crates/bevy_scene2/macros/src/bsn/codegen.rs
scene_list
.0
.to_tokens(bevy_scene, bevy_ecs, bevy_asset, entity_refs);
// NOTE: The odd turbofish line breaks below avoid breaking rustfmt
Copy link
Copy Markdown
Contributor Author

@NicoZweifel NicoZweifel Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as ^

@alice-i-cecile alice-i-cecile requested a review from cart March 28, 2026 19:41
@NicoZweifel NicoZweifel changed the title Bsn codegen structure/indents, add errors and tests Bsn codegen error handling and tests Mar 28, 2026
@NicoZweifel NicoZweifel changed the title Bsn codegen error handling and tests Bsn codegen error handling and tests/docs Mar 28, 2026
@NicoZweifel NicoZweifel changed the title Bsn codegen error handling and tests/docs Bsn codegen error handling, tests/docs Mar 28, 2026
@NicoZweifel NicoZweifel marked this pull request as ready for review March 28, 2026 21:58
alice-i-cecile
alice-i-cecile reviewed Mar 29, 2026
View reviewed changes
crates/bevy_scene2/macros/src/bsn/codegen.rs Outdated
}
}
None => {
// NOTE: It is very important to still produce outputs for None field values. This is what
Copy link
Copy Markdown
Member

@alice-i-cecile alice-i-cecile Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that this behavior was retained?

I think we still need to push the field access to keep autocomplete working properly.

Copy link
Copy Markdown
Contributor Author

@NicoZweifel NicoZweifel Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda forgot to get back to this actually, my bad. I wanted to add a PR comment here. Thanks for catching that. I should've turned this into a TODO and I think it should either be removed for now or I am re-adding the proposed suggestion.

I could not find that this makes a difference for me when testing the lsp on main or on this branch at an earlier time but I also noticed that there is still an issue with the auto complete for sure.

I understand the logic behind it but I wanted a way to confirm the behavior.

I would not be opposed to adding a test for it either, I do think this behavior should be tested if it is added/kept.

I'll spend a bit more time to resolve this.

Copy link
Copy Markdown
Contributor Author

@NicoZweifel NicoZweifel Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out there were a few missing pieces to make this work correctly but I am happy with it now.
I added tests to make sure the assigments and errors in the macro expansion stay in the same location.
The order is essential for the auto correct to work when there are errors present.

I tested the enum and pretty much anything I could think of:

Image Image

alice-i-cecile
alice-i-cecile reviewed Mar 29, 2026
View reviewed changes
@alice-i-cecile alice-i-cecile changed the title Bsn codegen error handling, tests/docs bsn! macro code quality improvements: codegen, error handling, tests, docs Mar 29, 2026
@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Mar 29, 2026
@NicoZweifel NicoZweifel mentioned this pull request Mar 30, 2026
Draft
@cart cart moved this to Focus in @cart's attention Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@cart cart Awaiting requested review from cart

Assignees

No one assigned

Labels

A-Scenes Serialized ECS data stored on the disk C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Macros Code that generates Rust code S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

@cart's attention
Status: Focus

Milestone

0.19

Development

Successfully merging this pull request may close these issues.

4 participants

@NicoZweifel @alice-i-cecile @cart @Jondolf