Deprecation info support in RuntimeMetadataIR #4851

pkhry · 2024-06-20T20:15:43Z

Description:

Adds DeprecationStatus enum to sp_metadata_ir.
Adds deprecation_info field to
- RuntimeApiMetadataIR
- RuntimeApiMethodMetadataIR
- StorageEntryMetadataIR
- PalletConstantMetadataIR

There's also some test updates to make sure that deprecation attributes are getting propagated to the relevant structs.

see: #4098, Solution: A

TODO(to be done in separate MR):

Add a Btree<T::Type, DeprecationStatus> to PalletMetadataIR to store deprecation info on Pallet events and errors and have the ability to query deprecation info by T::Type.
updated MetadataIr would like like this:

/// The intermediate representation for a pallet metadata.
#[derive(Clone, PartialEq, Eq, Encode, Debug)]
pub struct PalletMetadataIR<T: Form = MetaForm> {
	/// Pallet name.
	pub name: T::String,
	/// Pallet storage metadata.
	pub storage: Option<PalletStorageMetadataIR<T>>,
        ...
	/// Deprecation info
	pub deprecation_info: DeprecationStatus<T>,
}

Note that it's not really possible to put deprecation metadata on Events and Errors themselves as they are required to have From<MetaType> impls.

PS:

i used cargo +nightly fmt --all and cargo +nightly-2024-04-10 fmt --all and still getting line changes due to formatting, what's the correct incantation?

lexnv · 2024-06-26T10:32:56Z

substrate/frame/support/procedural/src/construct_runtime/expand/metadata.rs

@@ -58,7 +58,6 @@ pub fn expand_runtime_metadata(
 					#attr
 				}
 			});
-


nit: Unrelated change

lexnv · 2024-06-26T10:34:38Z

substrate/frame/support/procedural/src/construct_runtime/parse.rs

