Skip to content

Detect duplicate method names in CI #505

Closed
python/cpython
#109161
Closed
Detect duplicate method names in CI#505
python/cpython
#109161

Description

@erlend-aasland
erlend-aasland
opened on Jun 27, 2023

[...] Maybe we can just use pyflakes in a CI job, only looking for this specific kind of issue?

Originally posted by @vstinner in python/cpython#105560 (comment)

Activity

  1. hugovk commented on Jun 27, 2023

    @hugovk
    hugovk
    on Jun 27, 2023
    Member

    We can do something like this with Flake8's F811 check:

    Currently detects 132 things:

    Details 
    Lib/collections/__init__.py:338:5: F811 redefinition of unused 'OrderedDict' from line 83
Lib/collections/__init__.py:538:5: F811 redefinition of unused '_count_elements' from line 531
Lib/ctypes/__init__.py:305:20: F811 redefinition of unused 'pointer' from line 255
Lib/functools.py:226:5: F811 redefinition of unused 'cmp_to_key' from line 206
Lib/functools.py:266:5: F811 redefinition of unused 'reduce' from line 237
Lib/functools.py:342:5: F811 redefinition of unused 'partial' from line 276
Lib/functools.py:643:5: F811 redefinition of unused '_lru_cache_wrapper' from line 526
Lib/heapq.py:587:5: F811 redefinition of unused '_heapreplace_max' from line 191
Lib/heapq.py:591:5: F811 redefinition of unused '_heapify_max' from line 198
Lib/heapq.py:595:5: F811 redefinition of unused '_heappop_max' from line 181
Lib/idlelib/run.py:639:5: F811 redefinition of unused 'main' from line 110
Lib/multiprocessing/synchronize.py:46:1: F811 redefinition of unused 'SemLock' from line 28
Lib/pickle.py:1769:5: F811 redefinition of unused 'PicklingError' from line 77
Lib/pickle.py:1769:5: F811 redefinition of unused 'UnpicklingError' from line 84
Lib/poplib.py:454:5: F811 redefinition of unused 'sys' from line 19
Lib/socket.py:563:5: F811 redefinition of unused 'array' from line 551
Lib/statistics.py:1231:5: F811 redefinition of unused '_normal_dist_inv_cdf' from line 1155
Lib/test/test_buffer.py:767:5: F811 redefinition of unused 'genslices' from line 702
Lib/test/test_buffer.py:768:5: F811 redefinition of unused 'genslices_ndim' from line 706
Lib/test/test_buffer.py:769:5: F811 redefinition of unused 'permutations' from line 21
Lib/test/test_buffer.py:4725:5: F811 redefinition of unused 'test_multiple_inheritance_buffer_last' from line 4697
Lib/test/test_capi/test_misc.py:1826:9: F811 redefinition of unused 'json' from line 9
Lib/test/test_capi/test_misc.py:1905:9: F811 redefinition of unused 'json' from line 9
Lib/test/test_ctypes/test_arrays.py:192:13: F811 redefinition of unused 'T' from line 189
Lib/test/test_ctypes/test_arrays.py:195:13: F811 redefinition of unused 'T' from line 192
Lib/test/test_ctypes/test_arrays.py:204:13: F811 redefinition of unused 'T' from line 200
Lib/test/test_ctypes/test_arrays.py:208:13: F811 redefinition of unused 'T' from line 204
Lib/test/test_ctypes/test_arrays.py:212:13: F811 redefinition of unused 'T' from line 208
Lib/test/test_ctypes/test_functions.py:49:13: F811 redefinition of unused 'X' from line 44
Lib/test/test_ctypes/test_functions.py:53:13: F811 redefinition of unused 'X' from line 49
Lib/test/test_ctypes/test_functions.py:57:13: F811 redefinition of unused 'X' from line 53
Lib/test/test_curses.py:1371:5: F811 redefinition of unused 'test_move_left' from line 1362
Lib/test/test_dataclasses.py:589:13: F811 redefinition of unused 'A' from line 584
Lib/test/test_dataclasses.py:599:13: F811 redefinition of unused 'A' from line 589
Lib/test/test_dataclasses.py:753:21: F811 redefinition of unused 'Point' from line 744
Lib/test/test_dataclasses.py:764:21: F811 redefinition of unused 'Point' from line 753
Lib/test/test_dataclasses.py:774:17: F811 redefinition of unused 'C' from line 769
Lib/test/test_dataclasses.py:2011:9: F811 redefinition of unused 'C' from line 2005
Lib/test/test_dataclasses.py:2570:13: F811 redefinition of unused 'C' from line 2561
Lib/test/test_dataclasses.py:2579:13: F811 redefinition of unused 'C' from line 2570
Lib/test/test_dataclasses.py:2588:13: F811 redefinition of unused 'C' from line 2579
Lib/test/test_dataclasses.py:3005:13: F811 redefinition of unused 'C' from line 2997
Lib/test/test_dataclasses.py:3011:9: F811 redefinition of unused 'C' from line 3005
Lib/test/test_dataclasses.py:4282:13: F811 redefinition of unused 'A' from line 4277
Lib/test/test_dataclasses.py:4287:13: F811 redefinition of unused 'A' from line 4282
Lib/test/test_dataclasses.py:4429:13: F811 redefinition of unused 'A' from line 4420
Lib/test/test_dataclasses.py:4438:13: F811 redefinition of unused 'A' from line 4429
Lib/test/test_grammar.py:1222:9: F811 redefinition of unused 'time' from line 1221
Lib/test/test_grammar.py:1226:9: F811 redefinition of unused 'path' from line 1225
Lib/test/test_grammar.py:1226:9: F811 redefinition of unused 'argv' from line 1225
Lib/test/test_grammar.py:1227:9: F811 redefinition of unused 'path' from line 1226
Lib/test/test_grammar.py:1227:9: F811 redefinition of unused 'argv' from line 1226
Lib/test/test_grammar.py:1610:9: F811 redefinition of unused 'sys' from line 8
Lib/test/test_import/__init__.py:541:13: F811 redefinition of unused 'x' from line 536
Lib/test/test_keywordonlyarg.py:173:13: F811 redefinition of unused 'f' from line 169
Lib/test/test_monitoring.py:973:5: F811 redefinition of unused 'test_line_then_instruction' from line 950
Lib/test/test_monitoring.py:978:5: F811 redefinition of unused 'test_instruction_then_line' from line 955
Lib/test/test_pkg.py:200:9: F811 redefinition of unused 't5' from line 193
Lib/test/test_subclassinit.py:233:13: F811 redefinition of unused 'MyClass' from line 219
Lib/test/test_subclassinit.py:244:9: F811 redefinition of unused 'MyClass' from line 233
Lib/test/test_subclassinit.py:266:9: F811 redefinition of unused 'MyClass' from line 257
Lib/test/test_tokenize.py:12:1: F811 redefinition of unused 'os_helper' from line 9
Lib/test/test_traceback.py:921:1: F811 redefinition of unused 'CPythonTracebackErrorCaretTests' from line 910
Lib/test/test_unittest/testmock/testpatch.py:1960:13: F811 redefinition of unused 'test' from line 1950
Lib/test/test_yield_from.py:921:9: F811 redefinition of unused 'two' from line 890
Lib/test/test_zoneinfo/test_zoneinfo.py:1109:9: F811 redefinition of unused '_add' from line 1081
Lib/test/test_zoneinfo/test_zoneinfo.py:[113](https://github.com/hugovk/cpython/actions/runs/5390304327/jobs/9785454219#step:4:113)3:9: F811 redefinition of unused '_add' from line 1109
Lib/test/test_zoneinfo/test_zoneinfo.py:1158:9: F811 redefinition of unused '_add' from line 1133
Lib/test/test_zoneinfo/test_zoneinfo.py:1182:9: F811 redefinition of unused '_add' from line 1158
Lib/test/test_zoneinfo/test_zoneinfo.py:1195:9: F811 redefinition of unused '_add' from line 1182
Lib/test/test_zoneinfo/test_zoneinfo.py:1208:9: F811 redefinition of unused '_add' from line 1195
Lib/test/test_zoneinfo/test_zoneinfo.py:1229:9: F811 redefinition of unused '_add' from line 1208
Lib/test/test_zoneinfo/test_zoneinfo.py:1262:9: F811 redefinition of unused '_add' from line 1229
Lib/test/test_zoneinfo/test_zoneinfo.py:1285:9: F811 redefinition of unused '_add' from line 1262
Lib/test/time_hashlib.py:60:5: F811 redefinition of unused 'creatorFunc' from line 9
Lib/tkinter/__init__.py:98:6: F811 redefinition of unused '_flatten' from line 87
Lib/tkinter/__init__.py:120:6: F811 redefinition of unused '_cnfmerge' from line 102
Lib/warnings.py:564:5: F811 redefinition of unused 'warn' from line 296
Lib/warnings.py:564:5: F811 redefinition of unused 'warn_explicit' from line 342
Tools/c-analyzer/c_parser/parser/_func_body.py:46:1: F811 redefinition of unused 'parse_function_body' from line 41
Tools/c-analyzer/distutils/msvccompiler.py:327:5: F811 redefinition of unused 'MacroExpander' from line 98
Tools/cases_generator/lexer.py:253:5: F811 redefinition of unused 'sys' from line 6

  2. hugovk commented on Jun 27, 2023

    @hugovk
    hugovk
    on Jun 27, 2023
    Member

    Or the same thing using Ruff to check F811, which is quicker:

    Time to install and run pre-commit on CI:

  3. sobolevn commented on Jun 28, 2023

    @sobolevn
    sobolevn
    on Jun 28, 2023
    Member

    Going throught the list. I found several entries that are real problems (by "real problems" I mean: things that alter the runtime behavior in unexpected ways):

    1. Lib/test/test_traceback.py:921 test_traceback does not run one set of CPythonTracebackErrorCaretTests cpython#106185
    2. Lib/test/test_monitoring.py test_monitoring has duplicated tests cpython#106193
    3. Lib/test/test_curses.py:1371 test_curses has duplicated tests cpython#106194
    4. Lib/test/test_buffer.py:4725 test_multiple_inheritance_buffer_last is duplicated in test_buffer cpython#106197

    Plus, there are multiple imports that can be removed. I will send a PR for that later.
    But, the amount of things that are just false positives is overwhelming.

  4. hugovk commented on Jun 28, 2023

    @hugovk
    hugovk
    on Jun 28, 2023
    Member

    Thanks for checking these. We should backport the test fixes to at least 3.11.

    And maybe for the security-only releases too? It's usually better to run tests than accidentally skip them :)

  5. AlexWaygood commented on Jun 28, 2023

    @AlexWaygood
    AlexWaygood
    on Jun 28, 2023
    Member

    I'd suggest that we only run pyflakes/the Ruff equivalent on Lib/test/. Accidentally skipping tests due to duplicate method names (or duplicate class names) is a common problem that we've hit multiple times in the past, so it would be good to have a CI check to guard against it. But I don't think we have the same issue with other Python files in the CPython repo (which are generally tested more rigorously than the tests themselves).

    But, the amount of things that are just false positives is overwhelming.

    I looked at this a while back -- I think a common source of false positives was things like this in tests:

    def test_bad_class(self):
    with self.assertRaises(TypeError):
        class C(BadBaseClass): pass
    with self.assertRaises(TypeError):
        class C(OtherBadBadClass): pass  # pyflakes reports redefinition of unused class "C"

    That kind of thing is trivial to resolve -- the second C definition can just be renamed to C1 in the test.

  6. sobolevn commented on Jun 28, 2023

    @sobolevn
    sobolevn
    on Jun 28, 2023
    Member

    I think that our own ast-based linter is better:

    1. It is quite easy to write
    2. We can fully customize it
    3. Many core-devs have expertise in ast
    4. The problem does actually exist
    5. We won't have to adapt any existing code to fit our rules (except ones I've fixed in the PRs above)

    I can write a prototype by today's evening (right after my lecture today).

  7. AlexWaygood commented on Jun 28, 2023

    @AlexWaygood
    AlexWaygood
    on Jun 28, 2023
    Member

    I think that our own ast-based linter is better:

    I predict that at some point somebody will propose writing tests for the custom linter, at which point we'll be writing a test for the test testing the tests 😆

    But, sounds good!

  8. vstinner commented on Jun 28, 2023

    @vstinner
    vstinner
    on Jun 28, 2023
    Member

    Thanks for checking these. We should backport the test fixes to at least 3.11. And maybe for the security-only releases too? It's usually better to run tests than accidentally skip them :)

    Sometimes, when tests which were disabled for long time are enabled again, they start to fail randomly and need be fixed. I dislike the idea of enabling in stable branches.

  9. hugovk commented on Jun 28, 2023

    @hugovk
    hugovk
    on Jun 28, 2023
    Member

    I slightly prefer using an existing linter (if suitable) than a custom one:

    • less stuff for us to maintain
    • well tested
    • many existing tools are well known in the community
    • usually highly configurable

    Although potentially they might not run on pre-release Python code that is under development, like failing to parse new syntax.

  10. sobolevn commented on Jun 28, 2023

    @sobolevn
    sobolevn
    on Jun 28, 2023
    Member

    I remembered that @vstinner already had a prototype, so here's mine based on python/cpython#105560 (comment)

    Code: https://gist.github.com/sobolevn/967ac61b8b2575e26a33f5a6bf5df201

    Output:

    » ./python.exe check_test_duplicates.py Lib/test                          
