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

handle Union type in string interpolation checker #7763 #7809

Merged
merged 2 commits into from Oct 29, 2019

Conversation

@vbarbaresi
Copy link
Contributor

vbarbaresi commented Oct 28, 2019

The problematic case was

t: Union[Tuple[str, int], Tuple[int, str]] = ('A', 1)
"%s %s" % t

The Union was treated as a single value instead of looking at the types inside
The fix consists in checking each of the union types

Two questions:

  • I created a TempNode for each of the sub types and added the current line to keep some context, not sure it's the right way to do it.
  • I'm running the check for all of the types in the union, but I've seen in the code base that it's usually checked for any().
    I suppose it shouldn't return an error if one of the types matches the formatting arguments, for instance with Union[Tuple[str, int], Tuple[str, str, str]]
    If you confirm, I will have to refactor part of the check in a sub-method
The problematic case was

```
t: Union[Tuple[str, int], Tuple[int, str]] = ('A', 1)
"%s %s" % t
```

The Union was treated as a single value instead of looking at the types inside

The fix consists in checking each of the union types
Copy link
Collaborator

JukkaL left a comment

Thanks for the PR! Looks good.

Answers to questions:

  • Using TempNode is a reasonable approach.
  • Operations on union types must be valid for all union items.

b: Union[Tuple[int, str], Tuple[int, int], Tuple[str, int]] = ('A', 1)
'%s %s' % b
'%s %s %s' % b # E: Not enough arguments for format string

This comment has been minimized.

Copy link
@JukkaL

JukkaL Oct 29, 2019

Collaborator

Add test case where one union item is valid and another is invalid. It should be an error if any item is invalid.

This comment has been minimized.

Copy link
@vbarbaresi

vbarbaresi Oct 29, 2019

Author Contributor

done

@JukkaL
JukkaL approved these changes Oct 29, 2019
Copy link
Collaborator

JukkaL left a comment

Thanks for the quick response!

@JukkaL JukkaL merged commit f0df1d6 into python:master Oct 29, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.