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

Complete cache key for inference tip #2158

Merged
merged 14 commits into from
May 6, 2023

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Apr 30, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

As identified by @kriek in #1828, part of the cache key (context) was missing.

Alternative to #1832, which removes the cache, this just adds the missing part of the key.

Closes #1828
Closes pylint-dev/pylint#7464
Closes pylint-dev/pylint#8074

Refs #1982 (reverted)

Pylint test

The following diff in pylint will (almost) pass the test suite with this change. PR shortly. PR once I can debug the remaining failures with deprecated_typing_alias.

diff --git a/pylint/checkers/design_analysis.py b/pylint/checkers/design_analysis.py
index 701615d89..d605aed76 100644
--- a/pylint/checkers/design_analysis.py
+++ b/pylint/checkers/design_analysis.py
@@ -166,6 +166,8 @@ STDLIB_CLASSES_IGNORE_ANCESTOR = frozenset(
         "typing.AsyncContextManager",
         "typing.Hashable",
         "typing.Sized",
+        TYPING_NAMEDTUPLE,
+        TYPING_TYPEDDICT,
     )
 )

The cache key was lacking the `context` arg

Co-authored-by: Sylvain Ackermann <sylvain.ackermann@gmail.com>
@jacobtylerwalls jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label Apr 30, 2023
@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a3 milestone Apr 30, 2023
@codecov
Copy link

codecov bot commented Apr 30, 2023

Codecov Report

Merging #2158 (a79a2a7) into main (06fafc4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2158   +/-   ##
=======================================
  Coverage   92.53%   92.53%           
=======================================
  Files          94       94           
  Lines       10800    10804    +4     
=======================================
+ Hits         9994     9998    +4     
  Misses        806      806           
Flag Coverage Ξ”
linux 92.29% <100.00%> (+<0.01%) ⬆️
pypy 87.48% <100.00%> (+<0.01%) ⬆️
windows 92.13% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
astroid/inference_tip.py 97.36% <100.00%> (+0.30%) ⬆️

DanielNoord
DanielNoord previously approved these changes Apr 30, 2023
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Test improvements look nice!

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Apr 30, 2023

Unfortunately it has the same problem in pypy with RecursionError identified in #2002, see https://github.com/jacobtylerwalls/pylint/actions/runs/4844716534/jobs/8633169007?pr=10.

Seems like the right approach, though, so we need to just deal with the RecursionError or something.

@jacobtylerwalls
Copy link
Member Author

More history!

The test in question was skipped in #1984, then the skip was removed in #2000 when the decision was made to revert the substantive change dating to #1982.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Nice long-awaited improvements seeing the modification in the tests πŸ˜„

@@ -954,7 +948,7 @@ def ident(var: T) -> T:

i1 = next(ast_nodes[1].infer())
assert isinstance(i1, nodes.Const)
assert i1.value == 2 # should be "Hello"!
assert i1.value == "Hello"
Copy link
Member

Choose a reason for hiding this comment

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

πŸŽ‰

@jacobtylerwalls
Copy link
Member Author

Sorry for the noise, I'm experimenting to see if I can get the recursion error in pypy to resolve.

@jacobtylerwalls jacobtylerwalls removed the pylint-tested PRs that don't cause major regressions with pylint label Apr 30, 2023
@jacobtylerwalls jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label Apr 30, 2023
@jacobtylerwalls
Copy link
Member Author

RecursionError in PyPy fixed, see run on my fork failing on something else (compare to earlier run failing with RecursionError).

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

You've been contributing some serious improvements in perf/caching -that astroid and pylint needs most- @jacobtylerwalls, amazing πŸ‘Œ

@Pierre-Sassoulas
Copy link
Member

The readthedoc fail could be fixed by pinning urllib3 to < 2 it seems ?

@jacobtylerwalls
Copy link
Member Author

Others have solved it by ensuring Python 3.9 or higher is used for the build. Can we do that in a read the docs .yml?

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented May 6, 2023

Suggestions!

@jacobtylerwalls jacobtylerwalls removed the request for review from cdce8p May 6, 2023 15:16
@jacobtylerwalls jacobtylerwalls merged commit 0740a0d into pylint-dev:main May 6, 2023
16 checks passed
@jacobtylerwalls jacobtylerwalls deleted the incomplete-cache-key branch May 6, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
3 participants