Macros to use path instead of ident by juangirini · Pull Request #1474 · paritytech/polkadot-sdk

juangirini · 2023-09-08T15:57:16Z

Relates to #173
As part of the efforts towards making the FRAME umbrella crate in #1337, we need macros to work without frame-support, frame-system and codec, and type-info being in scope.
This PR leaves generate_crate_access_2018 ready to check for the frame crate once this one is ready and use its reexports instead of the local dependencies.

substrate/frame/support/procedural/src/benchmark.rs

kianenigma · 2023-09-13T08:46:30Z

substrate/frame/support/procedural/src/benchmark.rs

 				Expr::Cast(t) => {
 					let ty = t.ty.clone();
 					quote! {
-						<<T as frame_system::Config>::RuntimeOrigin as From<#ty>>::from(#origin);


all of this seem to have been hardcoded 🙈 well, not like anything broke because of it, but indeed not a good practice.

unless if was intentional? but I can't see a reason.

I can't see a reason either, and we need this unhardcoded moving forward to work with the frame umbrella crate

kianenigma

Def. on the right path, just need more tests.

kianenigma · 2023-09-13T08:46:41Z

substrate/frame/support/procedural/src/benchmark.rs

 				Expr::Cast(t) => {
 					let ty = t.ty.clone();
 					quote! {
-						<<T as frame_system::Config>::RuntimeOrigin as From<#ty>>::from(#origin);


unless if was intentional? but I can't see a reason.

kianenigma · 2023-09-13T08:48:19Z

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

 			#scrate::__private::tt_call! {
 				macro = [{ #path::tt_error_token }]
-				frame_support = [{ #scrate }]
+				your_tt_return = [{ #scrate::__private::tt_return }]


I recall this was one of the BIG BIG fixes in this PR when I was working on it.

Try this for yourself: if you are familiar with the tt_call pattern a bit, instead of passing in the your_tt_return, pass frame_support as it was in the past to the caller. Then have the caller call #frame_support::__private::tt_return instead of plain your_tt_return. These two seem equivalent, but the former never worked.

But, hearing what I just said, in the scope of this PR, it is pretty unclear why we are doing this. I recall my working example was minimal-runtime. Would be good if you also bring back some sort of example here to demonstrate that this change is actually needed. Ideally, this example will just a test in frame-support and not the frame crate.

These two seem equivalent, but the former never worked.

Yes, that's what looks at a glance... did you find out what's the reason?

Would be good if you also bring back some sort of example here to demonstrate that this change is actually needed

@kianenigma Probably the best example is by using the frame-crate itself, which makes me reconsider if bringing this change here was a good idea and instead bring it in the 'main' frame umbrella PR. WDYT?

I highly suspect that it didn't work before because the "receiver" didn't expect to see any "arguments" named your_tt_return and have been coded to only recognize frame_support as an "argument". If you take a look at tt_default_parts.rs, you'll see what I mean.

If my memory is good, the former didn't work because there was another error when giving a path instead of a single ident:
Like code was something like this:

frame_support = [{ $($frame_support:ident)::* }] } => { $($frame_support::)*__private::tt_return! {

but it should have been like this for path to work

frame_support = [{ $($frame_support:ident)::* }] } => { // see the semicolon is after the parentheses here $($frame_support)::*__private::tt_return! {

but when you implemented with your_tt_return you fixed this kind of error.

macro_magic might make this easier to reason about perhaps 😇

the specific workaround for passing paths to these btw is to do an interpolation of idents segmented by :: and with an optional leading ::. For some reason that just works positionally where path doesn't,

see: https://github.com/sam0x17/macro_magic/blob/main/core/src/lib.rs#L511

substrate/frame/support/procedural/src/pallet/parse/config.rs

juangirini · 2023-09-15T10:52:13Z

bot fmt

command-bot · 2023-09-15T10:52:17Z

@juangirini https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3709127 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 6-74c39310-e640-4ebd-8566-7f58d7263896 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-09-15T10:53:15Z

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

substrate/frame/support/procedural/tools/src/lib.rs

substrate/frame/support/procedural/src/benchmark.rs

substrate/frame/support/procedural/src/pallet/parse/config.rs

substrate/frame/support/test/compile_pass/Cargo.toml

* master: (54 commits) Publish `xcm-emulator` crate (#1881) Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849) Arkworks Elliptic Curve utils overhaul (#1870) Fix typos (#1878) fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176) Refactor staking ledger (#1484) Paired-key Crypto Scheme (#1705) Include polkadot version in artifact path (#1828) add link to rfc-0001 in broker README (#1862) Discard `Executor` (#1855) Macros to use path instead of ident (#1474) Remove clippy clone-double-ref lint noise (#1860) Refactor alliance benchmarks to v2 (#1868) Check executor params coherence (#1774) frame: use derive-impl for beefy and mmr pallets (#1867) sc-consensus-beefy: improve gossip logic (#1852) Adds instance support for composite enums (#1857) Fix links to implementers' guide (#1865) Disabled validators runtime API (#1257) Adding `try_state` hook for `Treasury` pallet (#1820) ...

…ribution * tsv-disabling-backing: (54 commits) Publish `xcm-emulator` crate (#1881) Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849) Arkworks Elliptic Curve utils overhaul (#1870) Fix typos (#1878) fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176) Refactor staking ledger (#1484) Paired-key Crypto Scheme (#1705) Include polkadot version in artifact path (#1828) add link to rfc-0001 in broker README (#1862) Discard `Executor` (#1855) Macros to use path instead of ident (#1474) Remove clippy clone-double-ref lint noise (#1860) Refactor alliance benchmarks to v2 (#1868) Check executor params coherence (#1774) frame: use derive-impl for beefy and mmr pallets (#1867) sc-consensus-beefy: improve gossip logic (#1852) Adds instance support for composite enums (#1857) Fix links to implementers' guide (#1865) Disabled validators runtime API (#1257) Adding `try_state` hook for `Treasury` pallet (#1820) ...

juangirini added 7 commits September 8, 2023 17:54

use path instead of ident

7102f28

fix clippy

933fd3f

add path changes to frame_support

dc43fa5

add frame conditional

16668ca

remove references to the frame-crate

ebd13e3

update fn description

270a6b7

update fn description

8be65aa

juangirini changed the title ~~Pre FRAME crate: macros to use path instead of ident~~ Macros to use path instead of ident Sep 11, 2023

juangirini requested review from ggwpez and kianenigma September 11, 2023 17:02

juangirini added 5 commits September 11, 2023 20:09

make benchmarks to use generate_crate_access_2018

850a77e

undo unrelated changes

5ae6e07

undo unrelated changes

04dc503

update macro description

95444ae

refactor generate_crate_access

6a773d0

kianenigma reviewed Sep 13, 2023

View reviewed changes

substrate/frame/support/procedural/src/benchmark.rs Show resolved Hide resolved

kianenigma reviewed Sep 13, 2023

View reviewed changes

juangirini added 3 commits September 13, 2023 13:58

unhardcode codec import

bc17557

bring in hypotetical frame crate

6d9887b

refactor expected_system_config and add tests

35747a1

".git/.scripts/commands/fmt/fmt.sh"

330af4a

make zepter happy

10bf65c

juangirini marked this pull request as ready for review September 15, 2023 11:20

juangirini requested review from a team and removed request for a team September 15, 2023 11:20

KiChjang reviewed Sep 25, 2023

View reviewed changes

substrate/frame/support/procedural/tools/src/lib.rs Outdated Show resolved Hide resolved

gui1117 reviewed Sep 25, 2023

View reviewed changes

substrate/frame/support/procedural/tools/src/lib.rs Outdated Show resolved Hide resolved

franciscoaguirre approved these changes Sep 25, 2023

View reviewed changes

liamaharon reviewed Sep 25, 2023

View reviewed changes

juangirini added 3 commits October 2, 2023 10:54

implement review suggestions

f303276

implement review suggestions

963be94

add dummy frame crate for testing

76de90d

KiChjang approved these changes Oct 3, 2023

View reviewed changes

liamaharon approved these changes Oct 3, 2023

View reviewed changes

juangirini added 12 commits October 4, 2023 06:19

add pallet test example with frame crate

32f2f1e

wip

877fdc3

make dummy pallet work with frame crate

c89c0c7

add dummy pallet with frame crate

8184085

add min runtime ui test

1ee920b

simplify example pallet

5050625

update comment

55daf79

zepter improvement

43ca2c2

Merge branch 'master' into jg/frame-crate-path

2a2b45f

fix broken test

b55615b

update compile test

d253b66

fix todo

d2009d3

juangirini requested a review from kianenigma October 13, 2023 14:23

kianenigma approved these changes Oct 14, 2023

View reviewed changes

kianenigma merged commit 7c87d61 into master Oct 14, 2023

kianenigma deleted the jg/frame-crate-path branch October 14, 2023 06:26

kiltbot mentioned this pull request Nov 7, 2023

[AUTOMATIC] Update Polkadot dependencies from 1.2.0 to 1.3.0 KILTprotocol/kilt-node#575

Closed

ahmadkaouk mentioned this pull request Nov 16, 2023

Update polkadot-sdk from v.1.1.0 to v1.3.0 moonbeam-foundation/moonbeam#2565

Closed

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024

Macros to use path instead of ident (paritytech#1474)

31cc512

Conversation

juangirini commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam0x17 Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juangirini commented Sep 15, 2023

Uh oh!

command-bot bot commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Sep 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

juangirini commented Sep 8, 2023 •

edited

Loading

sam0x17 Oct 9, 2023 •

edited

Loading

command-bot bot commented Sep 15, 2023 •

edited

Loading