Check docs and run clippy on PRs (#326)

* Check docs, clippy, test run

* test parallel CI adapted from other package; is it faster?

* Remember to download substrate

* Nightly for cargo fmt

* Standardise CI names

* fix clippy complaints

* Ensure docs are valid and export publicly accessible types

* all-targets clippy, and fix additional lint errors

* newline in ci file

.github/workflows/rust.yml

@@ -2,39 +2,137 @@ name: Rust
 
 on:
   push:
-    branches: [ master ]
+    # Run jobs when commits are pushed to
+    # master or release-like branches:
+    branches:
+      - master
   pull_request:
-    branches: [ master ]
+    # Run jobs for any external PR that wants
+    # to merge to master, too:
+    branches:
+      - master
+
+env:
+  CARGO_TERM_COLOR: always
 
 jobs:
   build:
-
+    name: Cargo check
     runs-on: ubuntu-latest
-
     steps:
-    - uses: actions/checkout@v2
+      - name: Checkout sources
+        uses: actions/checkout@v2
 
-    - name: setup
-      uses: actions-rs/toolchain@v1
-      with:
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
           profile: minimal
-          toolchain: nightly
+          toolchain: stable
           override: true
-          components: rustfmt
-          target: wasm32-unknown-unknown
 
-    - name: download-substrate
-      run: |
-          curl "https://releases.parity.io/substrate/x86_64-debian:stretch/latest/substrate/substrate" --output substrate --location
-          chmod +x ./substrate
-          mkdir -p ~/.local/bin
-          mv substrate ~/.local/bin
+      - name: Rust Cache
+        uses: Swatinem/rust-cache@v1.3.0
 
-    - name: fmt
-      run: cargo fmt --all -- --check
+      - name: Build
+        uses: actions-rs/cargo@v1.0.3
+        with:
+          command: check
+          args: --all-targets --all-features --workspace
 
-    - name: build
-      run: cargo build --workspace --verbose
+  fmt:
+    name: Cargo fmt
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
 
-    - name: test
-      run: cargo test --workspace --verbose
+      - name: Install Rust nightly toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+            profile: minimal
+            toolchain: nightly
+            override: true
+            components: rustfmt
+
+      - name: Rust Cache
+        uses: Swatinem/rust-cache@v1.3.0
+
+      - name: Cargo fmt
+        uses: actions-rs/cargo@v1.0.3
+        with:
+          command: fmt
+          args: --all -- --check
+
+  docs:
+    name: Check documentation
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
+
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          override: true
+
+      - name: Rust Cache
+        uses: Swatinem/rust-cache@v1.3.0
+
+      - name: Check internal documentation links
+        run: RUSTDOCFLAGS="--deny broken_intra_doc_links" cargo doc --verbose --workspace --no-deps --document-private-items
+
+  tests:
+    name: Cargo test
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
+
+      - name: Download Substrate
+        run: |
+            curl "https://releases.parity.io/substrate/x86_64-debian:stretch/latest/substrate/substrate" --output substrate --location
+            chmod +x ./substrate
+            mkdir -p ~/.local/bin
+            mv substrate ~/.local/bin
+
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          override: true
+
+      - name: Rust Cache
+        uses: Swatinem/rust-cache@v1.3.0
+
+      - name: Cargo test
+        uses: actions-rs/cargo@v1.0.3
+        with:
+          command: test
+          args: --all-targets --workspace
+
+  clippy:
+    name: Cargo clippy
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
+
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+            profile: minimal
+            toolchain: stable
+            components: clippy
+            override: true
+
+      - name: Rust Cache
+        uses: Swatinem/rust-cache@v1.3.0
+
+      - name: Run clippy
+        uses: actions-rs/cargo@v1
+        with:
+          command: clippy
+          args: --all-targets -- -D warnings

.gitignore

@@ -1,4 +1,5 @@
 /target
 **/*.rs.bk
+**/.DS_Store
 Cargo.lock
-cargo-timing*
+cargo-timing*

codegen/src/api/storage.rs

@@ -44,7 +44,7 @@ pub fn generate_storage(
     let (storage_structs, storage_fns): (Vec<_>, Vec<_>) = storage
         .entries
         .iter()
-        .map(|entry| generate_storage_entry_fns(&type_gen, &pallet, entry))
+        .map(|entry| generate_storage_entry_fns(type_gen, pallet, entry))
         .unzip();
 
     quote! {

codegen/src/ir.rs

@@ -88,12 +88,9 @@ impl From<syn::Item> for Item {
                     let meta = attr.parse_meta().unwrap_or_else(|e| {
                         abort!(attr.span(), "Error parsing attribute: {}", e)
                     });
-                    let substitute_type_args =
-                        <attrs::Subxt as darling::FromMeta>::from_meta(&meta)
-                            .unwrap_or_else(|e| {
-                                abort!(attr.span(), "Error parsing attribute meta: {}", e)
-                            });
-                    substitute_type_args
+                    <attrs::Subxt as darling::FromMeta>::from_meta(&meta).unwrap_or_else(
+                        |e| abort!(attr.span(), "Error parsing attribute meta: {}", e),
+                    )
                 })
                 .collect::<Vec<_>>();
             if substitute_attrs.len() > 1 {
@@ -102,11 +99,11 @@ impl From<syn::Item> for Item {
                     "Duplicate `substitute_type` attributes"
                 )
             }
-            if let Some(attr) = substitute_attrs.iter().next() {
+            if let Some(attr) = substitute_attrs.get(0) {
                 let use_path = &use_.tree;
                 let substitute_with: syn::TypePath = syn::parse_quote!( #use_path );
                 let type_substitute = SubxtItem::TypeSubstitute {
-                    generated_type_path: attr.substitute_type().to_string(),
+                    generated_type_path: attr.substitute_type(),
                     substitute_with,
                 };
                 Self::Subxt(type_substitute)

src/events.rs

@@ -100,7 +100,7 @@ where
             let mut event_data = Vec::<u8>::new();
             let mut event_errors = Vec::<RuntimeError>::new();
             let result = self.decode_raw_event(
-                &event_metadata,
+                event_metadata,
                 input,
                 &mut event_data,
                 &mut event_errors,
@@ -204,9 +204,13 @@ where
             TypeDef::Variant(variant) => {
                 let variant_index = u8::decode(input)?;
                 variant_index.encode_to(output);
-                let variant = variant.variants().get(variant_index as usize).ok_or(
-                    Error::Other(format!("Variant {} not found", variant_index)),
-                )?;
+                let variant =
+                    variant
+                        .variants()
+                        .get(variant_index as usize)
+                        .ok_or_else(|| {
+                            Error::Other(format!("Variant {} not found", variant_index))
+                        })?;
                 for field in variant.fields() {
                     self.decode_type(field.ty().id(), input, output)?;
                 }
@@ -299,15 +303,21 @@ where
                     TypeDef::Composite(composite) => {
                         match composite.fields() {
                             [field] => {
-                                let field_ty =
-                                    self.metadata.resolve_type(field.ty().id()).ok_or(
-                                        MetadataError::TypeNotFound(field.ty().id()),
-                                    )?;
+                                let field_ty = self
+                                    .metadata
+                                    .resolve_type(field.ty().id())
+                                    .ok_or_else(|| {
+                                        MetadataError::TypeNotFound(field.ty().id())
+                                    })?;
                                 if let TypeDef::Primitive(primitive) = field_ty.type_def()
                                 {
                                     decode_compact_primitive(primitive)
                                 } else {
-                                    Err(EventsDecodingError::InvalidCompactType("Composite type must have a single primitive field".into()).into())
+                                    Err(EventsDecodingError::InvalidCompactType(
+                                    "Composite type must have a single primitive field"
+                                        .into(),
+                                )
+                                .into())
                                 }
                             }
                             _ => {

src/lib.rs

@@ -96,7 +96,11 @@ pub use crate::{
         Signer,
         UncheckedExtrinsic,
     },
-    metadata::Metadata,
+    metadata::{
+        Metadata,
+        MetadataError,
+        PalletMetadata,
+    },
     rpc::{
         BlockNumber,
         ExtrinsicSuccess,
@@ -167,10 +171,8 @@ pub enum Phase {
 
 /// A wrapper for any type `T` which implement encode/decode in a way compatible with `Vec<u8>`.
 ///
-/// This type is similar to [`WrapperOpaque`], but it differs in the way it stores the type `T`.
-/// While [`WrapperOpaque`] stores the decoded type, the [`WrapperKeepOpaque`] stores the type only
-/// in its opaque format, aka as a `Vec<u8>`. To access the real type `T` [`Self::try_decode`] needs
-/// to be used.
+/// [`WrapperKeepOpaque`] stores the type only in its opaque format, aka as a `Vec<u8>`. To
+/// access the real type `T` [`Self::try_decode`] needs to be used.
 #[derive(Debug, Eq, PartialEq, Default, Clone, Decode, Encode)]
 pub struct WrapperKeepOpaque<T> {
     data: Vec<u8>,
@@ -182,7 +184,7 @@ impl<T: Decode> WrapperKeepOpaque<T> {
     ///
     /// Returns `None` if the decoding failed.
     pub fn try_decode(&self) -> Option<T> {
-        T::decode_all(&mut &self.data[..]).ok()
+        T::decode_all(&self.data[..]).ok()
     }
 
     /// Returns the length of the encoded `T`.

src/metadata.rs

@@ -73,6 +73,7 @@ pub enum MetadataError {
     /// Constant is not in metadata.
     #[error("Constant {0} not found")]
     ConstantNotFound(&'static str),
+    /// Type is not in metadata.
     #[error("Type {0} missing from type registry")]
     TypeNotFound(u32),
 }
@@ -91,7 +92,7 @@ impl Metadata {
     pub fn pallet(&self, name: &'static str) -> Result<&PalletMetadata, MetadataError> {
         self.pallets
             .get(name)
-            .ok_or(MetadataError::PalletNotFound(name.to_string()))
+            .ok_or_else(|| MetadataError::PalletNotFound(name.to_string()))
     }
 
     /// Returns the metadata for the event at the given pallet and event indices.
@@ -131,6 +132,7 @@ impl Metadata {
     }
 }
 
+/// Metadata for a specific pallet.
 #[derive(Clone, Debug)]
 pub struct PalletMetadata {
     index: u8,
@@ -141,6 +143,7 @@ pub struct PalletMetadata {
 }
 
 impl PalletMetadata {
+    /// Encode a call based on this pallet metadata.
     pub fn encode_call<C>(&self, call: &C) -> Result<Encoded, MetadataError>
     where
         C: Call,
@@ -154,6 +157,7 @@ impl PalletMetadata {
         Ok(Encoded(bytes))
     }
 
+    /// Return [`StorageEntryMetadata`] given some storage key.
     pub fn storage(
         &self,
         key: &'static str,
@@ -163,7 +167,7 @@ impl PalletMetadata {
             .ok_or(MetadataError::StorageNotFound(key))
     }
 
-    /// Get a constant's metadata by name
+    /// Get a constant's metadata by name.
     pub fn constant(
         &self,
         key: &'static str,
@@ -239,11 +243,11 @@ impl TryFrom<RuntimeMetadataPrefixed> for Metadata {
 
     fn try_from(metadata: RuntimeMetadataPrefixed) -> Result<Self, Self::Error> {
         if metadata.0 != META_RESERVED {
-            return Err(InvalidMetadataError::InvalidPrefix.into())
+            return Err(InvalidMetadataError::InvalidPrefix)
         }
         let metadata = match metadata.1 {
             RuntimeMetadata::V14(meta) => meta,
-            _ => return Err(InvalidMetadataError::InvalidVersion.into()),
+            _ => return Err(InvalidMetadataError::InvalidVersion),
         };
 
         let get_type_def_variant = |type_id: u32| {

src/rpc.rs

@@ -616,10 +616,10 @@ impl<T: Config> Rpc<T> {
         Err(RpcError::Custom("RPC subscription dropped".into()).into())
     }
 
-    async fn process_block<'a>(
+    async fn process_block(
         &self,
         events_sub: EventStorageSubscription<T>,
-        decoder: &'a EventsDecoder<T>,
+        decoder: &EventsDecoder<T>,
         block_hash: T::Hash,
         ext_hash: T::Hash,
     ) -> Result<ExtrinsicSuccess<T>, Error> {

src/subscription.rs

@@ -391,9 +391,9 @@ mod tests {
                                 ]
                                 .into_iter(),
                             )),
-                            block: block_filter.clone(),
-                            extrinsic: extrinsic_filter.clone(),
-                            event: event_filter.clone(),
+                            block: block_filter,
+                            extrinsic: extrinsic_filter,
+                            event: event_filter,
                             events: Default::default(),
                             finished: false,
                         };

tests/integration/client.rs

@@ -117,7 +117,7 @@ async fn test_iter() {
         .await
         .unwrap();
     let mut i = 0;
-    while let Some(_) = iter.next().await.unwrap() {
+    while iter.next().await.unwrap().is_some() {
         i += 1;
     }
     assert_eq!(i, 13);

tests/integration/frame/balances.rs

@@ -51,13 +51,13 @@ async fn tx_basic_transfer() {
     let alice_pre = api
         .storage()
         .system()
-        .account(alice.account_id().clone().into(), None)
+        .account(alice.account_id().clone(), None)
         .await
         .unwrap();
     let bob_pre = api
         .storage()
         .system()
-        .account(bob.account_id().clone().into(), None)
+        .account(bob.account_id().clone(), None)
         .await
         .unwrap();
 
@@ -74,7 +74,7 @@ async fn tx_basic_transfer() {
         .unwrap();
     let _extrinsic_success = result
         .find_event::<system::events::ExtrinsicSuccess>()
-        .expect("Failed to decode ExtrinisicSuccess".into())
+        .expect("Failed to decode ExtrinisicSuccess")
         .expect("Failed to find ExtrinisicSuccess");
 
     let expected_event = balances::events::Transfer(
@@ -87,13 +87,13 @@ async fn tx_basic_transfer() {
     let alice_post = api
         .storage()
         .system()
-        .account(alice.account_id().clone().into(), None)
+        .account(alice.account_id().clone(), None)
         .await
         .unwrap();
     let bob_post = api
         .storage()
         .system()
-        .account(bob.account_id().clone().into(), None)
+        .account(bob.account_id().clone(), None)
         .await
         .unwrap();
 
@@ -200,7 +200,7 @@ async fn transfer_subscription() {
     let cxt = test_context().await;
     let sub = cxt.client().rpc().subscribe_events().await.unwrap();
     let decoder = cxt.client().events_decoder();
-    let mut sub = EventSubscription::<DefaultConfig>::new(sub, &decoder);
+    let mut sub = EventSubscription::<DefaultConfig>::new(sub, decoder);
     sub.filter_event::<balances::events::Transfer>();
 
     cxt.api

tests/integration/frame/contracts.rs

@@ -56,7 +56,7 @@ impl ContractsTestContext {
     }
 
     fn client(&self) -> &Client<DefaultConfig> {
-        &self.cxt.client()
+        self.cxt.client()
     }
 
     fn contracts_tx(&self) -> TransactionApi<DefaultConfig> {
@@ -172,7 +172,7 @@ async fn tx_instantiate() {
     let ctx = ContractsTestContext::init().await;
     let (code_hash, _) = ctx.instantiate_with_code().await.unwrap();
 
-    let instantiated = ctx.instantiate(code_hash.into(), vec![], vec![1u8]).await;
+    let instantiated = ctx.instantiate(code_hash, vec![], vec![1u8]).await;
 
     assert!(
         instantiated.is_ok(),

tests/integration/frame/sudo.rs

@@ -32,7 +32,7 @@ type BalancesCall = runtime_types::pallet_balances::pallet::Call;
 #[async_std::test]
 async fn test_sudo() {
     let alice = PairSigner::<DefaultConfig, _>::new(AccountKeyring::Alice.pair());
-    let bob = AccountKeyring::Bob.to_account_id().clone().into();
+    let bob = AccountKeyring::Bob.to_account_id().into();
     let cxt = test_context().await;
 
     let call = Call::Balances(BalancesCall::transfer {

tests/integration/frame/system.rs

@@ -37,7 +37,7 @@ async fn storage_account() {
         .api
         .storage()
         .system()
-        .account(alice.account_id().clone().into(), None)
+        .account(alice.account_id().clone(), None)
         .await;
     assert_matches!(account_info, Ok(_))
 }

tests/integration/runtime.rs

@@ -14,6 +14,8 @@
 // You should have received a copy of the GNU General Public License
 // along with subxt.  If not, see <http://www.gnu.org/licenses/>.
 
+#![allow(clippy::too_many_arguments)]
+
 #[subxt::subxt(
     runtime_metadata_path = "tests/integration/node_runtime.scale",
     generated_type_derives = "Debug, Eq, PartialEq"

tests/integration/utils/node_proc.rs

@@ -72,7 +72,7 @@ where
                 err
             );
             log::error!("{}", err);
-            return Err(err.into())
+            return Err(err)
         }
         Ok(())
     }
@@ -132,7 +132,7 @@ impl TestNodeProcessBuilder {
 
         let ws_port = if self.scan_port_range {
             let (p2p_port, http_port, ws_port) = next_open_port()
-                .ok_or("No available ports in the given port range".to_owned())?;
+                .ok_or_else(|| "No available ports in the given port range".to_owned())?;
 
             cmd.arg(format!("--port={}", p2p_port));
             cmd.arg(format!("--rpc-port={}", http_port));
@@ -169,7 +169,7 @@ impl TestNodeProcessBuilder {
                 Err(err) => {
                     if attempts < MAX_ATTEMPTS {
                         attempts += 1;
-                        wait_secs = wait_secs * 2; // backoff
+                        wait_secs *= 2; // backoff
                         continue
                     }
                     break Err(err)
@@ -187,7 +187,7 @@ impl TestNodeProcessBuilder {
                 proc.kill().map_err(|e| {
                     format!("Error killing substrate process '{}': {}", proc.id(), e)
                 })?;
-                Err(err.into())
+                Err(err)
             }
         }
     }
@@ -216,14 +216,11 @@ fn next_open_port() -> Option<(u16, u16, u16)> {
             Ordering::SeqCst,
         );
         let next = PORT.fetch_add(1, Ordering::SeqCst);
-        match TcpListener::bind(("0.0.0.0", next)) {
-            Ok(_) => {
-                ports.push(next);
-                if ports.len() == 3 {
-                    return Some((ports[0], ports[1], ports[2]))
-                }
+        if TcpListener::bind(("0.0.0.0", next)).is_ok() {
+            ports.push(next);
+            if ports.len() == 3 {
+                return Some((ports[0], ports[1], ports[2]))
             }
-            Err(_) => (),
         }
         ports_scanned += 1;
         if ports_scanned == MAX_PORTS {