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

#[derive(Serialize) and #[derive(Deserialize)] on deprecated types emit deprecation warnings #2195

Open
nsunderland1 opened this issue Mar 29, 2022 · 5 comments · May be fixed by #2879
Open

Comments

@nsunderland1
Copy link

Given this code (playground link):

#[deprecated]
#[derive(Default, serde::Serialize)]
struct WithSerialize;

#[deprecated]
#[derive(Default, serde::Deserialize)]
struct WithDeserialize;

#[deprecated]
#[derive(Default)]
struct WithoutSerde;

fn main() {
}

I expect not to get deprecation warnings. Instead, I get the following:

warning: use of deprecated struct `WithSerialize`
 [--> src/main.rs:3:8
](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=22d2042e09ce11dddfcbac0464dd4fe3#)  |
3 | struct WithSerialize;
  |        ^^^^^^^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default

warning: use of deprecated struct `WithDeserialize`
 [--> src/main.rs:7:8
](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=22d2042e09ce11dddfcbac0464dd4fe3#)  |
7 | struct WithDeserialize;
  |        ^^^^^^^^^^^^^^^

warning: use of deprecated unit struct `WithDeserialize`
 [--> src/main.rs:7:8
](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=22d2042e09ce11dddfcbac0464dd4fe3#)  |
7 | struct WithDeserialize;
  |        ^^^^^^^^^^^^^^^

Notice that the derives for Default don't trigger any warnings (this seems to have been fixed some time ago).

Not aware of any workaround (other than undeprecating), and this leads to a bunch of warnings for things that are deprecated but that we still want to be able to support serialization for.

@LovingMelody
Copy link

Not exactly a perfect workaround however you can disable the lint if the warnings are an issue by moving the deprecated items to another module (playground link).
This is not perfect as it will disable all lints for deprecated usages in that module.

@eric-seppanen
Copy link

I am having trouble trying to use a deprecated struct in a (non-deprecated) enum.

For example, this (playground) emits warnings no matter how hard I try to suppress them. If I don't derive serde traits on the enum, the problem goes away.

mod current_structs {
    use serde::{Deserialize, Serialize};

    #[derive(Serialize, Deserialize)]
    pub struct IsCurrent;
}

mod deprecated_structs {
    #![allow(deprecated)]

    use serde::{Deserialize, Serialize};

    #[deprecated]
    #[derive(Serialize, Deserialize)]
    pub struct IsDeprecated;
}

use serde::{Deserialize, Serialize};

use current_structs::*;

#[allow(deprecated)]
use deprecated_structs::*;

#[allow(deprecated)]
#[derive(Serialize, Deserialize)]
enum Container {
    #[allow(deprecated)]
    A(#[allow(deprecated)] IsDeprecated),
    B(IsCurrent),
}

rcrisanti pushed a commit to rcrisanti/serde that referenced this issue Jan 8, 2025
Allow deprecated in the `Serialize`/`Deserialize`
derive implementations. This allows you to
deprecate structs, enums, struct fields, or enum
variants and not get compiler warnings/errors
about use of deprecated thing.

Resolves serde-rs#2195
rcrisanti added a commit to rcrisanti/serde that referenced this issue Jan 8, 2025
Allow deprecated in the `Serialize`/`Deserialize`
derive implementations. This allows you to
deprecate structs, enums, struct fields, or enum
variants and not get compiler warnings/errors
about use of deprecated thing.

Resolves serde-rs#2195
@rcrisanti rcrisanti linked a pull request Jan 8, 2025 that will close this issue
@Elias-Graf
Copy link

@eric-seppanen Simply put: you're missing a bang / exclamation point / !.

If you go to the playground in the original issue and select "Tools" > "Expand Macros", you will find, among other code (and scoping magic aside), this:

        #[allow(unused_extern_crates, clippy :: useless_attribute)]
        extern crate serde as _serde;
        #[automatically_derived]
        impl _serde::Serialize for WithSerialize {
            fn serialize<__S>(&self, __serializer: __S)
                -> _serde::__private::Result<__S::Ok, __S::Error> where
                __S: _serde::Serializer {
                _serde::Serializer::serialize_unit_struct(__serializer,
                    "WithSerialize")
            }
        }

Here you can clearly see that the derive macro creates an impl for a deprecated struct, very much outside of any #[allow(derecated)] you specified on the struct item.

That's why you need an inner attribute (#![...]), which applies to the containing scope. The problem with that is, now you're basically ignoring all deprecations in the whole module. That's where we try to limit the "damage" as much as possible, by putting the deprecated serde items into their own module.

So the problem is not that you deprecated soem structs (which you put inside a module with an allow), but that you are again trying, through serde to use / instantiate that item. So all your serde items need to be inside that module, if they contain a deprecated item.

Your module should probably be this:

Playground

use serde::{Deserialize, Serialize};

#[derive(Serialize, Deserialize)]
pub struct IsCurrent;

mod deprecated_structs {
    #![allow(deprecated)]
    
    use super::*;

    #[deprecated]
    #[derive(Serialize, Deserialize)]
    pub struct IsDeprecated;

    #[derive(Serialize, Deserialize)]
    pub enum Container {
        A(IsDeprecated),
        B(IsCurrent),
    }
}
pub use deprecated_structs::*;

But I think this still leaves a lot of potential to accidentally allow other deprecations, or ignore new ones that should not be allowed. There really should be some way to tell serde, to allow certain specific deprecated items.

@eric-seppanen
Copy link

@Elias-Graf I don't think "you're missing a !" really addresses the question.

Yes, if I move the affected code into a module with the lint allowed, it will be suppressed (this is what I did it the real code that inspired my comment). But should we encourage that? Or should we try to allow more precisely targeted #[allow] attributes?

Rewriting my example to move the enum into one of the other modules isn't a general fix. In my example the modules were representing code that comes from other crates, so moving the enum isn't practical. If I'm trying to use a foreign deprecated item inside the enum, there seems to be no way to do that other than adding #![allow] to the module containing the enum.

I don't think forgiving lints at module scope is a practice that should be encouraged. The technique I'd like to encourage is applying the attribute at the most focused level possible. This is possible almost everywhere else, but not here, as far as I can tell.

The reason I think this is an interesting discussion at the serde level is because serde's derive proc-macro can see the attribute attached to the enum variant. If it wanted to, that macro could copy the #[allow(deprecated)] into the generated code (in this example, into Visitor::visit_enum). This may be too hand-wavey: it's probably really complicated to figure out which attributes to copy, or where to put them, or it may be too much work, or unwise for other reasons (e.g. it's unclear how many other lints/attributes this logic should apply to).

@Elias-Graf
Copy link

Elias-Graf commented Jan 17, 2025

@eric-seppanen I'm currently with the opinion of the author of this pr: #2879 (comment), which would fix the issue at hand.
The generated code should not be concerned with such warnings at all. The problem is not that the item can be serialized / deserialized, but arises, when a deprecated property / variant is used, of which the warning should still apply anyway.

And sorry if I did not address your post correctly. I do notice it was too broadly written. I hoped it would also be of use to other people finding this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants