Skip to content

Commit

Permalink
Include parents in "did you mean" suggestions
Browse files Browse the repository at this point in the history
For flattened members, this makes the "did you mean" suggestions accurate and more useful
  • Loading branch information
TedDriggs committed Feb 22, 2024
1 parent 08c0634 commit e782166
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 6 deletions.
40 changes: 35 additions & 5 deletions core/src/codegen/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,14 @@ impl<'a> Field<'a> {
Declaration(self)
}

pub fn as_flatten_initializer(&'a self) -> FlattenInitializer<'a> {
FlattenInitializer(self)
pub fn as_flatten_initializer(
&'a self,
parent_field_names: Vec<&'a str>,
) -> FlattenInitializer<'a> {
FlattenInitializer {
field: self,
parent_field_names,
}
}

pub fn as_match(&'a self) -> MatchArm<'a> {
Expand Down Expand Up @@ -93,15 +99,39 @@ impl<'a> ToTokens for Declaration<'a> {
}
}

pub struct FlattenInitializer<'a>(&'a Field<'a>);
pub struct FlattenInitializer<'a> {
field: &'a Field<'a>,
parent_field_names: Vec<&'a str>,
}

impl<'a> ToTokens for FlattenInitializer<'a> {
fn to_tokens(&self, tokens: &mut TokenStream) {
let field = self.0;
let Self {
field,
parent_field_names,
} = self;
let ident = field.ident;

let add_parent_fields = if parent_field_names.is_empty() {
None
} else {
Some(quote! {
.map_err(|e| e.add_sibling_alts_for_unknown_field(&[#(#parent_field_names),*]))
})
};

tokens.append_all(quote! {
#ident = (true, __errors.handle(::darling::FromMeta::from_list(__flatten.into_iter().cloned().map(::darling::ast::NestedMeta::Meta).collect::<Vec<_>>().as_slice())));
#ident = (true,
__errors.handle(
::darling::FromMeta::from_list(
__flatten
.into_iter()
.cloned()
.map(::darling::ast::NestedMeta::Meta)
.collect::<Vec<_>>().as_slice()
) #add_parent_fields
)
);
});
}
}
Expand Down
6 changes: 5 additions & 1 deletion core/src/codegen/variant_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ impl<'a> FieldsGen<'a> {
{
(
quote! { let mut __flatten = vec![]; },
Some(flatten_field.as_flatten_initializer()),
Some(flatten_field.as_flatten_initializer(self.field_names().collect())),
)
} else {
(quote!(), None)
Expand Down Expand Up @@ -119,4 +119,8 @@ impl<'a> FieldsGen<'a> {

quote!(#(#inits),*)
}

fn field_names(&self) -> impl Iterator<Item = &str> {
self.fields.iter().filter_map(Field::as_name)
}
}
18 changes: 18 additions & 0 deletions core/src/error/kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,24 @@ impl ErrorUnknownField {
ErrorUnknownField::new(field, did_you_mean(field, alternates))
}

/// Add more alternate field names to the error, updating the `did_you_mean` suggestion
/// if a closer match to the unknown field's name is found.
pub fn add_alts<'a, T, I>(&mut self, alternates: I)
where
T: AsRef<str> + 'a,
I: IntoIterator<Item = &'a T>,
{
if let Some(bna) = did_you_mean(&self.name, alternates) {
if let Some(current) = &self.did_you_mean {
if bna.0 > current.0 {
self.did_you_mean = Some(bna);
}
} else {
self.did_you_mean = Some(bna);
}
}
}

#[cfg(feature = "diagnostics")]
pub fn into_diagnostic(self, span: Option<::proc_macro2::Span>) -> ::proc_macro::Diagnostic {
let base = span
Expand Down
45 changes: 45 additions & 0 deletions core/src/error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,51 @@ impl Error {
self.kind.len()
}

/// Consider additional field names as "did you mean" suggestions for
/// unknown field errors **if and only if** the caller appears to be operating
/// at error's origin (meaning no calls to [`Self::at`] have yet taken place).
///
/// # Usage
/// `flatten` fields in derived trait implementations rely on this method to offer correct
/// "did you mean" suggestions in errors.
///
/// Because the `flatten` field receives _all_ unknown fields, if a user mistypes a field name
/// that is present on the outer struct but not the flattened struct, they would get an incomplete
/// or inferior suggestion unless this method was invoked.
pub fn add_sibling_alts_for_unknown_field<'a, T, I>(mut self, alternates: I) -> Self
where
T: AsRef<str> + 'a,
I: IntoIterator<Item = &'a T>,
{
// The error may have bubbled up before this method was called,
// and in those cases adding alternates would be incorrect.
if !self.locations.is_empty() {
return self;
}

if let ErrorKind::UnknownField(unknown_field) = &mut self.kind {
unknown_field.add_alts(alternates);
} else if let ErrorKind::Multiple(errors) = self.kind {
let alternates = alternates.into_iter().collect::<Vec<_>>();
self.kind = ErrorKind::Multiple(
errors
.into_iter()
.map(|err| {
err.add_sibling_alts_for_unknown_field(
// This clone seems like it shouldn't be necessary.
// Attempting to borrow alternates here leads to the following compiler error:
//
// error: reached the recursion limit while instantiating `darling::Error::add_sibling_alts_for_unknown_field::<'_, &&&&..., ...>`
alternates.clone(),
)
})
.collect(),
)
}

self
}

/// Adds a location chain to the head of the error's existing locations.
fn prepend_at(mut self, mut locations: Vec<String>) -> Self {
if !locations.is_empty() {
Expand Down
60 changes: 60 additions & 0 deletions tests/flatten.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,63 @@ fn flattening_into_hashmap() {
assert_eq!(parsed.volume, 10);
assert_eq!(parsed.others.len(), 2);
}

#[derive(FromMeta)]
#[allow(dead_code)]
struct Person {
first: String,
last: String,
parent: Option<Box<Person>>,
}

#[derive(FromDeriveInput)]
#[darling(attributes(v))]
#[allow(dead_code)]
struct Outer {
#[darling(flatten)]
owner: Person,
#[darling(default)]
blast: bool,
}

/// This test makes sure that field names from parent structs are not inappropriately
/// offered as alternates for unknown field errors in child structs.
///
/// A naive implementation that tried to offer all the flattened fields for "did you mean"
/// could inspect all errors returned by the flattened field's `from_list` call and add the
/// parent's field names as alternates to all unknown field errors.
///
/// THIS WOULD BE INCORRECT. Those unknown field errors may have already come from
/// child fields within the flattened struct, where the parent's field names are not valid.
#[test]
fn do_not_suggest_invalid_alts() {
let errors = Outer::from_derive_input(&parse_quote! {
#[v(first = "Hello", last = "World", parent(first = "Hi", last = "Earth", blasts = "off"))]
struct Demo;
})
.map(|_| "Should have failed")
.unwrap_err()
.to_string();

assert!(
!errors.contains("`blast`"),
"Should not contain `blast`: {}",
errors
);
}

#[test]
fn suggest_valid_parent_alts() {
let errors = Outer::from_derive_input(&parse_quote! {
#[v(first = "Hello", bladt = false, last = "World", parent(first = "Hi", last = "Earth"))]
struct Demo;
})
.map(|_| "Should have failed")
.unwrap_err()
.to_string();
assert!(
errors.contains("`blast`"),
"Should contain `blast` as did-you-mean suggestion: {}",
errors
);
}

0 comments on commit e782166

Please sign in to comment.