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

Restore setting a Call as a base for classes from six.with_metaclass() #2049

Merged

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Mar 11, 2023

Type of Changes

Type
🐛 Bug fix

Description

Harden support for using enums as metaclasses with six.with_metaclass(...) syntax.

Fixes the crash in pylint-dev/pylint#5935 by adopting the check for not-none bases as in ClassDef._inferred_bases without recausing the false positive reported in pylint-dev/pylint#7506, which requires having correct bases. Here is the analogous existing code:

https://github.com/PyCQA/astroid/blob/91fd7b95fc943676f5aafb2d0ee881c4e167fa04/astroid/nodes/scoped_nodes/scoped_nodes.py#L3001-L3006

Essentially reverts #1622 (Sorry! It was my best understanding at the time, and at least we were trading a crash for a false positive.)

Refs pylint-dev/pylint#5935 (crash: tested here with test_metaclass_generator_hack_enum_base)
Refs pylint-dev/pylint#7506 (false positive: will open a pylint regression test PR shortly)

@jacobtylerwalls
Copy link
Member Author

@Pierre-Sassoulas do I remember correctly that you develop on Linux? The failing test passes for me on MacOS, so I'm wondering if there are different wheels for regex responsible for the difference.

@tusharsadhwani
Copy link
Contributor

Does this resolve #1735 as well?

@jacobtylerwalls
Copy link
Member Author

Does this resolve #1735 as well?

No, it doesn't. But I haven't worked out what the root issue is there--is it that it should expect with_metaclass to exist on six?

@tusharsadhwani
Copy link
Contributor

I'm unsure. I'm not familiar with that part of astroid, plus I've never used six.

Harden support for using enums as metaclasses.

Fixes the crash in pylint-dev/pylint#5935 by adopting the check
for not-none bases as in ClassDef._inferred_bases without recausing
the false positive reported in pylint-dev/pylint#7506, which requires
correct bases.
@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

Merging #2049 (aea9410) into main (c752c33) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2049      +/-   ##
==========================================
- Coverage   92.82%   92.82%   -0.01%     
==========================================
  Files          95       95              
  Lines       11066    11065       -1     
==========================================
- Hits        10272    10271       -1     
  Misses        794      794              
Flag Coverage Δ
linux 92.54% <100.00%> (-0.01%) ⬇️
pypy 88.17% <100.00%> (+<0.01%) ⬆️
windows 92.38% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
astroid/brain/brain_six.py 89.88% <ø> (-0.12%) ⬇️
astroid/nodes/scoped_nodes/scoped_nodes.py 93.07% <100.00%> (ø)

self.assertIsInstance(inferred.bases[0], nodes.Name)
self.assertEqual(inferred.bases[0].name, "C")
self.assertIsInstance(inferred.bases[0], nodes.Call)
Copy link
Member Author

@jacobtylerwalls jacobtylerwalls Mar 13, 2023

Choose a reason for hiding this comment

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

This change restores the prior results before #1622.

@Pierre-Sassoulas
Copy link
Member

Yes, I'm on linux, do you want me to test something ?

@DanielNoord
Copy link
Collaborator

Yes, I'm on linux, do you want me to test something ?

We xfailed some tests for the regex brain. Could you see why they fail on Linux but not on Mac/Windows?

@Pierre-Sassoulas
Copy link
Member

I can't reproduce the fail either:

pytest -k test_regex_pattern_and_match_subscriptable -vvv

=========================================================================================== test session starts ===========================================================================================
platform linux -- Python 3.10.6, pytest-7.2.1, pluggy-1.0.0 -- /home/pierre/pylint/tests/.pylint_primer_tests/PyCQA/astroid/venv/bin/python3
cachedir: .pytest_cache
rootdir: /home/pierre/pylint/tests/.pylint_primer_tests/PyCQA/astroid, configfile: pyproject.toml, testpaths: tests
plugins: cov-4.0.0

collected 1549 items / 1548 deselected / 1 selected                                                                                                                                                       

tests/brain/test_regex.py::TestRegexBrain::test_regex_pattern_and_match_subscriptable PASSED                                                                                                        [100%]
=================================================================================== 1 passed, 1548 deselected in 1.51s ====================================================================================

@jacobtylerwalls jacobtylerwalls merged commit b5ebf99 into pylint-dev:main Mar 26, 2023
@jacobtylerwalls jacobtylerwalls deleted the with-metaclass-base-fixes branch March 26, 2023 12:17
github-actions bot pushed a commit that referenced this pull request Mar 26, 2023
…s` (#2049)

Harden support for using enums as metaclasses.

Fixes the crash in pylint-dev/pylint#5935 by adopting the check
for not-none bases as in ClassDef._inferred_bases without recausing
the false positive reported in pylint-dev/pylint#7506, which requires
correct bases.

(cherry picked from commit b5ebf99)
Pierre-Sassoulas pushed a commit that referenced this pull request Mar 26, 2023
…s` (#2049) (#2067)

Harden support for using enums as metaclasses.

Fixes the crash in pylint-dev/pylint#5935 by adopting the check
for not-none bases as in ClassDef._inferred_bases without recausing
the false positive reported in pylint-dev/pylint#7506, which requires
correct bases.

(cherry picked from commit b5ebf99)

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
@Pierre-Sassoulas Pierre-Sassoulas added backported Assigned once the backport is done and removed backport maintenance/2.15.x labels Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Assigned once the backport is done Bug 🪳 pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants