Fix `abstract-class-instantiated` false positive for `new` method. #8006

clavedeluna · 2022-12-31T20:43:15Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #3060

codecov · 2022-12-31T20:51:15Z

Codecov Report

Merging #8006 (e600117) into main (ae3f36d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8006   +/-   ##
=======================================
  Coverage   95.51%   95.52%           
=======================================
  Files         176      176           
  Lines       18541    18557   +16     
=======================================
+ Hits        17710    17726   +16     
  Misses        831      831

Impacted Files	Coverage Δ
pylint/checkers/base/basic_error_checker.py	`95.85% <100.00%> (+0.29%)`	⬆️

pylint/checkers/base/basic_error_checker.py

clavedeluna · 2023-01-05T11:57:33Z

Blocked by codecov issue

clavedeluna · 2023-01-09T13:32:36Z

primer output is exactly what we'd expect here

DanielNoord

Thanks for fixing the coverage and test!

DanielNoord · 2023-01-09T13:35:27Z

pylint/checkers/base/basic_error_checker.py

+    if "__new__" not in node.locals:
+        return False
+
+    new = next(node.igetattr("__new__"))


This should probably be done in a try...except. Or with safe_infer.

try: new = next(node.igetattr("__new__")) except astroid.InferenceError: return False

is the closest I see to other repo code using igetattr. However, I can't think of a single example where we'd actually raise InferenceError since new is a dunder for pretty much any class. Do you have a suggestion? I can remove the try/except or ignore coverage for this line just for precaution.

Hmm.. @Pierre-Sassoulas what do you want? Non-covered code? Or code that is known to raise a certain exception that we just don't know the edge case for yet?

Let's go with uncovered code catching the inference errors, it's safer imo.

pylint/checkers/base/basic_error_checker.py

DanielNoord · 2023-01-09T13:37:33Z

pylint/checkers/base/basic_error_checker.py

