Skip to content
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

Automatically parse strings into structs that implement FromStr #111

Closed
noeddl opened this issue Mar 18, 2021 · 7 comments
Closed

Automatically parse strings into structs that implement FromStr #111

noeddl opened this issue Mar 18, 2021 · 7 comments

Comments

@noeddl
Copy link

noeddl commented Mar 18, 2021

In parametrized tests involving structs that implement FromStr it would be cool if strings given in a case could automatically be parsed into the corresponding type (similar to the way arguments are magically converted in structopt).

For example, one of my tests looks something like this:

#[rstest(chord, root, third, fifth,
    case("C", "C", "E", "G"),
    case("C#", "C#", "F", "G#"),
    case("Db", "Db", "F", "Ab"),
)]
fn test_chord_notes(chord: &str, root: &str, third: &str, fifth: &str) {
    let c = Chord::from_str(chord).unwrap();
    let r = Note::from_str(root).unwrap();
    let t = Note::from_str(third).unwrap();
    let f = Note::from_str(fifth).unwrap();
    let notes: Vec<Note> = c.notes().collect();
    assert_eq!(notes, vec![r, t, f]);
}

What I would like to be able to do is the following:

#[rstest(chord, root, third, fifth,
    case("C", "C", "E", "G"),
    case("C#", "C#", "F", "G#"),
    case("Db", "Db", "F", "Ab"),
)]
fn test_chord_notes(chord: Chord, root: Note, third: Note, fifth: Note) {
    let notes: Vec<Note> = chord.notes().collect();
    assert_eq!(notes, vec![root, third, fifth]);
}
@la10736
Copy link
Owner

la10736 commented Mar 18, 2021

Awesome!!!

I should close next release and I'll work on it.

@la10736
Copy link
Owner

la10736 commented Mar 28, 2021

Ok, I took a look to this and I need to write some questions and notes.

  • When we should apply this conversion?
    1. Just when the input is a literal string (we can catch this just by AST)
    2. Avoid any type of conversion if the the receiver type is impl or dyn because we need a concrete type to use its FromStr
  • Can we catch if the receiver is something like &str?
    1. No, we cannot relay on this kind of information. The type can be aliased by type aliasing and procedural macro cannot resolve type aliasing: we must try to relay on type system to avoid aliasing problems
  • What we would like to see if the receiver type doesn't implement FromStr trait?
    1. The best would be an error message that say that the receiver type should implement FromStr trait but the bad news is that &str doesn't implement FromStr so we cannot use something that try to use it in every case and relay on compiler message
    2. We can accept that is the receiver T doesn't implement FromStr the error message say that we try to assign &str to T

I come back later with a sketch of how to perform conversion.

@la10736
Copy link
Owner

la10736 commented Mar 28, 2021

Ok, I found a way to implement it based on https://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.html

In our case what change is the receiver, but use PhantomData to mark our wrapper is enough to make this idea work.
Playground https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=ec4918ad4ba8f00c885c39678bffd704
or gist https://gist.github.com/rust-play/ec4918ad4ba8f00c885c39678bffd704

This approach is almost optimal because give also a good error description if the receiver type doesn't implement FromStr:

   |
73 | struct NotImplementFromStr;
   | --------------------------- doesn't satisfy `NotImplementFromStr: FromStr`
use std::str::FromStr;
use std::{fmt::Debug, marker::PhantomData};

struct Wrap<T>(std::marker::PhantomData<T>);

trait ViaParseDebug<'a, T> {
    fn magic_conversion(&self, input: &'a str) -> T;
}

impl<'a, T> ViaParseDebug<'a, T> for &&Wrap<T>
where
    T: FromStr,
    T::Err: Debug,
{
    fn magic_conversion(&self, input: &'a str) -> T {
        T::from_str(input).unwrap()
    }
}

trait ViaParse<'a, T> {
    fn magic_conversion(&self, input: &'a str) -> T;
}

impl<'a, T> ViaParse<'a, T> for &Wrap<T>
where
    T: FromStr,
{
    fn magic_conversion(&self, input: &'a str) -> T {
        match T::from_str(input) {
            Ok(v) => v,
            Err(_) => {
                panic!("Cannot parse '{}' to get T", input);
            }
        }
    }
}

trait ViaIdent<'a, T> {
    fn magic_conversion(&self, input: &'a str) -> T;
}

impl<'a> ViaIdent<'a, &'a str> for &&Wrap<&'a str> {
    fn magic_conversion(&self, input: &'a str) -> &'a str {
        input
    }
}

#[derive(Debug)]
struct S;
impl FromStr for S {
    type Err = ();

    fn from_str(_s: &str) -> Result<Self, Self::Err> {
        Ok(S)
    }
}

#[derive(Debug)]
struct ErrorNotImplementDebug;
struct ErrorWithoutDebug;
impl FromStr for ErrorNotImplementDebug {
    type Err = ErrorWithoutDebug;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s {
            "error" => Err(ErrorWithoutDebug),
            _ => Ok(ErrorNotImplementDebug),
        }
    }
}

