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

Add str literal mutation #189

Merged
merged 6 commits into from
Jul 29, 2022
Merged

Add str literal mutation #189

merged 6 commits into from
Jul 29, 2022

Conversation

samgoldman
Copy link
Contributor

If you're open to new mutations, I created a mutator for string literals. It replaces empty strings with a single character string and for non-empty strings it does a few things: append and prepend a single character as well as replacing the string with an empty string. I'd welcome any feedback. Thanks!

@samuelpilz
Copy link
Contributor

This seems like a really good implementation of a new mutation, thank you. Documentation and tests are well done.
The only thing that concerns me is the owned String as return value from the run function. Generally, changing the type of an expression is tricky, it messes with type-inference and could result in different trait resolutions.

One potential issue in particular: Does this implementation break code like this (because the owned String is only a temp variable)?

fn a() -> usize {let x = ""; x.len()}

I suggest using a return value of &'static str. This requires baking the possible mutation literals into the source code.

@samgoldman
Copy link
Contributor Author

Thanks for the feedback, that makes sense. I made the update, although I had to remove the appending and prepending mutations. I also added a test case for the example you provided and it doesn't break it (and it didn't break before changing to &'static str either).

@samuelpilz
Copy link
Contributor

samuelpilz commented Jul 24, 2022

Indeed, for some reason {let x = &String::new(); x.len()} is valid Rust now (for some reason, the lifetime of the tempvar gets extended). I thought this was a minimal example of lifetime extension. Probably a better example is:

fn a() -> &'static str {""}

This code should not break as well (which is now fixed).

I really liked the string-dependent mutations. My idea is changing the signature of run roughly to:

fn run(
    mutator_id: usize,
    original_lit: &'static str,
    mutations: &[&'statc str],
    runtime: &impl Deref<Target = MutagenRuntimeConfig>,
)  -> &'static str {...}

This allows you to create the mutated strings at compile-time and have them available at run-time as &'static str

@samgoldman
Copy link
Contributor Author

Yeah, the new example would have broken the original version. I updated to the signature you suggested and that example works (and I've added it as a test case).

@samuelpilz
Copy link
Contributor

Great. One last comment: could you revert the documentation to your original proposal?
Otherwise, this is ready for merge (although I don't have the rights)

@samgoldman
Copy link
Contributor Author

I forgot to leave a comment but that should be done.

@llogiq
Copy link
Owner

llogiq commented Jul 29, 2022

Thank you! I didn't have the time to look at this until now, but Samuel has already given all the feedback needed.

Thanks to both of you!

@llogiq llogiq merged commit a6377c4 into llogiq:master Jul 29, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants