DeprecationInfo Remove deprecated attribute from generated code.
#6312
Conversation
pkhry
added
R0-silent
pkhry
removed
the
T1-FRAME
|
bot fmt |
|
@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7675374 was started for your command Comment |
|
@pkhry Command |
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
|
bot update-ui |
|
@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7677803 was started for your command Comment |
|
@pkhry Command |
| @@ -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); | |||
There was a problem hiding this comment.
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.
I was thinking a more fine-grained implementation.
Like the dispatch_bypass_filter would be implemented with #[allow(deprecated)] just above the function call
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.
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!
There was a problem hiding this comment.
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.
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_attrsinside code generated byconstruct_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.
There was a problem hiding this comment.
Not sure what the purpose of deprecating these types would be. I mean I would understand if the entire pallet would be deprecated.
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
bot update-ui |
|
@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7871580 was started for your command Comment |
|
@pkhry Command |
There was a problem hiding this comment.
Some more comments, overall I think it is ok, do you think there is still time to not put some deprecate information in the metadata?
Like do you think it makes sense to reduce the scope of the deprecated information to only most relevant information?
But the PR should be good now.
| @@ -82,6 +88,7 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream { | |||
| quote::quote_spanned!(def.pallet_struct.attr_span => | |||
| impl<#type_impl_gen> #pallet_ident<#type_use_gen> #config_where_clause { | |||
| #[doc(hidden)] | |||
| #[allow(deprecated)] | |||
There was a problem hiding this comment.
This is only when the full error type is deprecated, I also would not handle this case personally. But it can be fine.
substrate/frame/support/procedural/src/pallet/expand/storage.rs
Outdated
Show resolved
Hide resolved
| @@ -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.
why is deprecated_semver needed by the way?
Also people will have to bound useless_deprecated, or we can remove it?
There was a problem hiding this comment.
there were tests during the initial review with non-semver strings in the deprecation attribute. removed all of allows above
Description
Removed
#[deprecated]attribute from the generated code, so that code is not littered with useless warnings butDeprecationInfois still propagated inMetadataIrsee an example of warnings being too noisy: #6169
Review Notes
The change itself is just adding attribute removal code after specific steps during macro expansion.