Skip to content

sp-api: Propagate deprecation information to metadata from impl_runtime_apis! blocks #6394

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pkhry
Copy link
Contributor

@pkhry pkhry commented Nov 6, 2024

Description

Now its possible to deprecate an ApiTrait or a Method inside impl_runtime_apis! blocks.
If a deprecation attribute was present inside decl_runtime_apis! block then it is overriden with information from impl_runtime_apis! block.

ie:

decl_runtime_apis! {
           trait .... {
             #[deprecated = "test"] 
             fn example() {}
           }
}

impl_runtime_apis! {
          impl .... {
             #[deprecated = "overriden"] 
             fn example() {}
           }
}

The resulting deprecation string inside metadata for method example will be "overriden"

Review Notes

Adds additional argument to <trait_>::runtime_metadata to pass the deprecation information from the implementations.

@pkhry pkhry added R0-no-crate-publish-required The change does not require any crates to be re-published. T4-runtime_API This PR/Issue is related to runtime APIs. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Nov 6, 2024
@pkhry
Copy link
Contributor Author

pkhry commented Nov 6, 2024

/cmd fmt

@pkhry pkhry marked this pull request as ready for review November 6, 2024 15:11
@pkhry pkhry requested a review from a team November 6, 2024 15:11
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Command "fmt" has started 🚀 See logs here

Copy link
Contributor

github-actions bot commented Nov 6, 2024

Command "fmt" has finished ✅ See logs here

@bkchr
Copy link
Member

bkchr commented Nov 6, 2024

If a deprecation attribute was present inside decl_runtime_apis! block then it is overriden with information from impl_runtime_apis! block.

Why should I override the deprecation notice in the impl block?

@pkhry
Copy link
Contributor Author

pkhry commented Nov 6, 2024

If a deprecation attribute was present inside decl_runtime_apis! block then it is overriden with information from impl_runtime_apis! block.

Why should I override the deprecation notice in the impl block?

I guess I need to reword. In case of deprecation attribute being present on the same member in both decl_runtime_apis and impl_runtime_apis metadata will reflect deprecation info in the deprecation attribute used in the impl_runtime_api block

@bkchr
Copy link
Member

bkchr commented Nov 6, 2024

Yeah, I mean I got this. My question still holds, why should I put a deprecate message into the impl block?

IMO we should do what rust is doing and return an error: rust-lang/rust#78626

@jsdw
Copy link
Contributor

jsdw commented Nov 19, 2024

Yeah, I mean I got this. My question still holds, why should I put a deprecate message into the impl block?

My 2c:

In "normal" rust, it makes sense that deprecation attributes only exist on trait declarations: I define a trait and then want implementors of my trait to know if I am planning to remove some function of it, ie

#[deprecated]        'Warning: deprecated'
trait defintion ---> trait implementors

In this runtime stuff, we may want to inform runtime developers that some runtime API trait function is deprecated (via usual rust mechanisms), but also we want to inform users calling into a runtime that some part of the runtime interface is deprecated (via metadata), so we have two targets now ie

#[deprecated]         #[deprecated] + 'Warning'     'Warning'
trait definition ---> trait implementation      ---> Runtime API users 

And so, if I am a runtime developer, I may deprecate some Runtime API_implementations_ to signal that the runtime is moving to using some different Runtime APIs, or planning to jump to a newer version of some trait and stop supporting the older one, or just planning to remove some implementations entirely.

It also makes sense to me that #[deprecated] attrs on the definitions are passed to users to signal that those methods currently implemented by some runtime are also expected to go away.

@bkchr
Copy link
Member

bkchr commented Nov 25, 2024

Yeah good argument. Maybe we should then differ between impl_deprecation and decl_deprecation?

@jsdw
Copy link
Contributor

jsdw commented Nov 26, 2024

Yeah good argument. Maybe we should then differ between impl_deprecation and decl_deprecation?

I haven't though about this long enough, but offhand I can't think of a time when it would matter to users where the deprecation came from at the moment; only that the method was deprecated and then the reason for it, if provided.

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12233615300
Failed job name: fmt

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

I agree that impl_deprecation is not really overriding decl_deprecation. But in practice I expect the impl_deprecation to be more precise than the decl_deprecation for runtime user so I think it is ok.

I have a concern in the review below, but apart from this it looks good to me

let name = &method.sig.ident;
dbg!(name);
let deprecation = crate::utils::get_deprecation(&crate_, &method.attrs)?;
deprecations.push(quote! {(stringify!(#name), #deprecation)});
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot, method names are unique or they are prefixed with the trait name in the runtime api?
If they are not unique and prefixed by the trait name we should do same here. Otherwise if 2 runtime API have a method with the same name then deprecation information will propagate to both.

You could even strongly type it to differentiate between trait deprecation and method deprecation, but it can be ok as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. R0-no-crate-publish-required The change does not require any crates to be re-published. T4-runtime_API This PR/Issue is related to runtime APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants