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

Migrate core to use ErrorMessage class #12004

Closed
wants to merge 26 commits into from
Closed

Migrate core to use ErrorMessage class #12004

wants to merge 26 commits into from

Conversation

tushar-deepsource
Copy link
Contributor

@tushar-deepsource tushar-deepsource commented Jan 17, 2022 •
edited

Description

Migrates fastparse.py and fastparse2.py, semanal.py, semanal_*.py and typeanal.py to use the ErrorMessage class.

The initial plan was to do only fastparse and fastparse2, but because of how intertwined the core files are, all of these had to be done in one go.

This is a continuation of the discussion on #10947.

Notes:

  • The fail method of SemanticAnalyzer and SemanticAnalyzerCoreInterface still implements the msg: str code instead of only supporting msg: ErrorMessage, because all the plugins that implement SemanticAnalyzerCoreInterface have yet to be migrated. I could do them in the same PR but the PR is already huge.
  • One message (specifically "Mypy doesn't support match stataments") was not moved to the message registry as it seems temporary.

@tushar-deepsource
Copy link
Contributor Author

Well, it seems like typeanal.py also uses the one method that I had to change in nodes.py, so this PR will be migrating all 3 of those files at once, hopefully that's not too big!

@github-actions

This comment has been minimized.

Sign in to view
@tushar-deepsource tushar-deepsource changed the title Migrate fastparse to use ErrorMessage class Migrate fastparse and typeanal to use ErrorMessage class Jan 17, 2022
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
1 similar comment
@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
@github-actions

This comment has been minimized.

Sign in to view
@tushar-deepsource tushar-deepsource changed the title Migrate fastparse and typeanal to use ErrorMessage class Migrate core to use ErrorMessage class Jan 18, 2022
@github-actions

This comment has been minimized.

Sign in to view
@tushar-deepsource
Copy link
Contributor Author

@sobolevn would you be able to review this?
The longer it takes to get reviewed the more work I'd have to keep putting in to maintain the PR

@github-actions

This comment has been minimized.

Sign in to view
tushar-deepsource
tushar-deepsource commented Feb 7, 2022
View reviewed changes
mypy/semanal.py Outdated Show resolved Hide resolved
sobolevn
sobolevn approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment •
edited

Choose a reason for hiding this comment

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

I am basically fine with these changes!

My concerns:

  1. Merging this will make many other PRs to have merge conflicts
  2. Some existing PRs add raw-text errors. We need to somehow catch this. Maybe using a custom mypy plugin? Similar to one we have with isinstance
  3. We now associate message contents with variable names, this might get out of sync quite quickly

I totally need a second opinion on that!

mypy/semanal.py
@@ -5148,8 +5132,9 @@ def in_checked_function(self) -> bool:
# no regular functions.
return True

# TODO(tushar): remove `str` type and `code` property from here
Copy link
Member

Choose a reason for hiding this comment

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

Plugins might rely on msg: str, so no need to delete it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you're right. But I think it's a better model in the long term, for plugins to define a set of ErrorMessage objects somewhere in their files, instead of doing inline messages. For the same reasons as why we're doing it in mypy.

Could consider a deprecation cycle and enforcing this on plugins too!

Copy link
Member

Choose a reason for hiding this comment

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

Could consider a deprecation cycle

As far as I know, we never did something like this before.

@tusharsadhwani
Copy link
Contributor

tusharsadhwani commented Feb 7, 2022 •
edited

Merging this will make many other PRs to merge conflicts

The merge conflicts will be easy fixes, and will teach the contributors about the new error message model, I think that's a good thing

We now associate message contents with variable names, this might get out of sync quite quickly

It's fine to change variable name and message text together. When issue codes (unique identifier for each message) are added to ErrorMessage objects, only they need to remain constant for a relatively long time.

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@JelleZijlstra
Copy link
Member

This seems invasive enough that I'd want to get @JukkaL's signoff.

@JelleZijlstra JelleZijlstra removed their request for review February 9, 2022 02:48
@sobolevn sobolevn requested a review from JukkaL February 9, 2022 06:13
@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
1 similar comment
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@tushar-deepsource
Copy link
Contributor Author

@JukkaL PTAL, #10959 is already merged for example, this is just a continuation of the effort (with more to come)

@tushar-deepsource
Copy link
Contributor Author

Friendly bump, it's been a year since I first started these set of PRs, and I really want to finish this refactor, to be able to add support for issue codes to mypy.

@tusharsadhwani tusharsadhwani mentioned this pull request Feb 21, 2023
Merged
@tusharsadhwani
Copy link
Contributor

I'm redoing this PR in a bunch of granular versions instead, starting with #14753.

Hope that will make it easier to get reviewed.

JukkaL pushed a commit that referenced this pull request Apr 24, 2023
@tusharsadhwani
Migrate fastparse to use ErrorMessage class (#14753)
9ce3470 
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.
@tusharsadhwani tusharsadhwani mentioned this pull request Apr 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn approved these changes

@tusharsadhwani tusharsadhwani tusharsadhwani left review comments

@raghavthind2005 raghavthind2005 raghavthind2005 approved these changes

@JukkaL JukkaL Awaiting requested review from JukkaL

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tushar-deepsource @tusharsadhwani @JelleZijlstra @sobolevn @raghavthind2005