@@ -97,26 +97,29 @@ impl Parse for RuntimeDeclaration {
 		let pallets_token = pallets.token;

 		match convert_pallets(pallets.content.inner.into_iter().collect())? {
-			PalletsConversion::Implicit(pallets) =>


All changes of this file look unrelated. I think you'll need to configure the rustfmt to comply with substrate

didn't notice that it was relying on nightly settings, corrected

weirdly enough even using the settings from rustfmt config it still changes some of the formatting, are some files formatted by hand?

lexnv · 2024-06-26T10:47:23Z

substrate/primitives/api/proc-macro/src/utils.rs

+		// assert_eq!(
+		// 	get_deprecation(&[simple_path]).unwrap(),


dq: Should we uncomment thsese?

lexnv · 2024-06-26T10:48:32Z

substrate/primitives/api/proc-macro/src/utils.rs

+		let simple_path: Attribute = parse_quote!(#[deprecated = #FIRST]);
+		let meta_list: Attribute = parse_quote!(#[deprecated(note = #FIRST)]);
+		let meta_list_with_since: Attribute =
+			parse_quote!(#[deprecated(note = #FIRST, since = #SECOND)]);


Would be interesting to see how this behaves with extra parameters

it will just ignore them

Do you think we can turn that into a compile error? And specify inside the message the forms we support?

it's a compiler error already, that's one of the reason why i decided to use rust's deprecated. But the parsing now doesnt fail if attribute is malformed(not sure how useful it is tho)

lexnv · 2024-06-26T10:53:24Z

substrate/frame/support/test/tests/instance.rs

@@ -441,6 +441,7 @@ fn expected_metadata() -> PalletStorageMetadataIR {
 				ty: StorageEntryTypeIR::Plain(scale_info::meta_type::<u32>()),
 				default: vec![0, 0, 0, 0],
 				docs: vec![],
+				deprecation_info: sp_metadata_ir::DeprecationStatus::NotDeprecated,


I would add a few tests similar to these (with/without note=.. since=..):

check deprecation info for storage entries

check deprecation info for runtime APIs for individual methods and for the trait itself

check deprecation info for constants

This way we'll also have a good example and see the API in action from the user's perspective

added more testcases for StorageEntryMetadataIr and constants metadata

lexnv · 2024-06-26T10:55:54Z

substrate/frame/support/src/tests/mod.rs

 			}

 			Ok(().into())
 		}
 	}

 	#[pallet::storage]
+	#[deprecated]


Nice test here! Bikeshedding on the attribute naming; how would pallet::depreacated sound like? Would it bring more clarity to the user that this is related entirely to the metadata collection?

hmm, but that would introduce the need to use two separate deprecation attributes, one for metadata and one for rustc deprecation warnings. Or that's totally ok?

I think that would make it a bit clear that the deprecation tag is collected into the metadata. Then, the pallet::deprecated could insert under the hood the deprecated attribute to the type, and propagate DeprecationInfo to the metadata. Up to you in the end, I have no strong pref here :D

what about sp-api-proc-macro stuff? should it also be pallet::deprecated there or for example api::deprecated?

I guess we can go with the default deprecation tag for now, I would focus on the metadata collection first, we can come back and change it later if needed

lexnv · 2024-06-26T11:04:19Z

Nice work here @pkhry! Certainly off to a good start! 👍

Left a few thoughts after the initial review, mainly:

would be good to keep the changes related to this PR only (ie adjust the rustfmt)
a few more tests to illustrate how this feels in practice for the user experience (also to validate the approach for all deprecation included calls / storage etc)
would be interesting also to include a negative test that fails to compile because pallet::deprecated includes unsupported fields (ie fields that are not empty; "note" or "since")
make the CI happy (to double check the tests for now, everything should pass since changes are made in isolation)

From the CI step: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6513912:

error: unused import: `proc_macro::TokenStream`
  --> substrate/frame/support/procedural/src/pallet/parse/config.rs:20:5
   |
20 | use proc_macro::TokenStream;
   |     ^^^^^^^^^^^^^^^^^^^^^^^

After that we can have a look at exposing deprecation info for events / errors (maybe in a separate PR), your initial approach of using a BTreeMap sounds good offhand, but I would consolidate this first. Let me know if you have any questions here 🙏

paritytech-cicd-pr · 2024-06-27T23:46:23Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6581715

lexnv reviewed Jun 26, 2024

View reviewed changes

lexnv assigned lexnv and unassigned lexnv Jun 27, 2024

lexnv added R0-silent Changes should not be mentioned in any release notes I5-enhancement An additional feature request. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Jun 27, 2024

pkhry added 11 commits June 28, 2024 01:04

WIP: storage macro errors out

4188611

update tests for methods and storage

f9312f9

WIP: no events and errors

64161bd

use +nightly fmt

4d977a1

update deprecation attribute parsing

6147dc1

add some more test-cases for deprecation

b815256

add prdoc and license

a7f577f

prdoc typo

f69854c

fix clippy lints

6dee2a6

cargo fmt

7fc2026

cargo fmt

c8d2285

pkhry force-pushed the pkhry/deprecation_attr_frame_support branch from f0008e0 to c8d2285 Compare June 27, 2024 23:06

pkhry added 3 commits June 28, 2024 01:33

fixup typo and test errors

6285ebb

cargo fmt

c04eb6f

fix clippy lints

5abb44e

more clippy fixes

21f032f

pkhry requested a review from lexnv June 28, 2024 00:01

pkhry marked this pull request as ready for review June 28, 2024 00:01

pkhry requested a review from a team as a code owner June 28, 2024 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecation info support in RuntimeMetadataIR #4851

Deprecation info support in RuntimeMetadataIR #4851

pkhry commented Jun 20, 2024 •

edited

Loading

lexnv Jun 26, 2024

pkhry Jun 28, 2024

lexnv Jun 26, 2024

pkhry Jun 26, 2024

pkhry Jun 26, 2024

lexnv Jun 26, 2024

pkhry Jun 26, 2024

lexnv Jun 26, 2024

pkhry Jun 26, 2024

lexnv Jul 2, 2024

pkhry Jul 3, 2024

lexnv Jun 26, 2024

pkhry Jun 26, 2024

lexnv Jun 26, 2024

pkhry Jun 26, 2024

lexnv Jul 2, 2024

pkhry Jul 3, 2024

lexnv Jul 3, 2024

lexnv commented Jun 26, 2024

paritytech-cicd-pr commented Jun 27, 2024

@@ @@ -58,7 +58,6 @@ pub fn expand_runtime_metadata( @@
               					#attr
               				}
               			});

Deprecation info support in RuntimeMetadataIR #4851

Are you sure you want to change the base?

Deprecation info support in RuntimeMetadataIR #4851

Conversation

pkhry commented Jun 20, 2024 • edited Loading

Description:

TODO(to be done in separate MR):

PS:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv commented Jun 26, 2024

paritytech-cicd-pr commented Jun 27, 2024

pkhry commented Jun 20, 2024 •

edited

Loading