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.

Sign up for GitHub

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

Jump to bottom

impl_runtime_apis! {}: prevent the use of Self #1890

Closed
kianenigma opened this issue Oct 16, 2023 · 3 comments · Fixed by #4012
Closed

impl_runtime_apis! {}: prevent the use of Self #1890

kianenigma opened this issue Oct 16, 2023 · 3 comments · Fixed by #4012
Assignees
@dastansam
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@kianenigma
Copy link
Contributor

Assuming some type like type HeaderFor<T> is in scope, you would expect HeaderFor<Runtime> and HeaderFor<Self> would be equivalent, but it is not. I am not sure of the exact reason, but this can be confusing.

A simple patch to impl_runtime_apis should simply prevent the use of Self in this block of code.
@kianenigma kianenigma added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 16, 2023
@kianenigma kianenigma moved this from Backlog to To Do in Runtime / FRAME Oct 16, 2023
@kianenigma kianenigma added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Feb 5, 2024
@dastansam
Copy link
Contributor

hey @kianenigma,

I want to work on this.

I looked at the source of impl_runtime_apis!, there is an inner function impl_runtime_apis_impl that parses the macro input, do you suggest to prevent the usage of Self in this level? Or I misunderstood?

@kianenigma
Copy link
Contributor Author

That sounds about correct to me, but I don't know the exact answer either without actually solving the issue, so you should try for yourself :)

@dastansam
Copy link
Contributor

That sounds about correct to me, but I don't know the exact answer either without actually solving the issue, so you should try for yourself :)

Great, could you please assign this to me :)

@dastansam dastansam mentioned this issue Apr 5, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Nov 6, 2024
@dastansam @pkhry
@lexnv @bkchr
sp-api: impl_runtime_apis! replace the use of Self as a type argu…
a1aa71e 
…ment (#4012)

closes #1890 

### Overview 

Introduces similar checker struct to `CheckTraitDecls` in
`decl_runtime_apis!` - `CheckTraitImpls`. Overrides
`visit::visit_type_path` to detect usage of `Self` as a type argument
within the scope of `impl_runtime_apis!`.

**Note**: only prevents the usage of `Self` as a type argument in an
angle bracket `<>`, as it is the only use case that fails to compile.
For example, the code
[below](https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs#L1002)
compiles fine:

```rs
impl BridgeMessagesConfig<WithBridgeHubRococoMessagesInstance> for Runtime {
  fn is_relayer_rewarded(relayer: &Self::AccountId) -> bool {
	  let bench_lane_id = <Self as BridgeMessagesConfig<WithBridgeHubRococoMessagesInstance>>::bench_lane_id();
	  // ...
```

### Result

Given a block of code like this:

```rs
impl_runtime_apis! {
	impl apis::Core<Block> for Runtime {
		fn initialize_block(header: &HeaderFor<Self>) -> ExtrinsicInclusionMode {
			let _: HeaderFor<Self> = header.clone();
			RuntimeExecutive::initialize_block(header)
		}
		// ...
         }
// ...
```

<details open>

<summary>Output:</summary>

```bash
$ cargo build --release -p minimal-template-node
  error: `Self` can not be used as type argument in the scope of `impl_runtime_apis!`. Use `Runtime` instead.
     --> /polkadot-sdk/templates/minimal/runtime/src/lib.rs:133:11
      |
  133 |             let _: HeaderFor<Self> = header.clone();
      |                    ^^^^^^^^^^^^^^^

  error: `Self` can not be used as type argument in the scope of `impl_runtime_apis!`. Use `Runtime` instead.
     --> /polkadot-sdk/templates/minimal/runtime/src/lib.rs:132:32
      |
  132 |         fn initialize_block(header: &HeaderFor<Self>) -> ExtrinsicInclusionMode {
```

</details>

---------

Co-authored-by: Pavlo Khrystenko <[email protected]>
Co-authored-by: Pavlo Khrystenko <[email protected]>
Co-authored-by: Alexandru Vasile <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
@github-project-automation github-project-automation bot moved this from To Do to Done in Runtime / FRAME Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dastansam dastansam

Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sp-api: impl_runtime_apis! replace the use of Self as a type argument dastansam/polkadot-sdk
2 participants
@kianenigma @dastansam