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 7 commits into
base: master
Choose a base branch
from

Conversation

pkhry
Copy link
Contributor

@pkhry pkhry commented Oct 31, 2024

Description

Removed #[deprecated] attribute from the generated code, so that code is not littered with useless warnings but DeprecationInfo is still propagated in MetadataIr

see 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.

@pkhry pkhry added R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Oct 31, 2024
@pkhry pkhry self-assigned this Oct 31, 2024
@pkhry pkhry removed the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 31, 2024
@pkhry
Copy link
Contributor Author

pkhry commented Oct 31, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 31, 2024

@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7675374 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 11-d04b7269-e0fa-486f-aeea-391e95ae9915 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 31, 2024

@pkhry Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7675374 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7675374/artifacts/download.

@pkhry
Copy link
Contributor Author

pkhry commented Oct 31, 2024

/cmd fmt

@pkhry pkhry marked this pull request as ready for review October 31, 2024 15:01
@pkhry pkhry requested a review from a team as a code owner October 31, 2024 15:01
@pkhry pkhry requested a review from kianenigma October 31, 2024 15:02
Copy link

Command "fmt" has started 🚀 See logs here

Copy link

Command "fmt" has finished ✅ See logs here

@pkhry
Copy link
Contributor Author

pkhry commented Nov 1, 2024

bot update-ui

@command-bot
Copy link

command-bot bot commented Nov 1, 2024

@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7677803 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/update-ui/update-ui.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 12-aa9c7dd9-df58-453f-9d18-c4c165eebfb0 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Nov 1, 2024

@pkhry Command "$PIPELINE_SCRIPTS_DIR/commands/update-ui/update-ui.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7677803 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7677803/artifacts/download.

@@ -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!

@pkhry pkhry requested a review from gui1117 November 4, 2024 09:49
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 4, 2024 09:49
@pkhry pkhry requested a review from davidk-pt November 5, 2024 11:33
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 5, 2024 11:34
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-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants