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

Refactor error messages into a single file #10947

Closed
wants to merge 33 commits into from

Conversation

tushar-deepsource
Copy link
Contributor

@tushar-deepsource tushar-deepsource commented Aug 8, 2021

Description

This PR moves all errors thrown by mypy into the already-exsting message_registry.py, while also wrapping every error message into an ErrorMessage class.

This is an effort to be able to add unique codes to every error that mypy throws, to resolve #10843.

Changes

  • Almost all of mypy's error messages are either static strings, or are templates which are filled using str.format. So, instead of having them scattered around all of the files, this puts every single error raised into message_registry.py.
  • Instead of passing a separate msg string and code value into every fail() method call, now error messages being raised are ErrorMessage objects, which puts the error message and error code together.

Progress

This tracks how many files have been migrated from strings to the central ErrorMessage approach.

  • Migrate checkers.py
  • Migrate other checker files
  • Migrate typeanal.py
  • Migrate smaller semantic analyzers
  • Migrate enums, typeddict and namedtuple analyzers
  • Migrate checkerstrformat.py
  • Migrate messages.py
  • Migrate semanal.py
  • Fix abstract classes in plugins.py
  • Migrate plugins

There are also a few instances of ErrorMessage objects being created outside message_registry.py for supporting dynamic behaviour. Most of those can be removed as well, which is work in progress.

Future plans

I would also like to properly categorize all of the error messages, and then eventually assign unique identifiers: either numeric (eg. E0163), or readable (eg. unreachable-right-operand) to every single message. This would allow much easier identification of error messages by people, or by other tools.

Suggestions

I'd like to know about any suggestions, changes you'd want to make, or things I have overlooked. Do let me know! I'd be happy to make changes accordingly.

@github-actions

This comment has been minimized.

@TH3CHARLie
Copy link
Collaborator

Thanks for working on this. This really is a nice refactoring though its own complexity may be too much to pack into a single PR. I feel it's completely OK to open separate ones to do the refactoring.

@tushar-deepsource
Copy link
Contributor Author

@TH3CHARLie Okay, how do I go about breaking this down. Maybe break it into checkers/semanal/messages and so on? each with 300 or so lines changed?

@TH3CHARLie
Copy link
Collaborator

That would be a decent approach, you can use your bullet list to break up the PR. I think that provides a very nice plan.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@tushar-deepsource
Copy link
Contributor Author

you can use your bullet list to break up the PR

@TH3CHARLie Alright, I'll do that.

The only issue seems to be that mypyc doesn't compile with the ErrorMessage class I introduced. Something about not being able to determine types. Every other test passes. Some bug?

@ethanhs
Copy link
Collaborator

ethanhs commented Aug 9, 2021

This change will also affect several plugins I expect, so if you want to go ahead with a split up version of this, we should notify plugin maintainers in #6617

@github-actions
Copy link
Contributor

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

core (https://github.com/home-assistant/core.git)
+ homeassistant/components/gios/__init__.py:33: error: unused "type: ignore" comment

pydantic (https://github.com/samuelcolvin/pydantic.git)
+ pydantic/mypy.py:567: error: Unexpected keyword argument "code" for "fail" of "CheckerPluginInterface"  [call-arg]
+ pydantic/mypy.py:567: error: Argument 1 to "fail" of "CheckerPluginInterface" has incompatible type "str"; expected "ErrorMessage"  [arg-type]
+ pydantic/mypy.py:571: error: Unexpected keyword argument "code" for "fail" of "SemanticAnalyzerPluginInterface"  [call-arg]
+ pydantic/mypy.py:571: error: Argument 1 to "fail" of "SemanticAnalyzerPluginInterface" has incompatible type "str"; expected "ErrorMessage"  [arg-type]
+ pydantic/mypy.py:575: error: Unexpected keyword argument "code" for "fail" of "SemanticAnalyzerPluginInterface"  [call-arg]
+ pydantic/mypy.py:575: error: Argument 1 to "fail" of "SemanticAnalyzerPluginInterface" has incompatible type "str"; expected "ErrorMessage"  [arg-type]
+ pydantic/mypy.py:583: error: Unexpected keyword argument "code" for "fail" of "CheckerPluginInterface"  [call-arg]
+ pydantic/mypy.py:583: error: Argument 1 to "fail" of "CheckerPluginInterface" has incompatible type "str"; expected "ErrorMessage"  [arg-type]
+ pydantic/mypy.py:587: error: Unexpected keyword argument "code" for "fail" of "SemanticAnalyzerPluginInterface"  [call-arg]
+ pydantic/mypy.py:587: error: Argument 1 to "fail" of "SemanticAnalyzerPluginInterface" has incompatible type "str"; expected "ErrorMessage"  [arg-type]

pytest (https://github.com/pytest-dev/pytest.git)
+ src/_pytest/python_api.py:719: error: Subclass of "Iterable[Any]", "Sized", and "bytes" cannot exist: would have inconsistent method resolution order  [misc]
+ src/_pytest/python_api.py:719: error: Subclass of "Iterable[Any]", "Sized", and "str" cannot exist: would have inconsistent method resolution order  [misc]
+ src/_pytest/cacheprovider.py:143: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)
- src/_pytest/cacheprovider.py:143: error: Returning Any from function declared to return "Path"  [no-any-return]
- src/_pytest/cacheprovider.py:153: error: Returning Any from function declared to return "Path"  [no-any-return]
- testing/test_tmpdir.py:52: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 10, 2021

I'm not sure if this is the right direction (see #10843 (comment)). This could break backward compatibility or regress usability of # type: ignore[...] comments, in particular.

@tushar-deepsource
Copy link
Contributor Author

@JukkaL This PR at least, i believe doesn't change any usability at all.

Regarding the introduction if fine-grained error codes, it can be implemented along with the already existing error codes, see #10843 (comment) for an example. I'd be happy to clarify if it's unclear.

JukkaL pushed a commit that referenced this pull request Apr 24, 2023
Use the `ErrorMessage` class from `message_registry.py` to move the
error messages from `fastparse` module into the message registry.

This is a retry of #10947 and #12004, but much more granular this time.
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.

Exact error codes for mypy errors
4 participants