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

Fix appending attributes instead of inserting them #3

Merged
merged 3 commits into from
Nov 19, 2022

Conversation

Luctins
Copy link
Contributor

@Luctins Luctins commented Nov 17, 2022

This fixes a warning (legacy_derive_helpers) about outer struct attributes being used before being declared. This warning is slated to become a hard error in the future.

Trying to use outer struct attributes with strikethrough work, but seems like it may fail in the future.

Trying to compile a minimal example shows:

warning: derive helper attribute is used before it is introduced
 --> src/main.rs:8:11
  |
4 |     #[strikethrough[derive(Debug, Serialize, Deserialize)]]
  |                                   --------- the attribute is introduced here
...
8 |         #[serde(transparent)]
  |           ^^^^^
  |
  = note: `#[warn(legacy_derive_helpers)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #79202 <https://github.com/rust-lang/rust/issues/79202>

Indicating probably that it adds the #[derive(Attr)] after the outer attributes.

Tracking issue: rust-lang/rust#79202

This fixes a warning (`legacy_derive_helpers`) about outer
struct attributes being used before being declared. This warning is
slated to become a hard error in the future.
@Luctins
Copy link
Contributor Author

Luctins commented Nov 17, 2022

Sorry for the force pushes, forgot to use the correct key+email+username combo

@Luctins
Copy link
Contributor Author

Luctins commented Nov 17, 2022

mwe:

src/main.rs

use serde::{Serialize, Deserialize};

structstruck::strike!{
    #[strikethrough[derive(Serialize, Deserialize)]]
    struct Outer {
        field:

        #[serde(transparent)]
        struct Transparent {
            inner: struct Inner {
                name: String,
            }
        }
    }
}

fn main() {
    println!("Hello, world!");
}

@jcaesar
Copy link
Owner

jcaesar commented Nov 18, 2022

Thanks for the PR. Can I ask you to make sure that it passes cargo test and cargo fmt -- --check? (If not, no problem, I can take care of that.)

@Luctins
Copy link
Contributor Author

Luctins commented Nov 18, 2022

no problem, will do

@Luctins
Copy link
Contributor Author

Luctins commented Nov 18, 2022

Ive messed around for a bit trying to make this work, and this seems to be a ok compromise.

It inserts non derive attributes after existing ones and inserts derive attributes before the existing attributes.

I'm not sure this is the most optimal way to do it though, as it creates 2 vectors each time.

@jcaesar jcaesar merged commit 90d9231 into jcaesar:master Nov 19, 2022
@jcaesar
Copy link
Owner

jcaesar commented Nov 19, 2022

This crate already allocates all over the place, no need to worry about a few more allocations. I do worry about the order though. Anyway.

Would it help you if I did a release with this quickly? (e.g. because you want to publish a crate that depends on this one to crates.io)

@Luctins
Copy link
Contributor Author

Luctins commented Nov 19, 2022

Would it help you if I did a release with this quickly? (e.g. because you want to publish a crate that depends on this one to crates.io)

yes, please!
I currently use this crate for a project that is due to ship soon. It would be best to have all the dependencies on crates.io if possible.

Thanks a lot.

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

Successfully merging this pull request may close these issues.

2 participants