Skip to content

Better error for missing index in CRV2 #4643

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

Merged
bkchr merged 5 commits into from
Jun 2, 2024
Merged

Better error for missing index in CRV2 #4643

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce87ac5
Better error for missing index in CRV2
gupnik May 30, 2024
eaf293f
Minor fix
gupnik May 30, 2024
8729b9e
".git/.scripts/commands/fmt/fmt.sh"
May 31, 2024
815a6a0
Merge branch 'master' into gupnik/crv2-missing-index
nixdotbuild May 31, 2024
37f1235
Remove the `expect`
bkchr Jun 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions substrate/frame/support/procedural/src/runtime/parse/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ impl Def {

for item in items.iter_mut() {
let mut pallet_item = None;
let mut pallet_index = 0;
let mut pallet_index = None;

let mut disable_call = false;
let mut disable_unsigned = false;
Expand All @@ -170,7 +170,7 @@ impl Def {
runtime_types = Some(types);
},
RuntimeAttr::PalletIndex(span, index) => {
pallet_index = index;
pallet_index = Some(index);
pallet_item = if let syn::Item::Type(item) = item {
Some(item.clone())
} else {
Expand All @@ -187,6 +187,13 @@ impl Def {
}
}

if pallet_index.is_none() {
Copy link
Member

@bkchr bkchr Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if pallet_index.is_none() {
let Some(pallet_index) = pallet_index else {

Copy link
Contributor Author

@gupnik gupnik Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pallet_index can be None for all items other than type aliases.

if let syn::Item::Type(item) = item {
Copy link
Member

@bkchr bkchr Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if doesn't make sense? I mean the index can only be set when the item is a type, at least judging based on the code above.

Copy link
Contributor Author

@gupnik gupnik Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if doesn't make sense? I mean the index can only be set when the item is a type, at least judging based on the code above.

So, the module marked with #[frame_support::runtime] can have arbitrary items. For example,

#[frame_support::runtime]
mod runtime {
    use super::*;
    
    pub type System = frame_system;
}

In this case, you only want this check for pub type System = frame_system; and not for use super::*;.

Basically, this change requires all type aliases within the runtime macro to be annotated with #[runtime::pallet_index] while the other items are preserved.

let msg = "Missing pallet index for pallet declaration. Please add `#[runtime::pallet_index(...)]`";
return Err(syn::Error::new(item.span(), &msg))
}
}

if let Some(pallet_item) = pallet_item {
match *pallet_item.ty.clone() {
syn::Type::Path(ref path) => {
Expand All @@ -209,7 +216,9 @@ impl Def {
let pallet = Pallet::try_from(
item.span(),
&pallet_item,
pallet_index,
pallet_index
.expect("Pallet item is present only for valid pallet index")
.into(),
disable_call,
disable_unsigned,
&bounds,
Expand Down
27 changes: 27 additions & 0 deletions substrate/frame/support/test/tests/runtime_ui/missing_pallet_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// 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.

#[frame_support::runtime]
mod runtime {
#[runtime::runtime]
#[runtime::derive(RuntimeCall)]
pub struct Runtime;

pub type System = frame_system;
}

fn main() {}
5 changes: 5 additions & 0 deletions substrate/frame/support/test/tests/runtime_ui/missing_pallet_index.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Missing pallet index for pallet declaration. Please add `#[runtime::pallet_index(...)]`
--> tests/runtime_ui/missing_pallet_index.rs:24:5
|
24 | pub type System = frame_system;
| ^^^