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

Don't break mypy cache/incremental mode #5

Merged
merged 5 commits into from
Aug 28, 2023
Merged

Don't break mypy cache/incremental mode #5

merged 5 commits into from
Aug 28, 2023

Conversation

drvink
Copy link
Contributor

@drvink drvink commented Aug 15, 2023

Fixes #4.

Verified

This commit was signed with the committer’s verified signature.
Fixes #4.
@klausweiss
Copy link
Owner

Hey. Thanks a lot for this! I'll try to dive into this in the next few days.

Copy link
Owner

@klausweiss klausweiss left a comment

Choose a reason for hiding this comment

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

Super cool you figured this out! Thanks for your contribution. Any chance for an automated test that would break prior to this change, but doesn't now? Something along the lines of this gist from #4 would do.

# 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
Copy link
Owner

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?

Suggested change
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).

Copy link
Contributor Author

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.

Comment on lines 29 to 32
# 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
Copy link
Owner

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?

Copy link
Contributor Author

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(
Copy link
Owner

Choose a reason for hiding this comment

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

@drvink
Copy link
Contributor Author

drvink commented Aug 19, 2023

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 ProtocolIntersection[...] as a type alias, only in function signatures and class inheritance lists. I do have a file that triggers this bug pre-PR if you want it, but I don't have time to add a test for it.

@klausweiss
Copy link
Owner

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.

It probably is, sure. It'll probably take a few days for me to get round to it.

[edit] I just tried the gist and it makes mypy error, because the plugin doesn't have the right stuff to support using ProtocolIntersection[...] as a type alias, only in function signatures and class inheritance lists. I do have a file that triggers this bug pre-PR if you want it, but I don't have time to add a test for 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 ;)

@drvink
Copy link
Contributor Author

drvink commented Aug 25, 2023

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.

@klausweiss
Copy link
Owner

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 typing_protocol_intersection.

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 report_config_data to hopefully return a value more in the spirit of what it's supposed to do. Some black scramble to resolve and I'll merge.

@klausweiss klausweiss merged commit f75c63a into klausweiss:master Aug 28, 2023
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.

Cannot find component ProtocolIntersection\u200b
2 participants