From f8c0082ae2a422e7b11d63ad6dade4d8d8d4f0d4 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 18 Feb 2022 16:22:29 -0800 Subject: [PATCH 1/6] Fix exit status in `api-linter` --- tools/api-linter/Cargo.lock | 8 ++--- tools/api-linter/Cargo.toml | 2 +- tools/api-linter/src/main.rs | 35 ++++++++++++++++++---- tools/api-linter/tests/integration_test.rs | 5 +++- 4 files changed, 39 insertions(+), 11 deletions(-) diff --git a/tools/api-linter/Cargo.lock b/tools/api-linter/Cargo.lock index 9f18de95db..9e1689155f 100644 --- a/tools/api-linter/Cargo.lock +++ b/tools/api-linter/Cargo.lock @@ -112,9 +112,9 @@ checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" [[package]] name = "clap" -version = "3.0.14" +version = "3.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b63edc3f163b3c71ec8aa23f9bd6070f77edbf3d1d198b164afa90ff00e4ec62" +checksum = "e5f1fea81f183005ced9e59cdb01737ef2423956dac5a6d731b06b2ecfaa3467" dependencies = [ "atty", "bitflags", @@ -129,9 +129,9 @@ dependencies = [ [[package]] name = "clap_derive" -version = "3.0.14" +version = "3.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a1132dc3944b31c20dd8b906b3a9f0a5d0243e092d59171414969657ac6aa85" +checksum = "5fd1122e63869df2cb309f449da1ad54a7c6dfeb7c7e6ccd8e0825d9eb93bb72" dependencies = [ "heck", "proc-macro-error", diff --git a/tools/api-linter/Cargo.toml b/tools/api-linter/Cargo.toml index 7815e3659d..d39439db3d 100644 --- a/tools/api-linter/Cargo.toml +++ b/tools/api-linter/Cargo.toml @@ -10,7 +10,7 @@ publish = false [dependencies] anyhow = "1" cargo_metadata = "0.14" -clap = { version = "3", features = ["derive"] } +clap = { version = "3.1", features = ["derive"] } owo-colors = { version = "3", features = ["supports-colors"] } pest = "2" # For pretty error formatting rustdoc-types = "0.6" diff --git a/tools/api-linter/src/main.rs b/tools/api-linter/src/main.rs index b895e0e568..e2c0ff2edb 100644 --- a/tools/api-linter/src/main.rs +++ b/tools/api-linter/src/main.rs @@ -15,6 +15,7 @@ use smithy_rs_tool_common::shell::ShellOperation; use std::fmt; use std::fs; use std::path::PathBuf; +use std::process; use std::str::FromStr; use tracing_subscriber::prelude::*; use tracing_subscriber::EnvFilter; @@ -64,7 +65,7 @@ struct ApiLinterArgs { #[clap(long)] no_default_features: bool, /// Comma delimited list of features to enable in the crate - #[clap(long, use_delimiter = true)] + #[clap(long, use_value_delimiter = true)] features: Option>, /// Path to the Cargo manifest manifest_path: Option, @@ -86,7 +87,29 @@ enum Args { ApiLinter(ApiLinterArgs), } -fn main() -> Result<()> { +enum Error { + ValidationErrors, + Failure(anyhow::Error), +} + +impl From for Error { + fn from(err: anyhow::Error) -> Self { + Error::Failure(err) + } +} + +fn main() { + process::exit(match run_main() { + Ok(_) => 0, + Err(Error::ValidationErrors) => 1, + Err(Error::Failure(err)) => { + println!("{:#}", dbg!(err)); + 2 + } + }) +} + +fn run_main() -> Result<(), Error> { let Args::ApiLinter(args) = Args::parse(); if args.verbose { let filter_layer = EnvFilter::try_from_default_env() @@ -124,14 +147,15 @@ fn main() -> Result<()> { let crate_path = if let Some(manifest_path) = args.manifest_path { cargo_metadata_cmd.manifest_path(&manifest_path); manifest_path - .canonicalize()? + .canonicalize() + .context(here!())? .parent() .expect("parent path") .to_path_buf() } else { - std::env::current_dir()? + std::env::current_dir().context(here!())? }; - let cargo_metadata = cargo_metadata_cmd.exec()?; + let cargo_metadata = cargo_metadata_cmd.exec().context(here!())?; let cargo_features = resolve_features(&cargo_metadata)?; eprintln!("Running rustdoc to produce json doc output..."); @@ -162,6 +186,7 @@ fn main() -> Result<()> { errors.len(), "errors".if_supports_color(Stream::Stdout, |text| text.red()) ); + return Err(Error::ValidationErrors); } } OutputFormat::MarkdownTable => { diff --git a/tools/api-linter/tests/integration_test.rs b/tools/api-linter/tests/integration_test.rs index 450e136d29..0a7b566008 100644 --- a/tools/api-linter/tests/integration_test.rs +++ b/tools/api-linter/tests/integration_test.rs @@ -17,7 +17,10 @@ fn run_with_args(in_path: impl AsRef, args: &[&str]) -> String { cmd.arg(arg); } let output = cmd.output().expect("failed to start cargo-api-linter"); - handle_failure("cargo-api-linter", &output).unwrap(); + match output.status.code() { + Some(1) => { /* expected */ } + _ => handle_failure("cargo-api-linter", &output).unwrap(), + } let (stdout, _) = output_text(&output); stdout } From 6d706da8419152083e2fea6dcf85be6a7eae6846 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 18 Feb 2022 16:24:24 -0800 Subject: [PATCH 2/6] Run `api-linter` against `aws-smithy-types` in CI --- .github/workflows/ci-sdk.yaml | 4 ++++ rust-runtime/aws-smithy-types/additional-ci | 15 +++++++++++++++ 2 files changed, 19 insertions(+) create mode 100755 rust-runtime/aws-smithy-types/additional-ci diff --git a/.github/workflows/ci-sdk.yaml b/.github/workflows/ci-sdk.yaml index 3a68e4d993..7d9d6dbbb0 100644 --- a/.github/workflows/ci-sdk.yaml +++ b/.github/workflows/ci-sdk.yaml @@ -102,6 +102,10 @@ jobs: if [[ ! -f ~/.cargo/bin/cargo-hack ]]; then cargo install cargo-hack fi + # Install the api-linter tool for finding external types in public APIs + pushd tools/api-linter &>/dev/null + cargo install --debug --path . + popd &>/dev/null - name: Generate a name for the SDK id: gen-name run: echo "name=${GITHUB_REF##*/}" >> $GITHUB_ENV diff --git a/rust-runtime/aws-smithy-types/additional-ci b/rust-runtime/aws-smithy-types/additional-ci new file mode 100755 index 0000000000..853f83f27f --- /dev/null +++ b/rust-runtime/aws-smithy-types/additional-ci @@ -0,0 +1,15 @@ +#!/bin/bash +# +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0. +# + +# This script contains additional CI checks to run for this specific package + +set -e + +echo "### Checking for duplicate dependency versions in the normal dependency graph with all features enabled" +cargo tree -d --edges normal --all-features + +echo "### Running api-linter" +cargo +nightly api-linter From 34bb78a0ba0554aa2116f76a48a25deeba15ff67 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 18 Feb 2022 16:49:05 -0800 Subject: [PATCH 3/6] Hide external buffer types used by `primitive::Encoder` --- rust-runtime/aws-smithy-types/additional-ci | 2 +- .../aws-smithy-types/src/primitive.rs | 54 +++++++++++-------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/rust-runtime/aws-smithy-types/additional-ci b/rust-runtime/aws-smithy-types/additional-ci index 853f83f27f..fb5a6d69da 100755 --- a/rust-runtime/aws-smithy-types/additional-ci +++ b/rust-runtime/aws-smithy-types/additional-ci @@ -12,4 +12,4 @@ echo "### Checking for duplicate dependency versions in the normal dependency gr cargo tree -d --edges normal --all-features echo "### Running api-linter" -cargo +nightly api-linter +cargo +nightly api-linter --all-features diff --git a/rust-runtime/aws-smithy-types/src/primitive.rs b/rust-runtime/aws-smithy-types/src/primitive.rs index dcf4c209cf..c025a937b8 100644 --- a/rust-runtime/aws-smithy-types/src/primitive.rs +++ b/rust-runtime/aws-smithy-types/src/primitive.rs @@ -94,6 +94,18 @@ impl Parse for f64 { } } +/// This type exists to hide `itoa::Buffer` implementation detail from the public API +#[allow(missing_debug_implementations)] +#[doc(hidden)] +#[derive(Default)] +pub struct IntegerEncoder(itoa::Buffer); + +/// This type exists to hide `ryu::Buffer` implementation detail from the public API +#[allow(missing_debug_implementations)] +#[doc(hidden)] +#[derive(Default)] +pub struct FloatingEncoder(ryu::Buffer); + /// Primitive Type Encoder /// /// Encodes primitive types in Smithy's specified format. For floating-point numbers, @@ -107,25 +119,25 @@ pub enum Encoder { Bool(bool), /// 8-bit signed integer #[non_exhaustive] - I8(i8, itoa::Buffer), + I8(i8, IntegerEncoder), /// 16-bit signed integer #[non_exhaustive] - I16(i16, itoa::Buffer), + I16(i16, IntegerEncoder), /// 32-bit signed integer #[non_exhaustive] - I32(i32, itoa::Buffer), + I32(i32, IntegerEncoder), /// 64-bit signed integer #[non_exhaustive] - I64(i64, itoa::Buffer), + I64(i64, IntegerEncoder), /// 64-bit unsigned integer #[non_exhaustive] - U64(u64, itoa::Buffer), + U64(u64, IntegerEncoder), #[non_exhaustive] /// 32-bit IEEE 754 single-precision floating-point number - F32(f32, ryu::Buffer), + F32(f32, FloatingEncoder), /// 64-bit IEEE 754 double-precision floating-point number #[non_exhaustive] - F64(f64, ryu::Buffer), + F64(f64, FloatingEncoder), } impl Debug for Encoder { @@ -149,12 +161,12 @@ impl Encoder { match self { Encoder::Bool(true) => "true", Encoder::Bool(false) => "false", - Encoder::I8(v, buf) => buf.format(*v), - Encoder::I16(v, buf) => buf.format(*v), - Encoder::I32(v, buf) => buf.format(*v), - Encoder::I64(v, buf) => buf.format(*v), - Encoder::U64(v, buf) => buf.format(*v), - Encoder::F32(v, buf) => { + Encoder::I8(v, IntegerEncoder(buf)) => buf.format(*v), + Encoder::I16(v, IntegerEncoder(buf)) => buf.format(*v), + Encoder::I32(v, IntegerEncoder(buf)) => buf.format(*v), + Encoder::I64(v, IntegerEncoder(buf)) => buf.format(*v), + Encoder::U64(v, IntegerEncoder(buf)) => buf.format(*v), + Encoder::F32(v, FloatingEncoder(buf)) => { if v.is_nan() { float::NAN } else if *v == f32::INFINITY { @@ -165,7 +177,7 @@ impl Encoder { buf.format_finite(*v) } } - Encoder::F64(v, buf) => { + Encoder::F64(v, FloatingEncoder(buf)) => { if v.is_nan() { float::NAN } else if *v == f64::INFINITY { @@ -188,43 +200,43 @@ impl From for Encoder { impl From for Encoder { fn from(input: i8) -> Self { - Self::I8(input, itoa::Buffer::new()) + Self::I8(input, Default::default()) } } impl From for Encoder { fn from(input: i16) -> Self { - Self::I16(input, itoa::Buffer::new()) + Self::I16(input, Default::default()) } } impl From for Encoder { fn from(input: i32) -> Self { - Self::I32(input, itoa::Buffer::new()) + Self::I32(input, Default::default()) } } impl From for Encoder { fn from(input: i64) -> Self { - Self::I64(input, itoa::Buffer::new()) + Self::I64(input, Default::default()) } } impl From for Encoder { fn from(input: u64) -> Self { - Self::U64(input, itoa::Buffer::new()) + Self::U64(input, Default::default()) } } impl From for Encoder { fn from(input: f32) -> Self { - Self::F32(input, ryu::Buffer::new()) + Self::F32(input, Default::default()) } } impl From for Encoder { fn from(input: f64) -> Self { - Self::F64(input, ryu::Buffer::new()) + Self::F64(input, Default::default()) } } From 4f0499aafc9d9dafc461fc473c4f63ad3a607dc8 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 18 Feb 2022 18:42:18 -0800 Subject: [PATCH 4/6] Unpin nightly in CI --- .github/workflows/ci-sdk.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci-sdk.yaml b/.github/workflows/ci-sdk.yaml index 7d9d6dbbb0..ee6398ee1f 100644 --- a/.github/workflows/ci-sdk.yaml +++ b/.github/workflows/ci-sdk.yaml @@ -68,7 +68,7 @@ jobs: - name: Clippy run: cargo clippy --all-features - name: Unused Dependencies - run: cargo +nightly-2022-02-08 udeps + run: cargo +nightly udeps - name: Additional per-crate checks run: ../tools/additional-per-crate-checks.sh ./sdk/ ../tools/ci-cdk/ env: @@ -87,7 +87,7 @@ jobs: default: true - uses: actions-rs/toolchain@v1 with: - toolchain: nightly-2022-02-08 + toolchain: nightly default: false - name: Cache cargo bin uses: actions/cache@v2 @@ -97,7 +97,7 @@ jobs: - name: Install additional cargo binaries run: | if [[ ! -f ~/.cargo/bin/cargo-udeps ]]; then - cargo +nightly-2022-02-08 install cargo-udeps + cargo +nightly install cargo-udeps fi if [[ ! -f ~/.cargo/bin/cargo-hack ]]; then cargo install cargo-hack From 8934c30e7a58c1ce822f3545fa8b03bd4b6e616d Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Tue, 22 Feb 2022 11:33:44 -0800 Subject: [PATCH 5/6] Make `Encoder` a struct and split out an inner enum --- .../aws-smithy-types/src/primitive.rs | 91 ++++++++----------- 1 file changed, 37 insertions(+), 54 deletions(-) diff --git a/rust-runtime/aws-smithy-types/src/primitive.rs b/rust-runtime/aws-smithy-types/src/primitive.rs index c025a937b8..b32ac438ab 100644 --- a/rust-runtime/aws-smithy-types/src/primitive.rs +++ b/rust-runtime/aws-smithy-types/src/primitive.rs @@ -94,53 +94,26 @@ impl Parse for f64 { } } -/// This type exists to hide `itoa::Buffer` implementation detail from the public API -#[allow(missing_debug_implementations)] -#[doc(hidden)] -#[derive(Default)] -pub struct IntegerEncoder(itoa::Buffer); - -/// This type exists to hide `ryu::Buffer` implementation detail from the public API -#[allow(missing_debug_implementations)] -#[doc(hidden)] -#[derive(Default)] -pub struct FloatingEncoder(ryu::Buffer); - -/// Primitive Type Encoder -/// -/// Encodes primitive types in Smithy's specified format. For floating-point numbers, -/// Smithy requires that NaN and Infinity values be specially encoded. -/// -/// This type implements `From` for all Smithy primitive types. -#[non_exhaustive] -pub enum Encoder { +enum Inner { /// Boolean - #[non_exhaustive] Bool(bool), /// 8-bit signed integer - #[non_exhaustive] - I8(i8, IntegerEncoder), + I8(i8, itoa::Buffer), /// 16-bit signed integer - #[non_exhaustive] - I16(i16, IntegerEncoder), + I16(i16, itoa::Buffer), /// 32-bit signed integer - #[non_exhaustive] - I32(i32, IntegerEncoder), + I32(i32, itoa::Buffer), /// 64-bit signed integer - #[non_exhaustive] - I64(i64, IntegerEncoder), + I64(i64, itoa::Buffer), /// 64-bit unsigned integer - #[non_exhaustive] - U64(u64, IntegerEncoder), - #[non_exhaustive] + U64(u64, itoa::Buffer), /// 32-bit IEEE 754 single-precision floating-point number - F32(f32, FloatingEncoder), + F32(f32, ryu::Buffer), /// 64-bit IEEE 754 double-precision floating-point number - #[non_exhaustive] - F64(f64, FloatingEncoder), + F64(f64, ryu::Buffer), } -impl Debug for Encoder { +impl Debug for Inner { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { Self::Bool(v) => write!(f, "Bool({})", v), @@ -155,18 +128,28 @@ impl Debug for Encoder { } } +/// Primitive Type Encoder +/// +/// Encodes primitive types in Smithy's specified format. For floating-point numbers, +/// Smithy requires that NaN and Infinity values be specially encoded. +/// +/// This type implements `From` for all Smithy primitive types. +#[non_exhaustive] +#[derive(Debug)] +pub struct Encoder(Inner); + impl Encoder { /// Encodes a Smithy primitive as a string. pub fn encode(&mut self) -> &str { - match self { - Encoder::Bool(true) => "true", - Encoder::Bool(false) => "false", - Encoder::I8(v, IntegerEncoder(buf)) => buf.format(*v), - Encoder::I16(v, IntegerEncoder(buf)) => buf.format(*v), - Encoder::I32(v, IntegerEncoder(buf)) => buf.format(*v), - Encoder::I64(v, IntegerEncoder(buf)) => buf.format(*v), - Encoder::U64(v, IntegerEncoder(buf)) => buf.format(*v), - Encoder::F32(v, FloatingEncoder(buf)) => { + match &mut self.0 { + Inner::Bool(true) => "true", + Inner::Bool(false) => "false", + Inner::I8(v, buf) => buf.format(*v), + Inner::I16(v, buf) => buf.format(*v), + Inner::I32(v, buf) => buf.format(*v), + Inner::I64(v, buf) => buf.format(*v), + Inner::U64(v, buf) => buf.format(*v), + Inner::F32(v, buf) => { if v.is_nan() { float::NAN } else if *v == f32::INFINITY { @@ -177,7 +160,7 @@ impl Encoder { buf.format_finite(*v) } } - Encoder::F64(v, FloatingEncoder(buf)) => { + Inner::F64(v, buf) => { if v.is_nan() { float::NAN } else if *v == f64::INFINITY { @@ -194,49 +177,49 @@ impl Encoder { impl From for Encoder { fn from(input: bool) -> Self { - Self::Bool(input) + Self(Inner::Bool(input)) } } impl From for Encoder { fn from(input: i8) -> Self { - Self::I8(input, Default::default()) + Self(Inner::I8(input, itoa::Buffer::new())) } } impl From for Encoder { fn from(input: i16) -> Self { - Self::I16(input, Default::default()) + Self(Inner::I16(input, itoa::Buffer::new())) } } impl From for Encoder { fn from(input: i32) -> Self { - Self::I32(input, Default::default()) + Self(Inner::I32(input, itoa::Buffer::new())) } } impl From for Encoder { fn from(input: i64) -> Self { - Self::I64(input, Default::default()) + Self(Inner::I64(input, itoa::Buffer::new())) } } impl From for Encoder { fn from(input: u64) -> Self { - Self::U64(input, Default::default()) + Self(Inner::U64(input, itoa::Buffer::new())) } } impl From for Encoder { fn from(input: f32) -> Self { - Self::F32(input, Default::default()) + Self(Inner::F32(input, ryu::Buffer::new())) } } impl From for Encoder { fn from(input: f64) -> Self { - Self::F64(input, Default::default()) + Self(Inner::F64(input, ryu::Buffer::new())) } } From 45e4273843f268e0d909977f0c960716c0bf0fdc Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Tue, 22 Feb 2022 11:36:29 -0800 Subject: [PATCH 6/6] Update changelog --- CHANGELOG.next.toml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 72d79628e7..5c328bc56d 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -10,3 +10,9 @@ # references = ["smithy-rs#920"] # meta = { "breaking" = false, "tada" = false, "bug" = false } # author = "rcoh" + +[[smithy-rs]] +message = "`aws_smithy_types::primitive::Encoder` is now a struct rather than an enum, but its usage remains the same." +references = ["smithy-rs#1209"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "jdisanti"