-
Notifications
You must be signed in to change notification settings - Fork 726
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
01f3639
524fffa
f199b9f
71e89d4
eb8077c
74c7b9f
6fe881a
c9eed15
0978bff
acd6c55
f87b3e7
4e638d6
5714f3c
9ada2aa
39ee4cc
99c0436
38affa5
b1c426a
5f9a1df
377981c
a23571b
b9c70c2
7e0712d
a1a14ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}; | ||
|
@@ -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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is deprecated_semver needed by the way? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
@@ -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::*; | ||
|
@@ -138,6 +139,7 @@ pub mod pallet { | |
{ | ||
/// Some comment | ||
/// Some comment | ||
#[deprecated = "test 2"] | ||
#[pallet::constant] | ||
type MyGetParam: Get<u32>; | ||
|
||
|
@@ -178,6 +180,7 @@ pub mod pallet { | |
} | ||
|
||
#[pallet::pallet] | ||
#[deprecated = "example"] | ||
#[pallet::storage_version(STORAGE_VERSION)] | ||
pub struct Pallet<T>(_); | ||
|
||
|
@@ -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(); | ||
|
There was a problem hiding this comment.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted this change
There was a problem hiding this comment.
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 callpolkadot-sdk/substrate/frame/support/procedural/src/pallet/expand/call.rs
Line 460 in 74c7b9f
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.
There was a problem hiding this comment.
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 ofconstruct_runtime!
. Ie if we deprecate an event or an error we dont have any attributes to includemaybe_allow_attrs
inside code generated byconstruct_runtime!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about
construct_runtime!
if the whole call or event or pallet is deprecated, then I think the writer of theconstruct_runtime!
should add#[allow(deprecated)]
on top of the pallet variant: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
orCall
orPallet
, not when deprecating a single call inside the palletCall
.There was a problem hiding this comment.
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.