#[allow(dead_code)]
struct NotImplementFromStr;

type StrTypeAlias = &'static str;

fn main() {
    let u = (&&&Wrap::<u32>(PhantomData)).magic_conversion("42");
    println!("{}", u);

    let s = (&&&Wrap::<&str>(PhantomData)).magic_conversion("hi");
    println!("{}", s);

    let str_type_alias = (&&&Wrap::<StrTypeAlias>(PhantomData)).magic_conversion("alias");
    println!("{}", str_type_alias);

    let parse_without_error =
        (&&&Wrap::<ErrorNotImplementDebug>(PhantomData)).magic_conversion("42");
    println!("{:?}", parse_without_error);

    let _parse_without_error =
        (&&&Wrap::<ErrorNotImplementDebug>(PhantomData)).magic_conversion("error");
    // let not_implement: NotImplementFromStr = (&&&Wrap::<NotImplementFromStr>(PhantomData)).magic_conversion("42");
}

@la10736
Copy link
Owner

la10736 commented Apr 5, 2021

Test list:

  • Cases ok literal str
  • Cases ok literal raw str
  • Cases not convert from byte array
  • Cases fail to convert (panic)
  • Values ok literal str
  • Values ok literal raw str
  • Value not convert from byte array
  • Values fail to convert (panic)
  • Fixture default ok literal str
  • Fixture default ok literal raw str
  • Fixture default fail to convert (panic)
  • Fixture not convert from byte array
  • That not try to convert impl and dyn types
  • Not convert to generic type
  • Where 1 generic need conversion and 1 not
  • Error message if type doesn't implement FromStr
  • Test one type conversion if implement FromStr but not Debug
  • Error message if type doesn't implement Debug and parse fail

But cannot work for fixture arguments because we need to know the receiver type in order to use the trick.

A &dyn SomeTrait variable cannot be assigned to &str literal type because str have not know size at compile time.

la10736 added a commit that referenced this issue Apr 5, 2021
la10736 added a commit that referenced this issue Apr 5, 2021
la10736 added a commit that referenced this issue Apr 5, 2021
la10736 added a commit that referenced this issue Apr 5, 2021
la10736 added a commit that referenced this issue Apr 5, 2021
if implement FromStr but not Debug
la10736 added a commit that referenced this issue Apr 5, 2021
@la10736
Copy link
Owner

la10736 commented Apr 5, 2021

We should recover follow unit tests in the new implementation

mod arg_2_fixture_should {
    use super::{assert_eq, *};

    mod call_fixture {
        use super::{assert_eq, *};

        fn test(function: &str, expected: &str) {
            let ast = function.ast();
            let arg = first_arg_ident(&ast);

            let line = arg_2_fixture(arg, &EmptyResolver);

            assert_eq!(line, expected.ast());
        }

        #[test]
        fn as_is() {
            test("fn foo(fix: String) {}", "let fix = fix::default();");
        }

        #[test]
        fn by_remove_underscore_if_any() {
            test("fn foo(_fix: String) {}", "let _fix = fix::default();");
        }

        #[test]
        fn do_not_remove_more_underscores() {
            test("fn foo(f_i_x: String) {}", "let f_i_x = f_i_x::default();");
        }

        #[test]
        fn do_not_remove_two_underscores() {
            test("fn foo(__fix: String) {}", "let __fix = __fix::default();");
        }

        #[test]
        fn without_add_mut() {
            test("fn foo(mut fix: String) {}", "let fix = fix::default();");
        }
    }

    mod call_given_fixture {
        use super::{assert_eq, *};

        fn test(function: &str, fixture: &str, call: Expr, expected: &str) {
            let ast = function.ast();
            let arg = first_arg_ident(&ast);
            let mut resolver = HashMap::new();
            resolver.insert(fixture.to_owned(), &call);

            let line = arg_2_fixture(arg, &resolver);

            assert_eq!(line, expected.ast());
        }

        #[test]
        fn as_is() {
            test(
                "fn foo(mut fix: String) {}",
                "fix",
                expr("bar()"),
                "let fix = bar();",
            );
        }

        #[test]
        fn by_remove_underscore() {
            test(
                "fn foo(_fix: String) {}",
                "fix",
                expr("bar()"),
                "let _fix = bar();",
            );
        }
    }
}

@la10736
Copy link
Owner

la10736 commented Apr 5, 2021

Next steps:

  • Refactoring (render code is a mess)
  • Recover unit test
  • Add more unit tests
  • Document new feature
  • Fill change log

@la10736
Copy link
Owner

la10736 commented Apr 25, 2021

Completed

@la10736 la10736 closed this as completed Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants