Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
Propagate type narrowing to nested functions#15133
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
This comment has been minimized.
This comment has been minimized.
JukkaL commentedApr 25, 2023
mypy_primer output looks good. The new errors in openlibrary are from fixed false negatives. Similarly, this error from spark is due to better inferred types: Everything else seems to be caused by either fewer errors generated (good!) or some error code changing due to a union type being replaced with a narrowed, non-union type. |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This comment has been minimized.
This comment has been minimized.
hauntsaninja left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is awesome, thanks for making it happen! One possible improvement: this fails if you use the name after the function is defined, even if you're just loading it and not assigning to it
JukkaL commentedApr 26, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think that this mostly works, since we only stop narrowing if the name is used in an lvalue. However, I noticed that we didn't handle some cases correctly. I've pushed a fix. Also, there was minimal test coverage for this. I updated tests to cover this case better. |
This comment has been minimized.
This comment has been minimized.
hauntsaninja commentedApr 26, 2023
Ah yeah, sorry, it does work! Thanks for adding the tests and for fixing for index and member exprs. (What happened is I'd noticed there weren't tests for that case, so I locally added the test case and it failed, but only now am I realising that it failed for fixture reasons since we don't have |
ilevkivskyi left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LG, but have couple comments on tests.
test-data/unit/check-optional.test Outdated
| x: Optional[str] = "x" | ||
| def cannot_narrow_top_level() -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The actual narrowing is missing here. Until#2008 is fixed this would be optional despite r.h.s. So you need to write
x:Optional[str]ifxisNone:x="x"
here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah this was an incomplete test case.
| cc[c] = 3 | ||
| nested() | ||
| [builtins fixtures/isinstance.pyi] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it would be good to add tests for edge cases in the visitor likex, y = foo() and/orx = y = foo().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added some test cases.
Diff frommypy_primer, showing the effect of this PR on open source code: werkzeug (https://github.com/pallets/werkzeug)+ src/werkzeug/local.py:495: error: Unused "type: ignore" comment [unused-ignore]+ src/werkzeug/local.py:506: error: Unused "type: ignore" comment [unused-ignore]+ src/werkzeug/local.py:515: error: Unused "type: ignore" comment [unused-ignore]jinja (https://github.com/pallets/jinja)+ src/jinja2/utils.py:262: error: Unused "type: ignore" comment [unused-ignore]+ src/jinja2/runtime.py:933: error: Unused "type: ignore" comment [unused-ignore]+ src/jinja2/environment.py:925: error: Unused "type: ignore" comment [unused-ignore]psycopg (https://github.com/psycopg/psycopg)+ psycopg/psycopg/rows.py:125: error: Unused "type: ignore" comment [unused-ignore]+ psycopg/psycopg/rows.py:172: error: Unused "type: ignore" comment [unused-ignore]+ psycopg/psycopg/rows.py:208: error: Unused "type: ignore" comment [unused-ignore]openlibrary (https://github.com/internetarchive/openlibrary)+ openlibrary/plugins/upstream/account.py: note: In function "csv_string":+ openlibrary/plugins/upstream/account.py:854: error: Argument 1 to "row_formatter" has incompatible type "Mapping[Any, Any]"; expected "dict[Any, Any]" [arg-type]+ openlibrary/plugins/upstream/account.py:856: error: Argument 1 to "row_formatter" has incompatible type "Mapping[Any, Any]"; expected "dict[Any, Any]" [arg-type]isort (https://github.com/pycqa/isort)+ isort/sorting.py:120: error: Unused "type: ignore" comment [unused-ignore]graphql-core (https://github.com/graphql-python/graphql-core)+ src/graphql/validation/validate.py:61: error: Unused "type: ignore" comment [unused-ignore]+ tests/test_user_registry.py:495: error: Unused "type: ignore" comment [unused-ignore]+ tests/test_user_registry.py:501: error: Unused "type: ignore" comment [unused-ignore]spark (https://github.com/apache/spark)+ python/pyspark/rdd.py:1515: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/rdd.py:2212: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/rdd.py:2252: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/rdd.py:2465: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/rdd.py:2472: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/sql/streaming/readwriter.py:1243: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/sql/streaming/readwriter.py:1286: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/sql/streaming/readwriter.py:1286: error: "SupportsProcess" has no attribute "open" [attr-defined]+ python/pyspark/sql/streaming/readwriter.py:1286: note: Error code "attr-defined" not covered by "type: ignore" comment+ python/pyspark/sql/streaming/readwriter.py:1293: error: Redundant cast to "SupportsProcess" [redundant-cast]+ python/pyspark/sql/streaming/readwriter.py:1298: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/sql/streaming/readwriter.py:1298: error: "SupportsProcess" has no attribute "close" [attr-defined]+ python/pyspark/sql/streaming/readwriter.py:1298: note: Error code "attr-defined" not covered by "type: ignore" comment+ python/pyspark/streaming/dstream.py:803: error: Unused "type: ignore" comment [unused-ignore]+ python/pyspark/streaming/dstream.py:852: error: Redundant cast to "int" [redundant-cast]+ python/pyspark/pandas/frame.py:6247: error: Argument "value" to "replace" of "Series" has incompatible type "int | float | str | list[Any] | tuple[Any, ...] | dict[Any, Any]"; expected "list[Any] | tuple[Any, ...]" [arg-type]+ python/pyspark/pandas/namespace.py:2306: error: Redundant cast to "list[str]" [redundant-cast]+ python/pyspark/pandas/namespace.py:2309: error: Redundant cast to "list[str]" [redundant-cast]cwltool (https://github.com/common-workflow-language/cwltool)+ cwltool/validate_js.py: note: In function "get_expressions":+ cwltool/validate_js.py:97:17: error: Redundant cast to "ArraySchema" [redundant-cast]ibis (https://github.com/ibis-project/ibis)- ibis/backends/base/__init__.py:754: error: "BaseBackend" has no attribute "compiler"; maybe "compile"? [attr-defined]hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)- src/hydra_zen/structured_configs/_implementations.py:1832: error: Argument 1 to "list" has incompatible type "list[str | DataClass_ | Mapping[str, str | Sequence[str] | None]] | None"; expected "Iterable[Any]" [arg-type]- src/hydra_zen/structured_configs/_make_custom_builds.py:299: error: List item 0 has incompatible type "DataclassOptions | None"; expected "SupportsKeysAndGetItem[Any, Any]" [list-item]discord.py (https://github.com/Rapptz/discord.py)- discord/app_commands/tree.py:332: error: Incompatible types in assignment (expression has type "Command[Any, [VarArg(Any), KwArg(Any)], Any] | ContextMenu | Group", target has type "ContextMenu") [assignment]prefect (https://github.com/PrefectHQ/prefect)- src/prefect/_internal/compatibility/deprecated.py:199: error: "None" not callable [misc]- src/prefect/_internal/compatibility/experimental.py:232: error: "None" not callable [misc]urllib3 (https://github.com/urllib3/urllib3)+ src/urllib3/util/request.py:226: error: Unused "type: ignore" comment [unused-ignore] |
schuelermine commentedMay 3, 2023
Thank you for this fix! |
cdce8p commentedMay 3, 2023
This PR seems to cause a new crash with Home Assistant. I narrowed it down to this (tested with Python 3.11). Not sure why fromcontextlibimportsuppressimportosfromtypingimportAnydefsetup(config:Any)->None:ifconfigisnotNone:os.path.join(config,"a")deffunc()->None:withsuppress(RuntimeError):passreveal_type(1) Traceback(venv-311) $ mypy --no-incremental test.py test.py:11: error: INTERNAL ERROR -- Please try using mypy master on GitHub:https://mypy.readthedocs.io/en/stable/common_issues.html#using-a-development-mypy-buildPlease report a bug at https://github.com/python/mypy/issuesversion: 1.4.0+dev.61b9b9c75fd79090fcd127e5030f18342224e90bTraceback (most recent call last): File"/.../venv-311/bin/mypy", line 8,in<module>sys.exit(console_entry()) File"/.../mypy/mypy/__main__.py", line 15,in console_entrymain() File"/.../mypy/mypy/main.py", line 95,in main res, messages, blockers = run_build(sources, options, fscache, t0, stdout, stderr) File"/.../mypy/mypy/main.py", line 174,in run_build res = build.build(sources, options, None, flush_errors, fscache, stdout, stderr) File"/.../mypy/mypy/build.py", line 194,in build result = _build( File"/.../mypy/mypy/build.py", line 267,in _build graph = dispatch(sources, manager, stdout) File"/.../mypy/mypy/build.py", line 2926,in dispatch process_graph(graph, manager) File"/.../mypy/mypy/build.py", line 3324,in process_graph process_stale_scc(graph, scc, manager) File"/.../mypy/mypy/build.py", line 3425,in process_stale_sccgraph[id].type_check_first_pass() File"/.../mypy/mypy/build.py", line 2311,in type_check_first_passself.type_checker().check_first_pass() File"/.../mypy/mypy/checker.py", line 475,in check_first_pass self.accept(d) File"/.../mypy/mypy/checker.py", line 585,in accept stmt.accept(self) File"/.../mypy/mypy/nodes.py", line 784,in acceptreturn visitor.visit_func_def(self) File"/.../mypy/mypy/checker.py", line 963,in visit_func_def self._visit_func_def(defn) File"/.../mypy/mypy/checker.py", line 967,in _visit_func_def self.check_func_item(defn, name=defn.name) File"/.../mypy/mypy/checker.py", line 1039,in check_func_item self.check_func_def(defn, typ, name, allow_empty) File"/.../mypy/mypy/checker.py", line 1234,in check_func_def self.accept(item.body) File"/.../mypy/mypy/checker.py", line 585,in accept stmt.accept(self) File"/.../mypy/mypy/nodes.py", line 1218,in acceptreturn visitor.visit_block(self) File"/.../mypy/mypy/checker.py", line 2659,in visit_block self.accept(s) File"/.../mypy/mypy/checker.py", line 585,in accept stmt.accept(self) File"/.../mypy/mypy/nodes.py", line 784,in acceptreturn visitor.visit_func_def(self) File"/.../mypy/mypy/checker.py", line 963,in visit_func_def self._visit_func_def(defn) File"/.../mypy/mypy/checker.py", line 967,in _visit_func_def self.check_func_item(defn, name=defn.name) File"/.../mypy/mypy/checker.py", line 1039,in check_func_item self.check_func_def(defn, typ, name, allow_empty) File"/.../mypy/mypy/checker.py", line 1234,in check_func_def self.accept(item.body) File"/.../mypy/mypy/checker.py", line 585,in accept stmt.accept(self) File"/.../mypy/mypy/nodes.py", line 1218,in acceptreturn visitor.visit_block(self) File"/.../mypy/mypy/checker.py", line 2659,in visit_block self.accept(s) File"/.../mypy/mypy/checker.py", line 585,in accept stmt.accept(self) File"/.../mypy/mypy/nodes.py", line 1574,in acceptreturn visitor.visit_with_stmt(self) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File"/.../mypy/mypy/checker.py", line 4771,in visit_with_stmt with self.binder.frame_context(can_skip=True, try_frame=True): File"/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/contextlib.py", line 144,in __exit__ next(self.gen) File"/.../mypy/mypy/binder.py", line 435,in frame_context self.pop_frame(can_skip, fall_through) File"/.../mypy/mypy/binder.py", line 247,in pop_frame self.last_pop_changed = self.update_from_options(options) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File"/.../mypy/mypy/binder.py", line 223,in update_from_optionstype = join_simple(self.declarations[key], type, other)~~~~~~~~~~~~~~~~~^^^^^KeyError: ('Var',<mypy.nodes.Var object at 0x104254e10>)test.py:11:: note: use --pdb to drop into pdb |
JukkaL commentedMay 4, 2023
I can confirm the crash. Thanks for the report! I'm looking at it. |
hauntsaninja commentedMay 4, 2023
Thanks for testing dev versions! |
Fixes#2608.
Use the heuristic suggested in#2608 and allow narrowed types of variables (but not attributes) to be propagated to nested functions if the variable is not assigned to after the definition of the nested function in the outer function.
Since we don't have a full control flow graph, we simply look for assignments that are textually after the nested function in the outer function. This can result in false negatives (at least in loops) and false positives (in if statements, and if the assigned type is narrow enough), but I expect these to be rare and not a significant issue. Type narrowing is already unsound, and the additional unsoundness seems minor, while the usability benefit is big.
This doesn't do the right thing for nested classes yet. I'll create an issue to track that.