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.

Sign up for GitHub

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

Jump to bottom

WIP: adds better namedtuple semantic analyzer #11206

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

WIP: adds better namedtuple semantic analyzer #11206

wants to merge 13 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 27, 2021 •
edited

This is almost a complete rewrite of inline NamedTuple and namedtuple semantic analyzer.

Changelog:

  1. NamedTuple now supports **kwargs as it should
  2. NamedTuple and namedtuple now have better validation for fields: keywords, underscores, duplication: basically the same as collections.namedtuple does at runtime
  3. namedtuple now supports rename argument (it actually renames invalid fields)
  4. We now also check NamedTuple() and namedtuple() calls during typechecking, this allows us to simplify the setup
  5. Namedtuples no longer think that bytes are valid arguments on python3
  6. NamedTuple and namedtuple now support named arguments as they should
  7. I've moved all errors to a single place, now semanal.py does not raise any namedtuple-expr-related errors
  8. More consistent errors: I've copied texts from runtime version of collections.namedtuple, added "" everywhere

This is still WIP, but I've wrote several tests for this in testsemanal.py. They are passing. And self check also passes.

Снимок экрана 2021-09-27 в 18 02 24

I can continue to work on this after #11162 is merged or rejected. Because I need better typing fixtures for NamedTuple type.

Closes #11047

@sobolevn sobolevn marked this pull request as draft September 27, 2021 15:12
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@sobolevn
Copy link
Member Author

This PR would also help us a lot: python/typeshed#6080

Since we now typecheck NamedTupleExpr.call, together with that PR it can detect cases, when we use invalid kwargs and fields combination.

sobolevn added a commit to sobolevn/optuna that referenced this pull request Sep 27, 2021
@sobolevn
Fixes namedtuple type name
f0eeade 
The best practice is to match `typename` with variable name.
For example, `mypy` reports any other usages as errors: python/mypy#11206 (comment)
@sobolevn sobolevn mentioned this pull request Sep 27, 2021
Merged
sobolevn added a commit to sobolevn/paasta that referenced this pull request Sep 27, 2021
@sobolevn
Fixes namedtuple type name
762b52b 
The best practice is to match `typename` with variable name.
For example, `mypy` reports any other usages as errors: python/mypy#11206 (comment)
@sobolevn sobolevn mentioned this pull request Sep 27, 2021
Merged
@github-actions

This comment has been minimized.

Sign in to view
@sobolevn sobolevn mentioned this pull request Sep 28, 2021
Merged
nemacysts pushed a commit to Yelp/paasta that referenced this pull request Sep 29, 2021
@sobolevn
Fixes namedtuple type name (#3217)
d5a3f9a 
The best practice is to match `typename` with variable name.
For example, `mypy` reports any other usages as errors: python/mypy#11206 (comment)
This was referenced Oct 9, 2021
Merged
Closed
sobolevn added a commit to sobolevn/manticore that referenced this pull request Nov 7, 2021
@sobolevn
Improves namedtuple definition
5a5ea64 
I am working on new `mypy` feature and it identified a problem with your `namedtuple` definitions.
python/mypy#11206 (comment)

By standard first string arg should match variable name.
@sobolevn sobolevn mentioned this pull request Nov 7, 2021
Merged
@sobolevn
Copy link
Member Author

sobolevn commented Nov 7, 2021

I finally have the time to continue my work!

All PRs that I needed: python/typeshed#6080 / #11303 / #11162 are merged!

@github-actions

This comment has been minimized.

Sign in to view
@sobolevn
Copy link
Member Author

sobolevn commented Nov 7, 2021

All check-namedtuple.test cases are passing. Still a lot to fix though.
Снимок экрана 2021-11-07 в 13 37 57

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
ekilmer pushed a commit to trailofbits/manticore that referenced this pull request Nov 8, 2021
@sobolevn
Improves namedtuple definition (#2501)
6e036f3 
I am working on new `mypy` feature and it identified a problem with your `namedtuple` definitions.
python/mypy#11206 (comment)

By standard first string arg should match variable name.
@sobolevn sobolevn mentioned this pull request Nov 18, 2021
Closed
@github-actions

This comment has been minimized.

Sign in to view
6 similar comments
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@sobolevn sobolevn mentioned this pull request Dec 4, 2021
Closed
@github-actions

This comment has been minimized.

Sign in to view
3 similar comments
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
sobolevn added a commit to sobolevn/manticore that referenced this pull request Dec 4, 2021
@sobolevn
Improves namedtuple definition
7e657a1 
While working on python/mypy#11206 I found that `Regspec` definition is not ideal.
It is recommended to use the same string name, as variable name. 
For example, it affects how `pickle` works.

Related trailofbits#2501
Related trailofbits@6e036f3
@sobolevn sobolevn mentioned this pull request Dec 4, 2021
Merged
ekilmer pushed a commit to trailofbits/manticore that referenced this pull request Dec 5, 2021
@sobolevn
Improve namedtuple definition (#2506)
285ab50 
While working on python/mypy#11206 I found that `Regspec` definition is not ideal.
It is recommended to use the same string name, as variable name. 
For example, it affects how `pickle` works.

Related #2501
Related 6e036f3
@97littleleaf11
Copy link
Collaborator

You might use WORD_SIZE*1 and WORD_SIZE*2 to fix the failure on win32

@sobolevn
Copy link
Member Author

sobolevn commented Dec 7, 2021

@97littleleaf11 thank you! Can you please be more specific? 🙂
I don't understand your idea.

@97littleleaf11
Copy link
Collaborator

@sobolevn You can just replace +4 with + WORD_SIZE*1 and replace +8 with + WORD_SIZE*2 in testTypeAlias_toplevel.

@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/frame.py:1402: error: Unused "type: ignore" comment
+ pandas/core/frame.py:1403: error: "namedtuple()" expects a string literal as the typename argument  [misc]

@sobolevn
Copy link
Member Author

sobolevn commented Dec 7, 2021

Awesome, thank you, @97littleleaf11
It now works.

I will add more tests for .call type checking. And this will be ready to be reviewed.

@hauntsaninja hauntsaninja reopened this Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NamedTuple single line declaration gives error "Too many arguments for NamedTuple() [misc]
3 participants
@sobolevn @97littleleaf11 @hauntsaninja