-
Notifications
You must be signed in to change notification settings - Fork 798
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
sp-api: impl_runtime_apis!
replace the use of Self
as a type argument
#4012
Merged
pkhry
merged 33 commits into
paritytech:master
from
dastansam:feat/runtime-api-restrict-self
Nov 6, 2024
Merged
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
6e2607e
Initial solution
dastansam cedcc57
Merge branch 'master' into feat/runtime-api-restrict-self
dastansam f89b983
Add prdoc
dastansam 7751770
Merge branch 'feat/runtime-api-restrict-self' of github-dastansam:das…
dastansam 598f9b0
Fix prdoc
dastansam 30dd063
Use `Visit` trait
dastansam c0a959d
Prevent type arguments only
dastansam ab26d18
Rename checker struct
dastansam 6c5abbe
Update pr_4012.prdoc
dastansam 706e2f7
Final changes
dastansam e66a3c5
Merge branch 'feat/runtime-api-restrict-self' of github-dastansam:das…
dastansam 48db48a
Merge branch 'master' into feat/runtime-api-restrict-self
dastansam 6f2aebb
Merge branch 'master' into feat/runtime-api-restrict-self
dastansam ff0c518
Merge branch 'master' into HEAD
pkhry 14cbca2
Rename "Self" to "Runtime" in sp_api::impl_item_apis
pkhry eb62a31
revert cargo lock change
pkhry 1339e3c
"Runtime" -> "ItemImpl.self_ty"
pkhry e778552
add test for renamer
pkhry 8ff5005
pr-doc
pkhry 0828b6c
remove prdoc
pkhry a736e4f
bring back prdoc
pkhry 3d4d25f
clippy
pkhry 0d0f89f
add prdoc-change
pkhry b5ed2cf
Update prdoc/pr_4012.prdoc
pkhry b7eec20
review comments
pkhry 01f5e92
Merge branch 'master' into feat/runtime-api-restrict-self
pkhry 2ed3ba7
Merge branch 'master' into feat/runtime-api-restrict-self
pkhry b05cf24
Update substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs
pkhry d425328
Update substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs
pkhry da66f7b
suggestions fixup
pkhry 35227cf
Merge branch 'master' into feat/runtime-api-restrict-self
pkhry b24f886
Merge remote-tracking branch 'origin/master' into HEAD
pkhry ec25795
clippy
pkhry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
# 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: "`impl_runtime_apis!`: replace the use of `Self` with `Runtime`" | ||
|
||
doc: | ||
- audience: Runtime Dev | ||
description: | | ||
Currently, if there is a type alias similar to `type HeaderFor<T>` in the scope, it makes sense to expect that | ||
`HeaderFor<Runtime>` and `HeaderFor<Self>` are equivalent. However, this is not the case. It currently leads to | ||
a compilation error that `Self is not in scope`, which is confusing. This PR introduces a visitor, similar to | ||
`CheckTraitDecl` in `decl_runtime_apis!`, `ReplaceSelfImpl`. It identifies usage of `Self` as a type argument in | ||
`impl_runtime_apis!` and replaces `Self` with an explicit `Runtime` type. | ||
|
||
For example, the following example code will be transformed before expansion: | ||
```rust | ||
impl apis::Core<Block> for Runtime { | ||
fn initialize_block(header: &HeaderFor<Self>) -> ExtrinsicInclusionMode { | ||
let _: HeaderFor<Self> = header.clone(); | ||
RuntimeExecutive::initialize_block(header) | ||
} | ||
} | ||
``` | ||
Instead, it will be passed to macro as: | ||
```rust | ||
impl apis::Core<Block> for Runtime { | ||
fn initialize_block(header: &HeaderFor<Runtime>) -> ExtrinsicInclusionMode { | ||
let _: HeaderFor<Runtime> = header.clone(); | ||
RuntimeExecutive::initialize_block(header) | ||
} | ||
} | ||
``` | ||
crates: | ||
- name: sp-api | ||
bump: none | ||
- name: sp-api-proc-macro | ||
bump: none |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
ie would it currently work if I had something like this?:
let foo: Self = ...
Or is that sort of thing nonsensical anyways.
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'd keep things conservative in this regard