Handle python 3.6 f-strings without error #2622

achauve · 2016-12-30T17:16:13Z

Linked to #2265

gvanrossum · 2016-12-30T17:33:55Z

mypy/fastparse.py

@@ -769,6 +769,11 @@ def visit_Str(self, n: ast35.Str) -> Union[UnicodeExpr, StrExpr]:
        else:
            return UnicodeExpr(n.s)

+    # JoinedStr(expr* values)


This whole method should be inside something like if hasattr(ast35, 'JoinedStr'): so that mypy will still work with the previous version of typed_ast.

gvanrossum · 2016-12-30T17:40:39Z

test-data/unit/check-expressions.test

+
+
+[case testFStringParseOk]
+a = f'foobar'


You need # flags: --fast-parser here (see how other tests use it) else you get a SyntaxError.

Can you also add tests showing that the expressions in {} are actually type-checked?

PS. To quickly run just this test, try: pytest -n0 -k testFStringParseOk

Thanks for the tip, better than python runtests.py testcheck that I was using.

I've just added another test to check type checking of expressions but I can't get it working.
If I put the content in a test_f_string.py file and run mypy --fast-parser --python-version 3.6 --show-traceback test_f_strings.py it runs without any error.

However running the test with pytest -n0 -k testFStringParseOk gives me:

___________________________________________________ testFStringTypecheckExpression ____________________________________________________ data: /Users/achauve/dev/github.com/achauve/mypy/test-data/unit/check-expressions.test:1169: ../../mypy/test/testcheck.py:113: in run_case self.run_case_once(testcase) ../../mypy/test/testcheck.py:190: in run_case_once assert_string_arrays_equal(output, a, msg.format(testcase.file, testcase.line)) ../../mypy/test/helpers.py:85: in assert_string_arrays_equal raise AssertionFailure(msg) E mypy.myunit.AssertionFailure: Invalid type checker output (/Users/achauve/dev/github.com/achauve/mypy/test-data/unit/check-expressions.test, line 1169) -------------------------------------------------------- Captured stderr call --------------------------------------------------------- Expected: Actual: main:3: error: Variable annotation syntax is only suppoted in Python 3.6, use type comment instead (diff)

It doesn't use python 3.6 syntax. How can I specify it in the tests?

gvanrossum · 2016-12-30T18:07:22Z

test-data/unit/check-expressions.test

+
+[case testFStringTypecheckExpression]
+# flags: --fast-parser
+# flags: --python-version 3.6


You should combine the flags like so:

# flags: --fast-parser --python-version 3.6

gvanrossum · 2016-12-30T18:08:59Z

test-data/unit/check-expressions.test

+a = 'foo'
+b: str
+b = f'{a}bar'
+b == 'foobar'


I get Unsupported left operand type for == ("str") here, which I think is a red herring. Did you mean = instead of ==?

No I meant ==.
I think I'm missing something here. Isn't it supposed to pass?

Oh, hm. It's supposed to pass in real life, but in the tests we're using a set of minimal stubs (see lib-stub/* and fixtures/*, and the [builtins ...] directove). I would just use a different test that doesn't use == -- it's not like the tests run this code, it's only type-checked.

gvanrossum · 2016-12-30T18:09:46Z

test-data/unit/check-expressions.test

+a: str
+a = 'foo'
+b: str
+b = f'{a}bar'


I'd also try something with a type error inside the {}, e.g.

a = 1 b = '' f'{a + b}'

Note that plain a + b gives an error, so f'{a + b}' should also gives an error.

Yes absolutely I was in the process of adding such a test, it's working minus the variation in error output (diff)

gvanrossum · 2016-12-30T20:55:03Z

test-data/unit/check-expressions.test

+a = 'foo'
+b: str
+b = f'{a}bar'
+b == 'foobar'


Oh, hm. It's supposed to pass in real life, but in the tests we're using a set of minimal stubs (see lib-stub/* and fixtures/*, and the [builtins ...] directove). I would just use a different test that doesn't use == -- it's not like the tests run this code, it's only type-checked.

gvanrossum · 2016-12-30T20:55:16Z

test-data/unit/check-expressions.test

+f'{1 + "a"}'
+f'{1 + 1}'
+[out]
+main:7: error: Unsupported operand types for + ("int" and "str") (diff)


You don't need the (diff) bit at the end.

achauve · 2016-12-30T21:15:00Z

I've cleanup the tests and removed the ==. The method visit_JoinedStr was too simplistic and is fixed now (f'{1 + ""}' was not testing visit_JoinedStr but only visit_FormattedValue).

-> should we use a more complex solution by building an ast expression of `str(...)` to get rid of the casts?

gvanrossum · 2016-12-30T22:35:54Z

mypy/fastparse.py

@@ -776,7 +776,8 @@ def visit_Str(self, n: ast35.Str) -> Union[UnicodeExpr, StrExpr]:
        def visit_JoinedStr(self, n: ast35.JoinedStr) -> StrExpr:
            result_string_expression = StrExpr('')
            for value in n.values:
-                result_string_expression = OpExpr('+', result_string_expression, self.visit(value))
+                value_as_string_expr = cast(StrExpr, self.visit(value))
+                result_string_expression = cast(StrExpr, OpExpr('+', result_string_expression, value_as_string_expr))


I wonder if you could use asserts instead of casts, e.g.

value_as_string_expr = self.visit(value) assert isinstance(value_as_string_expr, StrExpr) r = OpExpr('+', result_string_expression, value_as_string_expr) assert isinstance(r, StrExpr) result_string_expression = r

That way if the type is ever not what you expect it'll fail right there rather than mysteriously failing at some later point.

I used cast because otherwise I thought it would systematically fail without a call to str(...). Am I missing something?

I thought another typesafe solution would be to replace casts by something like (untested) CallExpr(NameExpr('str'), OpExpr('+', result_string_expression, value_as_string_expr))? but it's rather complex compared to untyped casts, and at the end every expression in a f-string is indeed passed to a str() call?

gvanrossum · 2016-12-30T22:38:30Z

mypy/fastparse.py

+    if hasattr(ast35, 'JoinedStr'):
+        # JoinedStr(expr* values)
+        @with_line
+        def visit_JoinedStr(self, n: ast35.JoinedStr) -> StrExpr:


You're also going to have to add JoinedStr and FormattedValue to the stubs for typed_ast.ast35 in typeshed.

Yes thanks for the tip

Done python/typeshed#803

gvanrossum · 2016-12-30T23:03:59Z

Hm, my solution doesn't work either (but the traceback it gives shows that your cast() version is wrong). I've gotta run, but you should look at other places that do similar things.

achauve · 2016-12-31T15:42:16Z

Thanks for the review.

I'm not sure what you mean by "your cast() version is wrong". True it is not typesafe and it is just here to fool mypy, but is it so far from what happens in real life when you construct a f-string from expressions? In f'{expr1}{expr2}', expr1 and expr2 are any kind of expressions and they are transformed into strings before being concatenated?
I can't see a typesafe solution for this right now, even after looking around in mypy code. Would you have more indications on this?

gvanrossum · 2016-12-31T18:01:49Z

There seems to be confusion here between the types of the expressions being analyzed and the type of the mypy variables used to do the type checking.

The failing assert (when running mypy) proves that the cast (which is happening in mypy) is wrong because the type of the mypy expression being cast is not what the cast says it should be.

I tried something like this to prove it:

a = ''
b = f'.{a}.'

and the second assert in my version failed because r is an OpExpr, not an StrExpr.

You can probably fix this by using Expression instead of StrExpr in a few places (like the return value of visit_JoinedStr() and result_string_expression). You shouldn't need casts at all then.

The other thing is that you have to somehow insert a node around each expression inside {...} that converts it to a string. My first thought would be to somehow insert a call to str(). The problem with this is that the later stages will break if there's a variable named str in scope that is not the built-in str() type. Perhaps a more suitable call would be a method call to .__str__().

We would then effectively get a translation from e.g.

b = f"foo {a + 1} bar"

to

b = "foo " + (a + 1).__str__() + " bar"

A remaining issue to solve is what to do with the optional formatting after the value, e.g. (from PEP 498):

f'my anniversary is {anniversary:%A, %B %d, %Y}.'

But that can be done in a separate PR -- it's similar in scope and complexity as making sure that % formatting codes are used correctly (and that one is still a work in progress).

Finally: the most correct method call to insert would actually be .__format__(...) but then you'd also have to add __format__() to class object in builtins.pyi in typeshed.

achauve · 2016-12-31T19:44:37Z

Indeed there was some confusion on my part. I'll try to be more specific and answer your different points.

1 - Casts vs asserts vs loosing up the return type of method visit_JoinedStr to Expression.
--> I was confused about the required type of visit_JoinedStr. Using Expression seems to be the best choice as any type of expression can actually be used inside f-strings? I'll fix it.

2 - Inserting a call to str(), .__str__() or .__format__() around expressions before concatenating them using OpExpr('+', ...).
--> It seems to be required only for point 3 and is not an issue in itself. I'm not sure about how calling .__format__(). The simple case using str() seems to be something like:
CallExpr(NameExpr('str'), [value_expr], [ARG_POS])?

3 - Handling (ie typechecking) optional formatting params of expressions inside f-strings.
--> Yes, another PR seems quite good ;-).

gvanrossum · 2016-12-31T21:01:33Z

No, you need the call even without (3). Consider this: f".{1}." -- your current code would generate the equivalent of "." + 1 + "." which produces a type error -- try for yourself, you should add a unit test like this.

achauve · 2017-01-01T16:46:57Z

Exact, I'll add the missing test case. The version of the code using the casts was working though ;-).

I've tried to use:

CallExpr(MemberExpr(value_expr, '__str__'), [], [])

and it's working fine when I run mypy in the command line on a test file (using the same test cases as in the unit tests). But when I run the unit test with pytest -n0 -k testFString I get the following errors:

main: error: "str" has no attribute "__str__" 
main: error: "int" has no attribute "__str__"

and I don't know why. It seems like during check-*.test unit tests the symbol table of builtin objects is not fully loaded from typeshed... but if I copy the tests in regression tests file pythoneval.test, there is no error.
What are the best practises with these tests, I'm a little lost here?

gvanrossum · 2017-01-02T04:07:13Z

So to give another hint, if you add def __str__(self) -> str: pass to class object in test-data/unit/fixtures/primitives.pyi and add [builtins fixtures/primitives.pyi] to your tests, the first test fails with a single error:

  main: error: Unsupported left operand type for + ("str") (diff)

Note that there's no line number! You have to pass the line number along to your calls constructing new nodes (there are some other examples for that in the same file).

achauve · 2017-01-05T10:41:12Z

@kirbyfan64 Thanks for the tip, I'll use my forks in the meantime ;-)

achauve · 2017-01-31T19:29:24Z

@gvanrossum any news on typed_ast and when this PR could be merged? Thx!

gvanrossum · 2017-01-31T20:51:25Z

@ddfisher can you fill us in?

AngeloSalvade · 2017-02-10T06:50:03Z

Could you please explain what BLOCKED means.
Will f-strings handling not be implemented in mypy?
Or is it delayed? If so, could you estimate for how long?

michael-k · 2017-02-10T10:18:56Z

As always blocked means delayed. See also description of PR:

Wait for new release of typed_ast with python/typed_ast#22 merged

for how long?

See python/typed_ast#29 (comment)

ddfisher · 2017-02-13T22:10:25Z

This should be unblocked now!

achauve · 2017-02-14T08:51:01Z

@ddfisher Thanks! :)
@gvanrossum I'm not sure about the next step here since it's been a long time. What do you prefer: a rebase on master? a merge from master?

gvanrossum · 2017-02-14T18:39:16Z

@achauve I don't have a preference between merging or rebasing from master; we squash commits when we merge into master anyways. Sometimes for an old PR, merging is easier than repeatedly solving the rebase conflicts. Once your tests passing I'll review again!

…ithout-error

…=3.6)

achauve · 2017-02-16T18:24:37Z

The tests are failing on python < 3.6 and I don't know why for now (f-string tests pass when using pytest pytest -n0 -k testNewSyntaxFstring on python 3.5.1 for instance but fail when launched from python runtests.py -x lint).

ilevkivskyi · 2017-02-16T23:56:45Z

@achauve
The problem could be related to the segfault I discovered in typed_ast, see python/typed_ast#30, essentially segfault could happen on all non-trivial f-strings.

I made a PR python/typed_ast#32 to fix this.
@ddfisher could you please take a look?

achauve · 2017-02-17T08:16:10Z

@ilevkivskyi ok thanks!

ddfisher · 2017-02-17T11:40:48Z

Merged the PR -- releasing typed-ast 1.0.1 now.

ddfisher · 2017-02-17T11:51:11Z

typed-ast 1.0.1 released. Thanks for the fix @ilevkivskyi!

ilevkivskyi · 2017-02-17T12:24:33Z

@ddfisher Thank you for fast reaction! I have tried this branch and now all tests pass on 3.4 and 3.5.1

@achauve I have found that in your PR s = f'Hi {s}' fails with:

error: Cannot determine type of 's'

The error looks strange to me, f-string literals cannot produce anything but str. However, maybe this is a trace of known problem: mypy does not check for variables used before definition, see e.g. #2400. I would therefore propose to fix this in a separate PR.

achauve · 2017-02-17T12:53:47Z

@ilevkivskyi Thanks for the fix!

For your test case, is s not defined before?

ilevkivskyi · 2017-02-17T12:55:14Z

For your test case, is s not defined before?

Yes, exactly.

achauve · 2017-02-17T13:12:09Z

As mypy typechecks the expression inside the f-string, isn't it sound that there is an error?
But an error saying that s is used before assignment would be clearer.

achauve · 2017-02-17T13:12:59Z

@gvanrossum Thanks to @ilevkivskyi the PR is now all green!

ilevkivskyi · 2017-02-17T17:50:22Z

@achauve

As mypy typechecks the expression inside the f-string, isn't it sound that there is an error?
But an error saying that s is used before assignment would be clearer.

If you give it a type, s: str = f'Hi {s}' then there will be no error at all, but as I already mentioned, it is not related to this PR.

achauve · 2017-02-17T18:01:26Z

ok

Handle python 3.6 f-strings without error

fc03182

gvanrossum reviewed Dec 30, 2016

View reviewed changes

Fix to support older versions of typed_ast

31e0252

gvanrossum reviewed Dec 30, 2016

View reviewed changes

Fix f-string test

6c4aa78

gvanrossum requested changes Dec 30, 2016

View reviewed changes

Add new f-string test to check expression type checking

2112398

achauve force-pushed the handle-f-string-without-error branch from abdc281 to 2112398 Compare December 30, 2016 18:12

Fix test flags

025c948

achauve force-pushed the handle-f-string-without-error branch from f5714c6 to 025c948 Compare December 30, 2016 19:01

gvanrossum requested changes Dec 30, 2016

View reviewed changes

Split f-string tests and fix ast visit of JoinedStr

e8cb282

Use casts to fix typechecking of visitor method

d6ba5fd

-> should we use a more complex solution by building an ast expression of `str(...)` to get rid of the casts?

gvanrossum reviewed Dec 30, 2016

View reviewed changes

gvanrossum approved these changes Dec 30, 2016

View reviewed changes

achauve mentioned this pull request Dec 31, 2016

Add FormattedValue and JoinedStr to typed_ast ast35 python/typeshed#803

Closed

Remove casts and fix return type of visit_JoinedStr

25043d4

Adrien Chauve added 2 commits January 1, 2017 17:50

Add new f-string test case

8f17a4c

Add call to __str__() before concatenating strings in visit_JoinedStr

4847e27

achauve force-pushed the handle-f-string-without-error branch from ba097e0 to 4847e27 Compare January 1, 2017 16:51

Fix tests by adding missing primitives

5534c1a

gvanrossum mentioned this pull request Jan 11, 2017

F-strings!!! python/typed_ast#22

Closed

gvanrossum added the blocked label Feb 6, 2017

ddfisher mentioned this pull request Feb 13, 2017

Update to typed-ast 1.0.0 #2857

Merged

ddfisher removed the blocked label Feb 13, 2017

Adrien Chauve added 3 commits February 15, 2017 09:28

Merge remote-tracking branch 'upstream/master' into handle-f-string-w…

2c73695

…ithout-error

fix: Rename asst35 to ast3

47d592a

fix: Move f-string tests to check-newsyntax.test (executed only for >…

0867026

…=3.6)

Minor cleanings

8a17249

JukkaL merged commit fe1056b into python:master Feb 21, 2017

filipgolem mentioned this pull request Sep 21, 2021

fix blender.py example on python3.6 golemfactory/yapapi#667

Merged

Handle python 3.6 f-strings without error #2622

Handle python 3.6 f-strings without error #2622

Conversation

achauve commented Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achauve Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achauve commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achauve Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

gvanrossum commented Dec 30, 2016

achauve commented Dec 31, 2016

gvanrossum commented Dec 31, 2016

achauve commented Dec 31, 2016 • edited Loading

gvanrossum commented Dec 31, 2016

achauve commented Jan 1, 2017 • edited Loading

gvanrossum commented Jan 2, 2017

achauve commented Jan 5, 2017

achauve commented Jan 31, 2017

gvanrossum commented Jan 31, 2017

AngeloSalvade commented Feb 10, 2017

michael-k commented Feb 10, 2017 • edited Loading

ddfisher commented Feb 13, 2017

achauve commented Feb 14, 2017

gvanrossum commented Feb 14, 2017

achauve commented Feb 16, 2017

ilevkivskyi commented Feb 16, 2017

achauve commented Feb 17, 2017

ddfisher commented Feb 17, 2017

ddfisher commented Feb 17, 2017 • edited Loading

ilevkivskyi commented Feb 17, 2017 • edited Loading

achauve commented Feb 17, 2017

ilevkivskyi commented Feb 17, 2017

achauve commented Feb 17, 2017

achauve commented Feb 17, 2017

ilevkivskyi commented Feb 17, 2017

achauve commented Feb 17, 2017

achauve commented Dec 30, 2016 •

edited

Loading

achauve Dec 30, 2016 •

edited

Loading

achauve Dec 31, 2016 •

edited

Loading

achauve commented Dec 31, 2016 •

edited

Loading

achauve commented Jan 1, 2017 •

edited

Loading

michael-k commented Feb 10, 2017 •

edited

Loading

ddfisher commented Feb 17, 2017 •

edited

Loading

ilevkivskyi commented Feb 17, 2017 •

edited

Loading