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

DeprecationInfo Remove deprecated attribute from generated code. #6312

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions prdoc/pr_6312.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: DeprecationInfo Remove deprecated attribute from generated code.

doc:
- audience:
- Runtime dev
- Runtime user
description: |
filters out `deprecated` attribute from code generated by macros

crates:
- name: frame-support-procedural
bump: none
- name: frame-support
bump: none
- name: sp-api-proc-macro
bump: none
- name: sp-api
bump: none
5 changes: 5 additions & 0 deletions substrate/frame/support/procedural/src/deprecation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ fn deprecation_msg_formatter(msg: &str) -> String {
)
}

/// removes deprecation attribute if its present.
pub fn remove_deprecation_attribute(attrs: &mut Vec<syn::Attribute>) {
attrs.retain(|attr| !attr.path().is_ident("deprecated"))
}

fn parse_deprecated_meta(crate_: &TokenStream, attr: &syn::Attribute) -> Result<TokenStream> {
match &attr.meta {
Meta::List(meta_list) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// limitations under the License.

use crate::{
deprecation::remove_deprecation_attribute,
pallet::{
expand::warnings::{weight_constant_warning, weight_witness_warning},
parse::{call::CallWeightDef, helper::CallReturnType},
Expand Down Expand Up @@ -209,6 +210,8 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
let return_type =
&call.methods.get(i).expect("def should be consistent with item").return_type;

remove_deprecation_attribute(&mut method.attrs);
Copy link
Contributor

Choose a reason for hiding this comment

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

So this removes the deprecated attribute from the call?
I think it is not ideal, because we lose the deprecation of the call itself in rust code.

I think the ideal is to have the deprecation on the call function, also the deprecation on the call variant.
But then all usage of this call inside the macro should be used with #[allow(deprecated)] on top of the instruction. (We should be careful not to have this #[allow(deprecated)] for logic written by user).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted this change

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking a more fine-grained implementation.

Like the dispatch_bypass_filter would be implemented with #[allow(deprecated)] just above the function call

<#pallet_ident<#type_use_gen>>::#fn_name(origin, #( #args_name, )* )

Same for all other types: when the macro use them we would write: #[allow(deprecated)].

But then we have to be careful not to write #[allow(deprecated)] on top of blocks which include code written by user. So that user is still notified when they use deprecated elements in their own code.

If it is too difficult, maybe we can merge this PR, without the revert then.

Copy link
Contributor Author

@pkhry pkhry Nov 5, 2024

Choose a reason for hiding this comment

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

I've tried implementing it this morning, and i can't seem to understand how to propagate #[allow(deprecated)] from defs inside #[pallet] to expansion of construct_runtime!. Ie if we deprecate an event or an error we dont have any attributes to include maybe_allow_attrs inside code generated by construct_runtime!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree here with @gui1117. Just removing the deprecation attribute, because it adds more noise is not the right way. The attribute is doing exactly for what it is designed for.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tried implementing it this morning, and i can't seem to understand how to propagate #[allow(deprecated)] from defs inside #[pallet] to expansion of construct_runtime!. Ie if we deprecate an event or an error we dont have any attributes to include maybe_allow_attrs inside code generated by construct_runtime!

about construct_runtime! if the whole call or event or pallet is deprecated, then I think the writer of the construct_runtime! should add #[allow(deprecated)] on top of the pallet variant:

construct_runtime!{
struct Runtime {
  #[allow(deprecated)]
  MyPalletWithDeprecatedCall: my_pallet; // my pallet is using call implicitly here.
}
}

Otherwise it is fair to actually warn the runtime writer than one of the pallet he uses has actual deprecated elements.

Note that this should only happen when deprecating whole types like Event or Call or Pallet, not when deprecating a single call inside the pallet Call.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the purpose of deprecating these types would be. I mean I would understand if the entire pallet would be deprecated.


let (ok_type, err_type) = match return_type {
CallReturnType::DispatchResult => (
quote::quote!(()),
Expand Down Expand Up @@ -272,6 +275,11 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
Err(e) => return e.into_compile_error(),
};

match def.call.as_mut() {
pkhry marked this conversation as resolved.
Show resolved Hide resolved
Some(call) => remove_deprecation_attribute(&mut call.attrs),
None => (),
};

quote::quote_spanned!(span =>
#[doc(hidden)]
mod warnings {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::pallet::Def;
use crate::{
deprecation::remove_deprecation_attribute,
pallet::{parse::helper::MutItemAttrs, Def},
};
use proc_macro2::TokenStream;
use quote::quote;
use syn::{parse_quote, Item};
Expand Down Expand Up @@ -47,6 +50,10 @@ Consequently, a runtime that wants to include this pallet must implement this tr
]
),
);
config_item.attrs.retain(|attr| !attr.path().is_ident("deprecated"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind this. Thinking back I think it was overkill to allow deprecating the config in the first place.

I wouldn't mind also just letting the flag there, and have deprecated warning everywhere.

for item in config_item.items.iter_mut() {
item.mut_item_attrs().map(remove_deprecation_attribute);
}

// we only emit `DefaultConfig` if there are trait items, so an empty `DefaultConfig` is
// impossible consequently.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::pallet::Def;
use crate::{deprecation::remove_deprecation_attribute, pallet::Def};

struct ConstDef {
/// Name of the associated type.
Expand Down Expand Up @@ -45,7 +45,7 @@ pub fn expand_constants(def: &mut Def) -> proc_macro2::TokenStream {
let completed_where_clause = super::merge_where_clauses(&where_clauses);

let mut config_consts = vec![];
for const_ in def.config.consts_metadata.iter() {
for const_ in def.config.consts_metadata.iter_mut() {
pkhry marked this conversation as resolved.
Show resolved Hide resolved
let ident = &const_.ident;
let const_type = &const_.type_;
let deprecation_info = match crate::deprecation::get_deprecation(
Expand All @@ -55,6 +55,8 @@ pub fn expand_constants(def: &mut Def) -> proc_macro2::TokenStream {
Ok(deprecation) => deprecation,
Err(e) => return e.into_compile_error(),
};
remove_deprecation_attribute(&mut const_.attrs);

config_consts.push(ConstDef {
ident: const_.ident.clone(),
type_: const_.type_.clone(),
Expand All @@ -70,7 +72,7 @@ pub fn expand_constants(def: &mut Def) -> proc_macro2::TokenStream {
}

let mut extra_consts = vec![];
for const_ in def.extra_constants.iter().flat_map(|d| &d.extra_constants) {
for const_ in def.extra_constants.iter_mut().flat_map(|d| &mut d.extra_constants) {
pkhry marked this conversation as resolved.
Show resolved Hide resolved
let ident = &const_.ident;
let deprecation_info = match crate::deprecation::get_deprecation(
&quote::quote! { #frame_support },
Expand All @@ -79,6 +81,7 @@ pub fn expand_constants(def: &mut Def) -> proc_macro2::TokenStream {
Ok(deprecation) => deprecation,
Err(e) => return e.into_compile_error(),
};
remove_deprecation_attribute(&mut const_.attrs);

extra_consts.push(ConstDef {
ident: const_.ident.clone(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// limitations under the License.

use crate::{
deprecation::remove_deprecation_attribute,
pallet::{
parse::error::{VariantDef, VariantField},
Def,
Expand Down Expand Up @@ -99,7 +100,6 @@ pub fn expand_error(def: &mut Def) -> proc_macro2::TokenStream {
unreachable!("Checked by error parser")
}
};

error_item.variants.insert(0, phantom_variant);

let capture_docs = if cfg!(feature = "no-metadata-docs") { "never" } else { "always" };
Expand All @@ -117,6 +117,12 @@ pub fn expand_error(def: &mut Def) -> proc_macro2::TokenStream {
Err(e) => return e.into_compile_error(),
};

error_item
.variants
.iter_mut()
.for_each(|variant| remove_deprecation_attribute(&mut variant.attrs));
remove_deprecation_attribute(&mut error_item.attrs);

// derive TypeInfo for error metadata
error_item.attrs.push(syn::parse_quote! {
#[derive(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// limitations under the License.

use crate::{
deprecation::remove_deprecation_attribute,
pallet::{parse::event::PalletEventDepositAttr, Def},
COUNTER,
};
Expand Down Expand Up @@ -108,6 +109,11 @@ pub fn expand_event(def: &mut Def) -> proc_macro2::TokenStream {
Err(e) => return e.into_compile_error(),
};

event_item
.variants
.iter_mut()
.for_each(|variant| remove_deprecation_attribute(&mut variant.attrs));

if get_doc_literals(&event_item.attrs).is_empty() {
event_item
.attrs
Expand All @@ -134,6 +140,8 @@ pub fn expand_event(def: &mut Def) -> proc_macro2::TokenStream {
#[scale_info(skip_type_params(#event_use_gen), capture_docs = #capture_docs)]
));

remove_deprecation_attribute(&mut event_item.attrs);

let deposit_event = if let Some(deposit_event) = &event.deposit_event {
let event_use_gen = &event.gen_kind.type_use_gen(event.attr_span);
let trait_use_gen = &def.trait_use_generics(event.attr_span);
Expand Down
17 changes: 10 additions & 7 deletions substrate/frame/support/procedural/src/pallet/expand/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ mod type_value;
mod validate_unsigned;
mod warnings;

use crate::pallet::Def;
use crate::{deprecation::remove_deprecation_attribute, pallet::Def};
use quote::ToTokens;

use super::parse::helper::MutItemAttrs;

/// Merge where clause together, `where` token span is taken from the first not none one.
pub fn merge_where_clauses(clauses: &[&Option<syn::WhereClause>]) -> Option<syn::WhereClause> {
let mut clauses = clauses.iter().filter_map(|f| f.as_ref());
Expand Down Expand Up @@ -121,12 +123,13 @@ storage item. Otherwise, all storage items are listed among [*Type Definitions*]
#composites
);

def.item
.content
.as_mut()
.expect("This is checked by parsing")
.1
.push(syn::Item::Verbatim(new_items));
let item = &mut def.item.content.as_mut().expect("This is checked by parsing").1;
item.push(syn::Item::Verbatim(new_items));
for i in item {
if let Some(attrs) = i.mut_item_attrs() {
remove_deprecation_attribute(attrs);
}
}

def.item.into_token_stream()
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::pallet::{expand::merge_where_clauses, Def};
use crate::{
deprecation::remove_deprecation_attribute,
pallet::{expand::merge_where_clauses, Def},
};
use frame_support_procedural_tools::get_doc_literals;

///
Expand All @@ -35,6 +38,13 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
let type_decl_gen = &def.type_decl_generics(def.pallet_struct.attr_span);
let pallet_ident = &def.pallet_struct.pallet;
let config_where_clause = &def.config.where_clause;
let deprecation_status =
match crate::deprecation::get_deprecation(&quote::quote! {#frame_support}, &def.item.attrs)
{
Ok(deprecation) => deprecation,
Err(e) => return e.into_compile_error(),
};
remove_deprecation_attribute(&mut def.item.attrs);

let mut storages_where_clauses = vec![&def.config.where_clause];
storages_where_clauses.extend(def.storages.iter().map(|storage| &storage.where_clause));
Expand Down Expand Up @@ -185,12 +195,7 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
}
}
];
let deprecation_status =
match crate::deprecation::get_deprecation(&quote::quote! {#frame_support}, &def.item.attrs)
{
Ok(deprecation) => deprecation,
Err(e) => return e.into_compile_error(),
};

quote::quote_spanned!(def.pallet_struct.attr_span =>
#pallet_error_metadata

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

use crate::{
counter_prefix,
deprecation::remove_deprecation_attribute,
pallet::{
parse::{
helper::two128_str,
Expand Down Expand Up @@ -425,6 +426,7 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
Ok(deprecation) => deprecation,
Err(e) => return e.into_compile_error(),
};

entries_builder.push(quote::quote_spanned!(storage.attr_span =>
#(#cfg_attrs)*
(|entries: &mut #frame_support::__private::Vec<_>| {
Expand All @@ -440,6 +442,9 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
})
))
}
for storage in &mut def.storages.iter_mut() {
remove_deprecation_attribute(&mut storage.attrs);
}

let getters = def.storages.iter().map(|storage| {
if let Some(getter) = &storage.getter {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ fn construct_runtime_final_expansion(
&unchecked_extrinsic,
&system_pallet.path,
);
let outer_config = expand::expand_outer_config(&name, &pallets, &scrate);
let outer_config: TokenStream2 = expand::expand_outer_config(&name, &pallets, &scrate);
let inherent =
expand::expand_outer_inherent(&name, &block, &unchecked_extrinsic, &pallets, &scrate);
let validate_unsigned = expand::expand_outer_validate_unsigned(&name, &pallets, &scrate);
Expand Down
14 changes: 13 additions & 1 deletion substrate/frame/support/test/tests/pallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#![allow(useless_deprecated, deprecated, clippy::deprecated_semver)]
#![allow(useless_deprecated, clippy::deprecated_semver)]
Copy link
Contributor

Choose a reason for hiding this comment

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

why is deprecated_semver needed by the way?
Also people will have to bound useless_deprecated, or we can remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there were tests during the initial review with non-semver strings in the deprecation attribute. removed all of allows above


use std::collections::BTreeMap;

Expand Down Expand Up @@ -121,6 +121,7 @@ impl SomeAssociation2 for u64 {
// Comments should not be included in the pallet documentation
#[pallet_doc("../example-pallet-doc.md")]
#[doc = include_str!("../example-readme.md")]
#[deprecated = "example"]
pub mod pallet {
use super::*;
use frame_support::pallet_prelude::*;
Expand All @@ -138,6 +139,7 @@ pub mod pallet {
{
/// Some comment
/// Some comment
#[deprecated = "test 2"]
#[pallet::constant]
type MyGetParam: Get<u32>;

Expand Down Expand Up @@ -178,6 +180,7 @@ pub mod pallet {
}

#[pallet::pallet]
#[deprecated = "example"]
#[pallet::storage_version(STORAGE_VERSION)]
pub struct Pallet<T>(_);

Expand Down Expand Up @@ -2555,6 +2558,15 @@ fn pallet_metadata() {
meta.deprecation_info
)
}
{
// Example pallet constant is deprecated
let meta = &example.constants[0];
dbg!(meta);
pkhry marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(
DeprecationStatusIR::Deprecated { note: "test 2", since: None },
meta.deprecation_info
)
}
{
// Example pallet errors are partially and fully deprecated
let meta = &example.error.unwrap();
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/support/test/tests/runtime_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#![allow(useless_deprecated, deprecated, clippy::deprecated_semver)]
#![allow(useless_deprecated, clippy::deprecated_semver)]

use frame_support::{derive_impl, traits::ConstU32};
use scale_info::{form::MetaForm, meta_type};
Expand Down
14 changes: 12 additions & 2 deletions substrate/primitives/api/proc-macro/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ pub const CHANGED_IN_ATTRIBUTE: &str = "changed_in";
///
/// Is used when a trait method was renamed.
pub const RENAMED_ATTRIBUTE: &str = "renamed";
/// The `deprecated` attribute.
///
/// Is used when a trait or it's method was deprecated.
pub const DEPRECATED_ATTRIBUTE: &str = "deprecated";

/// All attributes that we support in the declaration of a runtime api trait.
pub const SUPPORTED_ATTRIBUTE_NAMES: &[&str] =
&[CORE_TRAIT_ATTRIBUTE, API_VERSION_ATTRIBUTE, CHANGED_IN_ATTRIBUTE, RENAMED_ATTRIBUTE];
pub const SUPPORTED_ATTRIBUTE_NAMES: &[&str] = &[
CORE_TRAIT_ATTRIBUTE,
API_VERSION_ATTRIBUTE,
CHANGED_IN_ATTRIBUTE,
RENAMED_ATTRIBUTE,
DEPRECATED_ATTRIBUTE,
];
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
Loading
Loading