Allow generator expressions in f-strings

[thatch]

Fixes #388

Summary

I'm kind of dogsciencing this, since there doesn't appear to be a formal grammar for f-strings. I tested a few other languages features and am pretty confident the only major feature that works in 3.8 that we didn't have was bare generator expressions.

Test Plan

Includes new test, also tested on John's longer example.

[thatch]

I don't think we have great coverage for what's allowed in f-strings. Assuming hypothesmith does not help here, any ideas for getting more variety?

[jimmylai]

I don't think we have great coverage for what's allowed in f-strings. Assuming hypothesmith does not help here, any ideas for getting more variety?

@thatch , out large scale codebase is a good dataset to test f-string variety, I believe that's how @zsol identified #388 and #387 f-string issues. Whenever we find new cases not supported, we should include the found cases as test cases.

[thatch]

large scale codebase is a good dataset

Yes, but written by humans it tends to not include a lot of edge cases.

I wrote up a quick script that just tries random tokens; it identified two issues in under a minute. One is generator expressions, the other is f'{* A}' which is invalid if you try to run it, but allowed by ast.parse. With this change, we match the ast.parse behavior for both (run for more than an hour).

(I did try Hypothesmith, but it wasn't generating any expressions with spaces in them even with bumping deadline and max_examples, so I switched to just doing the straightforward thing.)

Are you comfortable approving based off that testing?

[zsol]

Are you comfortable approving based off that testing?

I am :) but I won't have time to look through the changes today. ETA Monday

[jimmylai]

LGTM

[jimmylai]

I wrote up a quick script that just tries random tokens; it identified two issues in under a minute. One is generator expressions, the other is f'{* A}' which is invalid if you try to run it, but allowed by ast.parse. With this change, we match the ast.parse behavior for both (run for more than an hour).

(I did try Hypothesmith, but it wasn't generating any expressions with spaces in them even with bumping deadline and max_examples, so I switched to just doing the straightforward thing.)

@Zac-HD do you know if generating expression in f-string is supported in Hypothesmith?

[Zac-HD]

@Zac-HD do you know if generating expression in f-string is supported in Hypothesmith?

from_node() should generate some via libcst.FormattedString, but you're very unlikely to see any generated by from_grammar(). To test f-strings specifically, I'd use from_node(node=libcst.FormattedString) or from_node(node=libcst.BaseExpression).map("f'{{{}}}'".format).

More generally Hypothesmith does still deserve the 0.1.x version number - it works, and it does find bugs, but it's noticably less mature than Hypothesis itself. I have a substantial research project planned to improve it next year though 😁

[thatch]

@Zac-HD I trivially modified the expr example, and it looks like it never generated an expression with any whitespace. This is before the except ParserSyntaxError block. I'm not really expecting user support on a PR, but if there's something obvious (like the .map(str.strip) which in hindsight is suspicious) I'd like to include a test that works.

https://gist.github.com/thatch/2b0b13e50eb181f0fe0459ead810f518
https://gist.github.com/thatch/36fbfadd8d2ecbfa36940350d9c49eb0

[Zac-HD]

.map(str.strip) will remove any leading or trailing whitespace; internal whitespace for grammars is... harder than it sounds. I'd expect to generate some at least, but could be surprised. HypothesisWorks/hypothesis#2437 outlines some ideas about how to force whitespace when it's required but ignored, which is currently a little iffy.

I'd recommend using the from_node() strategy instead of from_grammar() for this, as above - it has much better support for generating complex inputs, and a wider range of options for starting nodes.