@@ -467,8 +496,16 @@ def _check_inferred_class_is_abstract(
            return

        if metaclass.qname() in ABC_METACLASSES:


Let's make L498 a if not and then return. There is no need for this indentation here.

DanielNoord · 2023-01-09T13:37:48Z

tests/functional/a/abstract/abstract_class_instantiated.py

 import weakref
 from lala import Bala
+import pandas as pd


Let's remove this test as we don't install pandas.

DanielNoord · 2023-01-09T13:39:19Z

tests/functional/a/abstract/abstract_class_instantiated.py

+        """book."""
+
+
+bad = GoodithComplexNew()


What about:

def __new__(self): cls = super().__new__() return cls def __new__(self): cls = super().__new__() return "1"

as long as the class has abstract methods, yes msg will be emitted. will add them as test cases

DanielNoord

Could you take a look at the coverage? For the first one the pragma needs to be on the line two below.

DanielNoord · 2023-01-22T12:51:19Z

pylint/checkers/base/basic_error_checker.py

-            self.add_message(
-                "abstract-class-instantiated", args=(inferred.name,), node=node
-            )
+        if metaclass.qname() not in ABC_METACLASSES:


Can you add a test for this?

Turns out I can't come up with a single class example where we would land in this code, so going to remove this check. The previous if-checks above this handle the edge cases.

clavedeluna · 2023-01-24T20:12:12Z

hm need to see what's up with that new primer output

… ``__new__`` method. Closes pylint-dev#3060

github-actions · 2023-01-26T12:47:34Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on pandas:
The following messages are no longer emitted:

abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/io/formats/excel.py#L937
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L110
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L145
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L363
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L519
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L657
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L660
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L1103
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L1218
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L1223
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L1248
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L1257
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L1293
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_writers.py#L1317
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_xlsxwriter.py#L23
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_xlsxwriter.py#L60
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_xlsxwriter.py#L68
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_xlsxwriter.py#L75
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L94
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L111
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L125
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L147
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L173
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L203
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L245
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L319
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_openpyxl.py#L374
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_style.py#L43
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_style.py#L138
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_style.py#L178
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_style.py#L240
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_style.py#L270
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_odswriter.py#L19
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_odswriter.py#L36
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_odswriter.py#L38
abstract-class-instantiated:
Abstract class 'ExcelWriter' with abstract methods instantiated
https://github.com/pandas-dev/pandas/blob/33f4f7b57b46bb295c1b71f3890377a5e541122e/pandas/tests/io/excel/test_odswriter.py#L45

Effect on pytest:
The following messages are now emitted:

abstract-class-instantiated:
Abstract class 'DontReadFromInput' with abstract methods instantiated
https://github.com/pytest-dev/pytest/blob/ca40380e99c2cdaab1d0c041f9f28cff37ef8ff9/src/_pytest/capture.py#L344

This comment was generated for commit e600117

clavedeluna · 2023-01-27T12:20:20Z

I'm still stumped by

abstract-class-instantiated:
Abstract class 'DontReadFromInput' with abstract methods instantiated
https://github.com/pytest-dev/pytest/blob/ca40380e99c2cdaab1d0c041f9f28cff37ef8ff9/src/_pytest/capture.py#L344

I tried to reproduce it locally bit that msg doesn't get raised. Maybe a maintainer has a better idea?

DanielNoord · 2023-01-29T12:53:13Z

Have you checked wether TextIO has any "abstract" methods?

clavedeluna · 2023-01-30T11:07:43Z

Have you checked wether TextIO has any "abstract" methods?

It does ... while I can see DontReadFromInput getting some abstract-method msg raised, I'm not sure why we'd raise abstract-class-instantiated? Maybe I'm missing something subtle and the primer is right.

DanielNoord · 2023-02-04T13:27:21Z

Have you checked wether TextIO has any "abstract" methods?

It does ... while I can see DontReadFromInput getting some abstract-method msg raised, I'm not sure why we'd raise abstract-class-instantiated? Maybe I'm missing something subtle and the primer is right.

I don't understand this comment. If DontReadFromInput has abstract methods that don't get implemented than it is correct that we raise this here as well right?

clavedeluna · 2023-02-07T11:21:56Z

Maybe I'm misunderstanding the purpose of the msg abstract-class-instantiated versus msg abstract-method? What I understand from the docs is that the former is telling a user "don't directly instantiate a class with abc.ABCMeta and with abstract-methods", but if the class instantiated is inheriting from one of these, but the instantiated class DOES implement the abstract methods, as in the case of DontReadFromInput then it's fine.

Anyway, if I'm wrong, then this PR should be able to be merged, I think everything asked is complete?

DanielNoord · 2023-02-12T11:27:18Z

Maybe I'm misunderstanding the purpose of the msg abstract-class-instantiated versus msg abstract-method? What I understand from the docs is that the former is telling a user "don't directly instantiate a class with abc.ABCMeta and with abstract-methods", but if the class instantiated is inheriting from one of these, but the instantiated class DOES implement the abstract methods, as in the case of DontReadFromInput then it's fine.

Anyway, if I'm wrong, then this PR should be able to be merged, I think everything asked is complete?

But isn't one of the abstract methods of DontReadFromInput missing from the final implementation?

github-actions · 2023-10-29T02:06:55Z

This PR needs take over because because it has been open 8 weeks with no activity.

DanielNoord · 2023-12-05T13:47:53Z

Closing this as it seems to have stalled.

This comment has been minimized.

Sign in to view

DanielNoord requested changes Jan 2, 2023

View reviewed changes

pylint/checkers/base/basic_error_checker.py Show resolved Hide resolved

pylint/checkers/base/basic_error_checker.py Outdated Show resolved Hide resolved

clavedeluna force-pushed the 3060-abstract branch from 45e7296 to 87fb10c Compare January 2, 2023 13:39

clavedeluna requested a review from Pierre-Sassoulas January 3, 2023 12:29

clavedeluna added the Blocked 🚧 Blocked by a particular issue label Jan 5, 2023

DanielNoord added False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Blocked 🚧 Blocked by a particular issue labels Jan 9, 2023

clavedeluna force-pushed the 3060-abstract branch from 87fb10c to aa898b4 Compare January 9, 2023 11:55

This comment has been minimized.

Sign in to view

clavedeluna requested a review from DanielNoord January 9, 2023 13:32

DanielNoord requested changes Jan 9, 2023

View reviewed changes

clavedeluna force-pushed the 3060-abstract branch from aa898b4 to 64d81ad Compare January 18, 2023 14:29

clavedeluna requested a review from DanielNoord January 18, 2023 14:29

DanielNoord reviewed Jan 18, 2023

View reviewed changes

clavedeluna force-pushed the 3060-abstract branch from 64d81ad to 1dac582 Compare January 19, 2023 11:21

This comment has been minimized.

Sign in to view

DanielNoord requested changes Jan 22, 2023

View reviewed changes

clavedeluna force-pushed the 3060-abstract branch from 1dac582 to bf04d87 Compare January 24, 2023 19:02

This comment has been minimized.

Sign in to view

clavedeluna added 6 commits January 26, 2023 08:45

Fix abstract-class-instantiated false positive when class defines…

2860dae

… ``__new__`` method. Closes pylint-dev#3060

code review

39a5726

add func test for coverage

54da0b9

add more tests

fafda6b

add no cover

d8dc242

remove qname check

e600117

clavedeluna force-pushed the 3060-abstract branch from bf04d87 to e600117 Compare January 26, 2023 11:45

clavedeluna requested a review from DanielNoord February 8, 2023 13:36

jacobtylerwalls added the Waiting on author Indicate that maintainers are waiting for a message of the author label Sep 3, 2023

github-actions bot added the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Oct 29, 2023

DanielNoord closed this Dec 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `abstract-class-instantiated` false positive for `new` method. #8006

Fix `abstract-class-instantiated` false positive for `new` method. #8006

clavedeluna commented Dec 31, 2022

codecov bot commented Dec 31, 2022 •

edited

Loading

This comment has been minimized.

clavedeluna commented Jan 5, 2023

This comment has been minimized.

clavedeluna commented Jan 9, 2023

DanielNoord left a comment

DanielNoord Jan 9, 2023

clavedeluna Jan 11, 2023

DanielNoord Jan 11, 2023

Pierre-Sassoulas Jan 11, 2023

DanielNoord Jan 9, 2023

DanielNoord Jan 9, 2023

DanielNoord Jan 9, 2023

clavedeluna Jan 11, 2023

DanielNoord left a comment

This comment has been minimized.

DanielNoord Jan 22, 2023

clavedeluna Jan 24, 2023

This comment has been minimized.

clavedeluna commented Jan 24, 2023

github-actions bot commented Jan 26, 2023

clavedeluna commented Jan 27, 2023

DanielNoord commented Jan 29, 2023

clavedeluna commented Jan 30, 2023

DanielNoord commented Feb 4, 2023

clavedeluna commented Feb 7, 2023

DanielNoord commented Feb 12, 2023

github-actions bot commented Oct 29, 2023

DanielNoord commented Dec 5, 2023

		@@ -467,8 +496,16 @@ def _check_inferred_class_is_abstract(
		return

		if metaclass.qname() in ABC_METACLASSES:

Fix abstract-class-instantiated false positive for __new__ method. #8006

Fix abstract-class-instantiated false positive for __new__ method. #8006

Conversation

clavedeluna commented Dec 31, 2022

Type of Changes

Description

codecov bot commented Dec 31, 2022 • edited Loading

Codecov Report

This comment has been minimized.

clavedeluna commented Jan 5, 2023

This comment has been minimized.

clavedeluna commented Jan 9, 2023

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

clavedeluna commented Jan 24, 2023

github-actions bot commented Jan 26, 2023

clavedeluna commented Jan 27, 2023

DanielNoord commented Jan 29, 2023

clavedeluna commented Jan 30, 2023

DanielNoord commented Feb 4, 2023

clavedeluna commented Feb 7, 2023

DanielNoord commented Feb 12, 2023

github-actions bot commented Oct 29, 2023

DanielNoord commented Dec 5, 2023

Fix `abstract-class-instantiated` false positive for `new` method. #8006

Fix `abstract-class-instantiated` false positive for `new` method. #8006

codecov bot commented Dec 31, 2022 •

edited

Loading