Skip to content

Remove open plugin, not needed with overloaded signatures on typeshed - #7794

Closed
ilai-deutel wants to merge 10 commits into
python:masterfrom
ilai-deutel:typeshed-overloaded-open
Closed

Remove open plugin, not needed with overloaded signatures on typeshed#7794
ilai-deutel wants to merge 10 commits into
python:masterfrom
ilai-deutel:typeshed-overloaded-open

Conversation

@ilai-deutel

@ilai-deutelilai-deutel commented Oct 25, 2019

Copy link
Copy Markdown
Contributor

python/typeshed#3371 adds overloaded signatures for open(), Path.open(), etc.
Consequently:

  • Plugins that analyze the signatures of these functions can be removed
  • Tests have to be changed. For instance, reveal_type(open()) now produces error: All overload variants of "open" require at least one argument instead of Too few arguments for "open".

This pull request should probably not be merged before python/typeshed#3371. However, we have a circular dependency: mypy_selftest in the typeshed PR fails because of the issues with mypy tests mentioned above. We can use CI on this PR to make sure the tests will pass on typeshed after merging both PRs.

After python/typeshed#3371 is merged, the typeshed submodule will need to be updated.

Comment threadtest-data/unit/pythoneval.test Outdated
[out]
_program.py:3: note: Revealed type is 'typing.BinaryIO'
_program.py:4: note: Revealed type is 'typing.BinaryIO'
_program.py:3: note: Revealed type is 'typing.IO[Any]'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem like regressions. Why can mypy no longer infer BinaryIO here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p.open(mode='rb', errors='replace') raises an exception (ValueError: binary mode doesn't take an errors argument), so its type is not really BinaryIO.

mypy infers it is a IO[Any] because of the fallback overload with mode: str, but ideally open should have a return type of NoReturn in this case. I wonder if adding an overload variant with a return type of NoReturn would solve the problem.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoReturn is not really intended for error cases. Falling back to the default return type on error case seems fine to me.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, when you say "default return type", do you mean IO[Any] or BinaryIO?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this it seems to me it is better to improve this plugin instead of removing it. For example, the p.open(mode='rb', errors='replace') call will silently fall back to the default overload, but with the plugin we can detect the error and give a nice error message (matching the runtime one).

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the plugin to detect use of text-only arguments in binary mode.

@ilevkivskyi

Copy link
Copy Markdown
Member

The CI failure is caused by python/typeshed#3446.

@ilai-deutel

Copy link
Copy Markdown
ContributorAuthor

Fixed upstream, AppVeyor tests now pass

@JukkaLJukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is close to ready to merge, but there is still an issue with the Python 2 signature of open().

Comment threadmypy/plugins/default.py Outdated
Comment threadmypy/plugins/default.py Outdated
Comment threadtest-data/unit/python2eval.test
@ilevkivskyi

Copy link
Copy Markdown
Member

Since Jukka made previous reviews, I think it would make sense if he will review.

@ilevkivskyi
ilevkivskyi removed their request for review April 18, 2020 20:15
@ilai-deutel

Copy link
Copy Markdown
ContributorAuthor

Hi @JukkaL, is there anything else I need to do before we can merge this PR?

@msullivanmsullivan mentioned this pull request Oct 18, 2020
@msullivan

Copy link
Copy Markdown
Collaborator

I'm sorry about this, but I'm going to close this in favor of #9275. (Which I'm also not sure if it is going to get merged)

Sign up for freeto 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.

5 participants

@ilai-deutel@ilevkivskyi@msullivan@JelleZijlstra@JukkaL