Lib/test/test_curses.py:1371 Found duplicate method test_move_left in TextboxTest class
Lib/test/test_monitoring.py:973 Found duplicate method test_line_then_instruction in TestInstallIncrementallly class
Lib/test/test_monitoring.py:978 Found duplicate method test_instruction_then_line in TestInstallIncrementallly class
Lib/test/test_traceback.py:921 Found duplicate CPythonTracebackErrorCaretTests class
Lib/test/test_importlib/test_util.py:614 Found duplicate MagicNumberTests class
Lib/test/test_importlib/test_abc.py:148 Found duplicate MetaPathFinder class
Lib/test/test_importlib/test_abc.py:167 Found duplicate PathEntryFinder class
Lib/test/test_importlib/test_abc.py:218 Found duplicate ResourceLoader class
Lib/test/test_importlib/test_abc.py:238 Found duplicate InspectLoader class
Lib/test/test_importlib/test_abc.py:268 Found duplicate ExecutionLoader class
Lib/test/test_importlib/test_abc.py:622 Found duplicate SourceLoader class

    Cases listed in Lib/test/test_importlib/test_abc.py are indeed duplicates, but they use very interesting magic. I can addapt my solution to fix that.

  11. gpshead commented on Jun 28, 2023

    @gpshead
    gpshead
    on Jun 28, 2023
    Member

    For tests specifically, couldn't a unittest.TestCase metaclass be made that detects duplicate method definitions at class definition time? (turning that into at least a warning if not an error)

  12. sobolevn commented on Jun 28, 2023

    @sobolevn
    sobolevn
    on Jun 28, 2023
    Member

    @gpshead technically, we can. It might break some 3rd party unittest plugin that use metaclasses for their own stuff, but people can eventually accommodate them.

    But, unittest.TestCase won't solve duplicated classes. We can try tweaking unittest.Loader for that as well.

  13. vstinner commented on Jun 28, 2023

    @vstinner
    vstinner
    on Jun 28, 2023
    Member

    @gpshead: i wrote python/cpython#105560 metaclass to detect duplicates.

  14. hugovk commented on Sep 8, 2023

    @hugovk
    hugovk
    on Sep 8, 2023
    Member

    Please see python/cpython#109161 to check for F811 with Ruff via pre-commit.

  15. hugovk
    closed this as completedin cpython#109161on Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions