From 7cb7e7ae6700d58931a18f41706c73775f053d0c Mon Sep 17 00:00:00 2001 From: Rain Date: Mon, 10 Mar 2025 14:04:38 -0700 Subject: [PATCH] [3/n] [daft-derive] attempt to better annotate semantic errors Add some span info to try and annotate semantic errors better (such as highlighting the name of the field that's not diffable). This isn't perfect for now because we still produce some call-site errors, but at least we produce one error that's attributed directly to the field name so rust-analyzer should be happy. I tried adding more bounds but ran into https://github.com/rust-lang/rust/issues/48214. Reviewers: andrewjstone Reviewed By: andrewjstone Pull Request: https://github.com/oxidecomputer/daft/pull/60 --- .github/workflows/ci.yml | 37 +++++++- Justfile | 4 +- daft-derive/src/internals/imp.rs | 19 ++-- daft-derive/tests/fixtures/README.md | 2 +- .../struct-field-not-diffable.output.rs | 39 ++++++++ .../invalid/struct-field-not-diffable.rs | 14 +++ .../invalid/struct-field-not-diffable.stderr | 88 +++++++++++++++++++ rust-toolchain.toml | 8 ++ 8 files changed, 197 insertions(+), 14 deletions(-) create mode 100644 daft-derive/tests/fixtures/invalid/output/struct-field-not-diffable.output.rs create mode 100644 daft-derive/tests/fixtures/invalid/struct-field-not-diffable.rs create mode 100644 daft-derive/tests/fixtures/invalid/struct-field-not-diffable.stderr create mode 100644 rust-toolchain.toml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e32d83..0df67c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,11 +38,38 @@ jobs: build-and-test: name: Build and test runs-on: ${{ matrix.os }} + strategy: + matrix: + os: [ubuntu-latest] + fail-fast: false + env: + RUSTFLAGS: -D warnings + CARGO_TERM_COLOR: always + steps: + - uses: actions/checkout@eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871 # v4 + - name: Install toolchain in rust-toolchain.toml + # GitHub Actions has an older rustup as of 2025-03-07 which doesn't accept + # `rustup toolchain install` with no arguments. Update rustup first. + run: rustup self update && rustup toolchain install + - uses: Swatinem/rust-cache@23bce251a8cd2ffc3c1075eaa2367cf899916d84 # v2 + - uses: taiki-e/install-action@cargo-hack + - uses: taiki-e/install-action@just + - uses: taiki-e/install-action@nextest + - name: Build + run: just powerset build + - name: Run tests + run: just powerset nextest run + - name: Doctests + run: just powerset test --doc + + build-and-test-msrv: + name: Build and test (MSRV) + runs-on: ${{ matrix.os }} strategy: matrix: os: [ubuntu-latest] # 1.81 is the MSRV - rust-version: ["1.81", stable] + rust-version: ["1.81"] fail-fast: false env: RUSTFLAGS: -D warnings @@ -54,7 +81,11 @@ jobs: - uses: Swatinem/rust-cache@23bce251a8cd2ffc3c1075eaa2367cf899916d84 # v2 - uses: taiki-e/install-action@cargo-hack - uses: taiki-e/install-action@just + - uses: taiki-e/install-action@nextest - name: Build run: just powerset build - - name: Test - run: just powerset test + # We don't run ui_test on the MSRV since compiler output varies by version. + - name: Test without ui_test + run: just powerset nextest run -E 'not binary(ui_test)' + - name: Doctests + run: just powerset test --doc diff --git a/Justfile b/Justfile index 5d5e3c2..e359c6d 100644 --- a/Justfile +++ b/Justfile @@ -1,3 +1,5 @@ +set positional-arguments + # Note: help messages should be 1 line long as required by just. # Print a help message. @@ -8,7 +10,7 @@ help: powerset *args: # Group third-party implementation features to avoid a full combinatorial # explosion -- we assume that they build independent of each other. - cargo hack --feature-powerset --workspace {{args}} --group-features newtype-uuid1,oxnet01,uuid1 --ignore-unknown-features + cargo hack --feature-powerset --workspace "$@" --group-features newtype-uuid1,oxnet01,uuid1 --ignore-unknown-features # Build docs for crates and direct dependencies rustdoc *args: diff --git a/daft-derive/src/internals/imp.rs b/daft-derive/src/internals/imp.rs index 6e423b3..a5f88bc 100644 --- a/daft-derive/src/internals/imp.rs +++ b/daft-derive/src/internals/imp.rs @@ -1,10 +1,11 @@ use super::error_store::{ErrorSink, ErrorStore}; use proc_macro2::{Span, TokenStream}; -use quote::{quote, ToTokens}; +use quote::{quote, quote_spanned, ToTokens}; use syn::{ - parse_quote, parse_str, visit::Visit, Attribute, Data, DataStruct, - DeriveInput, Expr, Field, Fields, GenericParam, Generics, Index, Lifetime, - LifetimeParam, Path, Token, WhereClause, WherePredicate, + parse_quote, parse_quote_spanned, parse_str, spanned::Spanned, + visit::Visit, Attribute, Data, DataStruct, DeriveInput, Expr, Field, + Fields, GenericParam, Generics, Index, Lifetime, LifetimeParam, Path, + Token, WhereClause, WherePredicate, }; pub struct DeriveDiffableOutput { @@ -563,11 +564,11 @@ impl DiffFields { let mut f = f.clone(); f.ty = if config.mode == FieldMode::Leaf { - parse_quote! { + parse_quote_spanned! {f.span()=> #daft_crate::Leaf<&#lt #ty> } } else { - parse_quote! { + parse_quote_spanned! {f.span()=> <#ty as #daft_crate::Diffable>::Diff<#lt> } }; @@ -591,7 +592,7 @@ impl DiffFields { trait_bound: &syn::TraitBound, ) -> WhereClause { let predicates = self.types().map(|ty| -> WherePredicate { - parse_quote! { + parse_quote_spanned! {ty.span()=> #ty: #trait_bound } }); @@ -627,14 +628,14 @@ fn generate_field_diffs( } }; if config.mode == FieldMode::Leaf { - quote! { + quote_spanned! {f.span()=> #field_name: #daft_crate::Leaf { before: &self.#field_name, after: &other.#field_name } } } else { - quote! { + quote_spanned! {f.span()=> #field_name: #daft_crate::Diffable::diff( &self.#field_name, &other.#field_name diff --git a/daft-derive/tests/fixtures/README.md b/daft-derive/tests/fixtures/README.md index 80b8b58..fd3ee48 100644 --- a/daft-derive/tests/fixtures/README.md +++ b/daft-derive/tests/fixtures/README.md @@ -20,4 +20,4 @@ These fixtures ensure that: Each file in `invalid` is automatically picked up by the snapshot and UI tests. -Like with valid fixtures, `snapshot_test.rs` tests all macro invocations annotated with `#[derive(Diffable)]`. \ No newline at end of file +Like with valid fixtures, `snapshot_test.rs` tests all macro invocations annotated with `#[derive(Diffable)]`. diff --git a/daft-derive/tests/fixtures/invalid/output/struct-field-not-diffable.output.rs b/daft-derive/tests/fixtures/invalid/output/struct-field-not-diffable.output.rs new file mode 100644 index 0000000..bbfcf45 --- /dev/null +++ b/daft-derive/tests/fixtures/invalid/output/struct-field-not-diffable.output.rs @@ -0,0 +1,39 @@ +struct MyStructDiff<'__daft> { + a: ::Diff<'__daft>, + b: ::Diff<'__daft>, +} +impl<'__daft> ::std::fmt::Debug for MyStructDiff<'__daft> +where + ::Diff<'__daft>: ::std::fmt::Debug, + ::Diff<'__daft>: ::std::fmt::Debug, +{ + fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result { + f.debug_struct(stringify!(MyStructDiff)) + .field(stringify!(a), &self.a) + .field(stringify!(b), &self.b) + .finish() + } +} +impl<'__daft> ::std::cmp::PartialEq for MyStructDiff<'__daft> +where + ::Diff<'__daft>: ::std::cmp::PartialEq, + ::Diff<'__daft>: ::std::cmp::PartialEq, +{ + fn eq(&self, other: &Self) -> bool { + self.a == other.a && self.b == other.b + } +} +impl<'__daft> ::std::cmp::Eq for MyStructDiff<'__daft> +where + ::Diff<'__daft>: ::std::cmp::Eq, + ::Diff<'__daft>: ::std::cmp::Eq, +{} +impl ::daft::Diffable for MyStruct { + type Diff<'__daft> = MyStructDiff<'__daft> where Self: '__daft; + fn diff<'__daft>(&'__daft self, other: &'__daft Self) -> MyStructDiff<'__daft> { + Self::Diff { + a: ::daft::Diffable::diff(&self.a, &other.a), + b: ::daft::Diffable::diff(&self.b, &other.b), + } + } +} diff --git a/daft-derive/tests/fixtures/invalid/struct-field-not-diffable.rs b/daft-derive/tests/fixtures/invalid/struct-field-not-diffable.rs new file mode 100644 index 0000000..2606665 --- /dev/null +++ b/daft-derive/tests/fixtures/invalid/struct-field-not-diffable.rs @@ -0,0 +1,14 @@ +use daft::Diffable; + +struct NonDiffable {} + +#[derive(Diffable)] +struct MyStruct { + a: i32, + b: NonDiffable, +} + +fn main() { + // MyStruct should still exist, even though the Diffable impl has errors. + let _ = MyStruct { a: 0, b: NonDiffable {} }; +} diff --git a/daft-derive/tests/fixtures/invalid/struct-field-not-diffable.stderr b/daft-derive/tests/fixtures/invalid/struct-field-not-diffable.stderr new file mode 100644 index 0000000..9c5628c --- /dev/null +++ b/daft-derive/tests/fixtures/invalid/struct-field-not-diffable.stderr @@ -0,0 +1,88 @@ +error[E0277]: the trait bound `NonDiffable: Diffable` is not satisfied + --> tests/fixtures/invalid/struct-field-not-diffable.rs:8:5 + | +8 | b: NonDiffable, + | ^ the trait `Diffable` is not implemented for `NonDiffable` + | + = help: the following other types implement trait `Diffable`: + &'a T + () + (A, B) + (A, B, C) + (A, B, C, D) + (A, B, C, D, E) + (A, B, C, D, E, F) + (A, B, C, D, E, F, G) + and $N others + +error[E0277]: the trait bound `NonDiffable: Diffable` is not satisfied + --> tests/fixtures/invalid/struct-field-not-diffable.rs:5:10 + | +5 | #[derive(Diffable)] + | ^^^^^^^^ the trait `Diffable` is not implemented for `NonDiffable` + | + = help: the following other types implement trait `Diffable`: + &'a T + () + (A, B) + (A, B, C) + (A, B, C, D) + (A, B, C, D, E) + (A, B, C, D, E, F) + (A, B, C, D, E, F, G) + and $N others + = note: this error originates in the derive macro `Diffable` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the trait bound `NonDiffable: Diffable` is not satisfied in `MyStructDiff<'__daft>` + --> tests/fixtures/invalid/struct-field-not-diffable.rs:5:10 + | +5 | #[derive(Diffable)] + | ^^^^^^^^ within `MyStructDiff<'__daft>`, the trait `Diffable` is not implemented for `NonDiffable` + | + = help: the following other types implement trait `Diffable`: + &'a T + () + (A, B) + (A, B, C) + (A, B, C, D) + (A, B, C, D, E) + (A, B, C, D, E, F) + (A, B, C, D, E, F, G) + and $N others +note: required because it appears within the type `MyStructDiff<'__daft>` + --> tests/fixtures/invalid/struct-field-not-diffable.rs:5:10 + | +5 | #[derive(Diffable)] + | ^^^^^^^^ +note: required by a bound in `daft::Diffable::Diff` + --> $WORKSPACE/daft/src/diffable.rs + | + | / type Diff<'daft> + | | where + | | Self: 'daft; + | |____________________^ required by this bound in `Diffable::Diff` + = note: this error originates in the derive macro `Diffable` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the trait bound `NonDiffable: Diffable` is not satisfied in `MyStructDiff<'__daft>` + --> tests/fixtures/invalid/struct-field-not-diffable.rs:5:10 + | +5 | #[derive(Diffable)] + | ^^^^^^^^ within `MyStructDiff<'__daft>`, the trait `Diffable` is not implemented for `NonDiffable` + | + = help: the following other types implement trait `Diffable`: + &'a T + () + (A, B) + (A, B, C) + (A, B, C, D) + (A, B, C, D, E) + (A, B, C, D, E, F) + (A, B, C, D, E, F, G) + and $N others +note: required because it appears within the type `MyStructDiff<'__daft>` + --> tests/fixtures/invalid/struct-field-not-diffable.rs:5:10 + | +5 | #[derive(Diffable)] + | ^^^^^^^^ + = note: the return type of a function must have a statically known size + = note: this error originates in the derive macro `Diffable` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/rust-toolchain.toml b/rust-toolchain.toml new file mode 100644 index 0000000..3788804 --- /dev/null +++ b/rust-toolchain.toml @@ -0,0 +1,8 @@ +# We use a specific toolchain revision to ensure our tests - which +# occasionally depend on specific compiler output - remain stable +# for all developers until the toolchain is explicitly advanced. +# The intent is to keep this updated as new stable versions are relased. + +[toolchain] +channel = "1.85.0" +profile = "default"