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

[Merged by Bors] - Make proc macros hygienic in bevy_reflect_derive #6752

Closed

Conversation

elbertronnie
Copy link
Contributor

@elbertronnie elbertronnie commented Nov 25, 2022

Objective

Solution

  • Replaced all the types with their fully quallified names
  • Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
  • Made a new file fq_std.rs that contains structs corresponding to commonly used Structs and Traits from std. These structs are replaced by their respective fully qualified names when used inside quote!

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change labels Nov 25, 2022
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

LGTM! Just one non-blocking comment

@MrGVSV
Copy link
Member

MrGVSV commented Nov 25, 2022

bors try

bors bot added a commit that referenced this pull request Nov 25, 2022
@bors
Copy link
Contributor

bors bot commented Nov 25, 2022

try

Build failed:

@@ -0,0 +1,45 @@
use proc_macro2::TokenStream;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Not super necessary but it would be nice to either have a module-level doc here stating the purpose of these FQ*** types or doc comments on the individual structs (or both haha).

It's pretty simple/self-explanatory so definitely not required, but I like to make sure the barrier to contributing is low for newcomers :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the doc too.

@MrGVSV MrGVSV added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 3, 2022
use proc_macro2::TokenStream;
use quote::{quote, ToTokens};

// This module contains unit structs that should be used inside `quote!` and `spanned_quote!` using the variable interpolation syntax in place of their equivalent structs and traits present in `std`.
Copy link
Member

Choose a reason for hiding this comment

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

Normally module doc comments are made using //! at the very top of the file (above imports). If we include these, could we move them to the top to be consistent with that format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved them to the top and used //! for the docs.
Initially, I had thought that using //! would make this module visible in the user-facing documentation of Bevy but later realized that won't happen.

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 4, 2022
@alice-i-cecile
Copy link
Member

Blocking on moving the module comment, then I'll merge.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 4, 2022
@cart
Copy link
Member

cart commented Dec 5, 2022

bors r+

bors bot pushed a commit that referenced this pull request Dec 5, 2022
# Objective

- Fixes #3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
@bors bors bot changed the title Make proc macros hygienic in bevy_reflect_derive [Merged by Bors] - Make proc macros hygienic in bevy_reflect_derive Dec 5, 2022
@bors bors bot closed this Dec 5, 2022
@james7132 james7132 added this to the 0.10 milestone Dec 6, 2022
ickshonpe pushed a commit to ickshonpe/bevy that referenced this pull request Dec 6, 2022
# Objective

- Fixes bevyengine#3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
@elbertronnie elbertronnie deleted the fix-unhygienic-macros branch January 17, 2023 12:01
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

- Fixes bevyengine#3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Fixes bevyengine#3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Reflect proc-macro causes compile errors when using custom Result type
5 participants