-
Notifications
You must be signed in to change notification settings - Fork 2
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
Don't break mypy cache/incremental mode #5
Conversation
Hey. Thanks a lot for this! I'll try to dive into this in the next few days. |
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.
# generated any types on a previous run; otherwise mypy will try to look | ||
# them up in the cache (and subsequently crash when it fails to find | ||
# them). | ||
return UniqueFullname.instance_counter == 0 |
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.
Should we rather use this in case instance count changed between reruns?
return UniqueFullname.instance_counter == 0 | |
return UniqueFullname.instance_counter |
TBF I'm not sure if that's even possible, but this seems like a more accurate thing to compare, as opposed to a bool (even though that seemed to work in my tests 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.
As far as I can tell, the situation is simply that if the funny unique names end up in the cache, mypy breaks. So it doesn't matter exactly how many were generated; we simply want to ensure that the cache for a given file is invalidated if the plugin generated any unique names during a previous typechecking run for that file. We don't care about the case where there's no cached plugin state, and we also don't need to worry about files that didn't get funny names written into their cache, so as long as we get the caches invalidated for ones that did, everything is fine.
You can see how this dumb trick works for yourself by using this:
ret = UniqueFullname.instance_counter == 0
print(f'[{ctx.is_check}] {ctx.path}: {ret}', file=sys.stderr)
return ret
Or, if you're feeling formal, I think this translation is correct:
∀ isCheck cachedVal didNoWork,
if isCheck ∧ cachedVal then didNoWork = cachedVal ⇒ didNoWork else
if isCheck ∧ ¬cachedVal then didNoWork ≠ cachedVal ⇒ didNoWork else
if ¬isCheck then didNoWork ∨ ¬didNoWork else
⊥
which works out to be true.
# When is_check is False, the value returned by this function will be | ||
# serialized as json and saved to the mypy cache. When is_check is | ||
# True, the value returned will be compared against whatever was loaded | ||
# from the cache. This is so that a plugin can potentially invalidate |
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.
Could we give some context for what is_check
is, when is it True or False and why do we care?
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 callback is first invoked pre-run (ctx.is_check
== True) in order to check that the loaded cache contents match the plugin's desired state, and later invoked post-run (ctx.is_check
== False) when mypy asks the plugin what it wants to store to the cache, if anything.
@@ -23,6 +23,19 @@ | |||
class ProtocolIntersectionPlugin(mypy.plugin.Plugin): | |||
# pylint: disable=unused-argument | |||
|
|||
def report_config_data( |
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.
If that gist used to reproduce the issue and no longer does, isn't that sufficient for the test? (You can push changes to this PR yourself to add a test before you merge it; probably easier for you to do it than me.) [edit] I just tried the gist and it makes mypy error, because the plugin doesn't have the right stuff to support using |
It probably is, sure. It'll probably take a few days for me to get round to it.
If you have one which makes mypy pass and used to crash before your change, that would be great, thanks! I'll try to find the time to wrap it in an integration test. Good call with the type alias support BTW. I'll open an issue when I investigate a bit more. Anyone is free to open if you beat me to it ;) |
This should do it. |
I couldn't reproduce the original error with this file under python 3.11 and mypy 1.15. Are you sure this is the right file? It doesn't import Anyway, I have finally found some time and utilized the script from the original gist for an automated test case (FYI @evhub, and many thanks!). I have modified |
Fixes #4.