-
Notifications
You must be signed in to change notification settings - Fork 62
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.
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
Add UI test for missing argument in args_into #145
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this. Great stuff.
A few minor requests then looks good to me!
ParseError::ArgsIntoArgNotFound { func, missing_arg } => Error::new_spanned( | ||
missing_arg.clone(), | ||
format!( | ||
r#"{}: Unknown argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
format!(r#"Argument "{}" was not found in the "fn {}(..)""#, missing_arg, func.sig.ident.to_token_stream().to_string());
Argument "foo" was not found in "fn some_function(..)".
let errors = parse_errors(tokens); | ||
|
||
// Only "bar" should be missing argument. | ||
assert_eq!(errors.len(), 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also match on &errors[0]
and confirm that it is an ArgsIntoArgNotFound
error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. something like:
assert_eq!(errors.len(), 1);
match &errors[0] {
ParseError::ArgsIntoArgNotFound {
func: _,
missing_arg,
} => {
assert_eq!(missing_arg, "bar")
}
_ => panic!(),
};
} | ||
} | ||
_ => { | ||
todo!("Add a test that hits this case") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will panic if you use args_into
on a method:
#[swift_bridge::bridge]
mod ffi {
extern "Rust" {
type SomeType;
#[swift_bridge(args_into = (foo))]
fn some_function(&self);
}
}
@@ -180,7 +179,32 @@ impl<'a> ForeignModParser<'a> { | |||
.push(ParseError::ArgCopyAndRefMut { arg: arg.clone() }); | |||
} | |||
} | |||
|
|||
if let Some(ref args) = attributes.args_into { | |||
for arg in args.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
if let Some(ref args) = attributes.args_into {
let mut func_sig_args = HashSet::with_capacity(args.len());
for fn_arg in func.sig.inputs.iter() {
match fn_arg {
FnArg::Receiver(_) => {}
FnArg::Typed(pat_ty) => {
let fn_arg_name = pat_ty.pat.to_token_stream().to_string();
func_sig_args.insert(fn_arg_name);
}
}
}
for arg in args.iter() {
let arg_name = arg.to_token_stream().to_string();
if !func_sig_args.contains(&arg_name) {
self.errors.push(ParseError::ArgsIntoArgNotFound {
func: func.clone(),
missing_arg: arg.clone(),
})
}
}
}
extern "Rust" { | ||
#[swift_bridge(args_into = (arg, arg_typo))] | ||
fn some_function(arg: u8); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a second extern "Rust" block here that uses a method?
Maybe something like:
extern "Rust" {
type SomeType;
#[swift_bridge(args_into = (arg, arg_typo))]
fn some_method(&self, arg: u8);
}
Addressed your review! |
Thanks! |
This PR addresses #11 and Introduces a compile error when using missing argument in
args_into
.Here's an example: