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

Support for hasattr() checks #13544

Merged
merged 16 commits into from
Aug 29, 2022
Merged

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Aug 28, 2022

Fixes #1424
Fixes mypyc/mypyc#939

Not that I really like hasattr() but this issue comes up surprisingly often. Also it looks like we can offer a simple solution that will cover 95% of use cases using extra_attrs for instances. Essentially the logic is like this:

  • In the if branch, keep types that already has a valid attribute as is, for other inject an attribute with Any type using fallbacks.
  • In the else branch, remove types that already have a valid attribute, while keeping the rest.

@ilevkivskyi
Copy link
Member Author

It looks like there is a mypyc crash (there is an existing issue). It looks like a one line fix, so I fix it here as well.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

Hm, it looks like a lot of errors, got fixed, but also there are a lot of unreachable errors. I guess it is probably because of Any types, that give no errors on attribute access. Let me try to exclude those.

@ilevkivskyi
Copy link
Member Author

Hm, there are also some non-Any cases. I guess people do this to guard against deleted attributes. I think we should allow this.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

OK, now mypy_primer looks very good. This is ready for review now.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Looks great! Should hasattr on module object be supported?

@ilevkivskyi
Copy link
Member Author

Should hasattr on module object be supported?

Hm, I would say ideally yes. Although as how it is implemented now it will not work, I explicitly prohibit this because I have found some tests that were checking that e.g. attribute access on union of modules does not work. IIUC the problem is that it is hard to make this work consistently and reliably for modules. They are handled specially and some errors are actually generated during semantic analysis.

I can take a look again now and check if there is a simple solution, but most likely this needs to be a separate PR.

@ilevkivskyi
Copy link
Member Author

@hauntsaninja I added some basic support for modules as well. It will not support some use cases, but should work on direct references to modules. (Let's hope this will not break anything.)

Comment on lines +879 to +883
if expr not in self.types:
# Mypy thinks this is unreachable.
iterable: ProperType = AnyType(TypeOfAny.from_error)
else:
iterable = get_proper_type(self.types[expr])
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding a test for the mypyc fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, OK, I will add a test.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member

Cool feature! 👍

I guess that we can expect if getattr(some, key, default): quite soon 😉 😄

@ilevkivskyi
Copy link
Member Author

OK, I added few more tests cases that you asked about.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

🎉

@github-actions
Copy link
Contributor

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

ignite (https://github.com/pytorch/ignite)
+ ignite/engine/utils.py:8: error: Unused "type: ignore" comment
+ ignite/engine/engine.py:340: error: Unused "type: ignore" comment
+ ignite/handlers/param_scheduler.py:210: error: Unused "type: ignore" comment
+ ignite/handlers/state_param_scheduler.py:62: error: Unused "type: ignore" comment

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/pycode/ast.py:107: error: Unused "type: ignore" comment
+ sphinx/pycode/ast.py:118: error: Unused "type: ignore" comment
+ sphinx/pycode/ast.py:121: error: Unused "type: ignore" comment
+ sphinx/pycode/parser.py:355: error: Unused "type: ignore" comment
+ sphinx/pycode/parser.py:357: error: Unused "type: ignore" comment
+ sphinx/util/inspect.py:60: error: Unused "type: ignore" comment
+ sphinx/util/inspect.py:286: error: Unused "type: ignore" comment
+ sphinx/util/inspect.py:750: error: Unused "type: ignore" comment
+ sphinx/util/inspect.py:760: error: Unused "type: ignore" comment
+ sphinx/environment/adapters/toctree.py:241: error: Unused "type: ignore" comment
+ sphinx/application.py:1012: error: Unused "type: ignore" comment
+ sphinx/application.py:1074: error: Unused "type: ignore" comment
+ sphinx/builders/gettext.py:60: error: Unused "type: ignore" comment
+ sphinx/ext/autodoc/type_comment.py:38: error: Unused "type: ignore" comment
+ sphinx/ext/viewcode.py:139: error: Unused "type: ignore" comment
+ sphinx/ext/viewcode.py:231: error: Unused "type: ignore" comment
+ sphinx/ext/viewcode.py:234: error: Unused "type: ignore" comment
+ sphinx/ext/viewcode.py:236: error: Unused "type: ignore" comment
+ sphinx/ext/intersphinx.py:170: error: Unused "type: ignore" comment
+ sphinx/ext/graphviz.py:228: error: Unused "type: ignore" comment

operator (https://github.com/canonical/operator)
- ops/testing.py:808: error: Item "str" of "Union[str, Application, Unit]" has no attribute "name"
- ops/testing.py:810: error: Argument 1 to "get" of "dict" has incompatible type "Union[str, Application, Unit]"; expected "str"
- ops/testing.py:2322: error: Item "str" of "Union[str, bytes, BinaryIO, TextIO]" has no attribute "read"
- ops/testing.py:2322: error: Item "bytes" of "Union[str, bytes, BinaryIO, TextIO]" has no attribute "read"
- ops/testing.py:2325: error: Argument 1 to "len" has incompatible type "Union[bytes, BinaryIO, TextIO]"; expected "Sized"
- ops/testing.py:2342: error: Argument 1 to "BytesIO" has incompatible type "Union[bytes, BinaryIO, TextIO]"; expected "bytes"
- ops/testing.py:2344: error: Item "BinaryIO" of "Union[bytes, BinaryIO, TextIO]" has no attribute "decode"
- ops/testing.py:2344: error: Item "TextIO" of "Union[bytes, BinaryIO, TextIO]" has no attribute "decode"

psycopg (https://github.com/psycopg/psycopg)
+ psycopg/psycopg/pq/misc.py:65: error: Redundant cast to "PGconn"  [redundant-cast]

urllib3 (https://github.com/urllib3/urllib3)
- test/with_dummyserver/test_https.py:463: error: Item "socket" of "Union[socket, Any]" has no attribute "server_hostname"  [union-attr]
- test/with_dummyserver/test_https.py:784: error: Item "socket" of "Union[socket, Any]" has no attribute "version"  [union-attr]

dd-trace-py (https://github.com/DataDog/dd-trace-py)
+ ddtrace/internal/logger.py:52: error: Unused "type: ignore" comment
+ ddtrace/profiling/_asyncio.py:31: error: Unused "type: ignore" comment
+ ddtrace/profiling/_asyncio.py:40: error: Unused "type: ignore" comment

discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/core.py:108: error: "Callable[..., Any]" has no attribute "__wrapped__"

streamlit (https://github.com/streamlit/streamlit)
+ lib/streamlit/logger.py:67: error: Unused "type: ignore" comment

pylint (https://github.com/pycqa/pylint)
+ pylint/utils/ast_walker.py:40: error: Unused "type: ignore" comment
+ pylint/checkers/similar.py:385: error: Unused "type: ignore" comment

mypy (https://github.com/python/mypy)
+ mypy/server/objgraph.py:67: error: Unused "type: ignore" comment
+ mypy/server/objgraph.py:69: error: Unused "type: ignore" comment
+ mypy/server/objgraph.py:71: error: Unused "type: ignore" comment

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/middleware/profiler.py:109: error: Unused "type: ignore" comment
+ src/werkzeug/serving.py:332: error: Unused "type: ignore" comment
+ src/werkzeug/wsgi.py:615: error: Unused "type: ignore" comment
+ src/werkzeug/wsgi.py:668: error: Unused "type: ignore" comment
+ src/werkzeug/debug/tbtools.py:187: error: Unused "type: ignore" comment
+ src/werkzeug/wrappers/response.py:442: error: Unused "type: ignore" comment
+ src/werkzeug/test.py:444: error: Unused "type: ignore" comment
+ src/werkzeug/test.py:452: error: Unused "type: ignore" comment
+ src/werkzeug/debug/__init__.py:332: error: Unused "type: ignore" comment
+ src/werkzeug/local.py:294: error: Unused "type: ignore" comment
+ src/werkzeug/middleware/lint.py:167: error: Unused "type: ignore" comment

cwltool (https://github.com/common-workflow-language/cwltool)
+ cwltool/executors.py:231: error: Unused "type: ignore" comment

rich (https://github.com/Textualize/rich)
+ rich/console.py:1290: error: Unused "type: ignore" comment

schema_salad (https://github.com/common-workflow-language/schema_salad)
+ schema_salad/makedoc.py: note: At top level:
+ schema_salad/makedoc.py:786: error: Unused "type: ignore" comment

pyp (https://github.com/hauntsaninja/pyp)
+ pyp.py:415: error: Unused "type: ignore" comment

bandersnatch (https://github.com/pypa/bandersnatch)
+ src/bandersnatch/utils.py:116: error: Unused "type: ignore" comment
+ src/bandersnatch/verify.py:139: error: Unused "type: ignore" comment

nox (https://github.com/wntrblm/nox)
+ nox/manifest.py:256: error: Unused "type: ignore" comment

pandas (https://github.com/pandas-dev/pandas)
+ pandas/util/_decorators.py:387: error: Unused "type: ignore" comment
+ pandas/io/common.py:1059: error: Unused "type: ignore" comment
+ pandas/io/common.py:1070: error: Unused "type: ignore" comment
+ pandas/core/base.py:1129: error: Unused "type: ignore" comment
+ pandas/core/indexes/base.py:5378: error: Unused "type: ignore" comment
+ pandas/plotting/_core.py:1853: error: Unused "type: ignore" comment
+ pandas/core/series.py:1702: error: Unused "type: ignore" comment
+ pandas/core/series.py:1707: error: Unused "type: ignore" comment
+ pandas/core/internals/array_manager.py:302: error: Unused "type: ignore" comment

spark (https://github.com/apache/spark)
+ python/pyspark/sql/types.py:1554: error: Unused "type: ignore" comment
+ python/pyspark/sql/types.py:1556: error: Unused "type: ignore" comment
+ python/pyspark/ml/util.py:307: error: Unused "type: ignore" comment
+ python/pyspark/pandas/typedef/typehints.py:155: error: Unused "type: ignore" comment
+ python/pyspark/pandas/typedef/typehints.py:157: error: Unused "type: ignore" comment
+ python/pyspark/pandas/typedef/typehints.py:162: error: Unused "type: ignore" comment
+ python/pyspark/pandas/typedef/typehints.py:172: error: Unused "type: ignore" comment

ibis (https://github.com/ibis-project/ibis)
- ibis/expr/types/temporal.py:617: error: "Node" has no attribute "negate"

core (https://github.com/home-assistant/core)
+ homeassistant/auth/providers/__init__.py:253: error: Unused "type: ignore" comment
+ homeassistant/auth/__init__.py:360: error: Unused "type: ignore" comment
+ homeassistant/helpers/entity.py:696: error: Unused "type: ignore" comment
+ homeassistant/helpers/entity.py:698: error: Unused "type: ignore" comment
+ homeassistant/components/notify/legacy.py:257: error: Unused "type: ignore" comment
+ homeassistant/components/media_player/__init__.py:903: error: Unused "type: ignore" comment
+ homeassistant/components/media_player/__init__.py:919: error: Unused "type: ignore" comment
+ homeassistant/components/media_player/__init__.py:937: error: Unused "type: ignore" comment
+ homeassistant/components/media_player/__init__.py:952: error: Unused "type: ignore" comment
+ homeassistant/components/climate/__init__.py:509: error: Unused "type: ignore" comment
+ homeassistant/components/climate/__init__.py:522: error: Unused "type: ignore" comment

jinja (https://github.com/pallets/jinja)
+ src/jinja2/loaders.py:392: error: Unused "type: ignore" comment
+ src/jinja2/async_utils.py:77: error: Redundant cast to "Iterable[V]"  [redundant-cast]
+ src/jinja2/runtime.py:275: error: Unused "type: ignore" comment
+ src/jinja2/runtime.py:277: error: Unused "type: ignore" comment
+ src/jinja2/ext.py:294: error: Unused "type: ignore" comment
+ src/jinja2/ext.py:301: error: Unused "type: ignore" comment

@ilevkivskyi ilevkivskyi merged commit b29051c into python:master Aug 29, 2022
@ilevkivskyi ilevkivskyi deleted the support-hasattr branch August 29, 2022 09:38
JukkaL added a commit that referenced this pull request Dec 26, 2022
This makes the implementation of hasattr() checks faster (introduced in
#13544).

In particular, since the `extra_attrs` attribute used for hasattr()
checks is usually None, I micro-optimized the codepaths to avoid
expensive operations whenever there are no hasattr() checks.

Also avoid expensive operations on simple unions and order `isinstance`
checks so that common types are checked first.

I measured a 2% performance uplift in self-check.
Sign up for free to 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.

mypyc failed to compile list comprehensions Type inference and hasattr
4 participants