symtable.Class.get_methods() returns non-methods

[JelleZijlstra]

Bug report

Bug description:

Running this file:

import symtable

st = symtable.symtable("""
class X:
    class Nested: pass
    type Alias = int
    x = (x for x in range(10))
    y = filter(lambda z: z % 2, range(10))
""", "mod", "exec")

cls = st.get_children()[0]
print(cls.get_methods())

Prints:

('Nested', 'Alias', 'genexpr', 'lambda')

None of these are methods.

@carljm and I noticed this as part of the work on PEP 649 (#119361 (comment)). My draft implementation excludes generated __annotate__ functions from the list of "methods", but perhaps all non-methods should be excluded.

CPython versions tested on:

3.12, CPython main branch

Operating systems tested on:

macOS

Linked PRs

• gh-119698: deprecate symtable.Class.get_methods #120148
• gh-119698: fix symtable.Class.get_methods and document its behaviour correctly #120151
• [3.12] gh-119698: fix symtable.Class.get_methods and document its behaviour correctly (#120151) #120776
• [3.13] gh-119698: fix symtable.Class.get_methods and document its behaviour correctly (GH-120151) #120777
• gh-119698: symtable: Fix merge race #120779
• gh-119698: fix a special case in symtable.Class.get_methods #121802
• gh-119698: deprecate symtable.Class.get_methods #121902
• [3.13] gh-119698: fix a special case in symtable.Class.get_methods (GH-121802) #121909
• [3.12] gh-119698: fix a special case in symtable.Class.get_methods (GH-121802) #121910

[serhiy-storchaka]

The symtable CLI

$ echo "
class X:
    class Nested: pass
    type Alias = int
    x = (x for x in range(10))
    y = filter(lambda z: z % 2, range(10))
" | ./python -m symtable

shows details:

symbol table for module from file '<stdin>':
    local symbol 'X': def_local

    symbol table for class 'X':
        local symbol 'Nested': def_local
        local symbol 'Alias': def_local
        local symbol 'x': def_local
        global_implicit symbol 'range': use
        local symbol 'y': def_local
        global_implicit symbol 'filter': use

        symbol table for class 'Nested':

        symbol table for type alias 'Alias':
            free symbol '__classdict__': use
            global_implicit symbol 'int': use

        symbol table for function 'genexpr':
            local symbol '.0': def_param
            local symbol 'x': use, def_local

        symbol table for function 'lambda':
            local symbol 'z': use, def_param

We see that get_methods() returns the names of nested symbol tables. Some symbol table names like "genexpr" and "lambda" has no relation to the name of any nested symbol. It is a question why get_methods() can be useful at all.

We can simply deprecate get_methods(). Or define it as returning names of local symbols that has corresponding symbol table with type "function". Then it may be worth to add also get_classes() and get_aliases(). What do you need?

[JelleZijlstra]

Then it may be worth to add also get_classes() and get_aliases(). What do you need?

I have never used this module and I don't know how people are using it; I ran into this problem because some tests failed while I was working on the PEP 649 implementation.

Deprecating this method seems like a reasonable path forward.

[JelleZijlstra]

cc @picnixz in case you're interested in other problems with the symtable module.

[picnixz]

Personally, I've never used it but I was interested in contributing to CPython so I started where I thought it was something not too hard and interesting. One use case that I have in mind is IntelliSense but I would expect those tools to actually use the AST instead (but since their code is private, I cannot say for sure).

If there is really no use-case, I'd suggest deprecating it (less things to maintain, easier for us). I've tried to search whether it's used or not on github but I've only seen this occurrence https://github.com/skulpt/skulpt/blob/8daae6db0d2834ae2e66f6407e00239d4a95ead1/skulpt.py#L101 being relevant (others are not I think). However, they are using the thing for Python 2 and are not claiming full Python 3 compatibility.

[picnixz]

I've implemented two suggestions so feel free to pick either of the two.

I decided to keep the PR fixing the interface since Carl and Jelle have reviewed it and the work was not that much.

[picnixz]

For the record, I've implemented get_aliases and get_classes using a more generic approach in a separate branch: https://github.com/picnixz/cpython/tree/feat-symtable-classes

Note that the branch actually contains a local copy of #120151 (and I will not open a PR yet).

[JelleZijlstra]

I would only add such methods if someone comes up with an actual use case.

[serhiy-storchaka]

I only have found the use of this method in the skulpt project, which is Python 2 only. I also prefer to deprecate it.

[picnixz]

To summarize, we would fix the method in 3.14 (and 3.12, 3.13, depending on the backport decision), deprecate it in 3.15 and remove it in 3.17? (and it's fine that we don't need the other methods)

[serhiy-storchaka]

Why not deprecate it in 3.14?

[picnixz]

Err..., maybe someone will be interested in using it since we fixed it? (I can add the deprecation in the same or a follow-up PR though)

[JelleZijlstra]

We should definitely do only one of deprecating and fixing, not both; it doesn't make sense to radically change the implementation of the method and then immediately deprecate it.

I would prefer to fix the method instead of deprecating it though. We have a solution that fixes the method (#120151), and I don't think maintaining the method in the future will be very costly. I think we should only deprecate the method if there's some strong reason we don't want it in the library in the future.

[serhiy-storchaka]

If we are going to fix get_methods(), we should also add get_classes(), get_aliases(), etc in the Class class. Then add the SymbolTable subclass Module with methods get_classes(), get_functions(), etc. Then perhaps add get_classes() and get_functions() also in the Function class, for local classes and functions.

Note that currently functions which return a limited set of names, such as get_parameters() and get_locals(), only filter names of symbols by their scope and flags. New get_methods(), get_classes(), etc, will be more complex, they need to look into two tables: symbols and symboltables. Too complex behavior for a feature that no one will possibly use.

[serhiy-storchaka]

I still prefer to deprecate this method. But it is fine if other core developers need it.

[JelleZijlstra]

I'm not strongly opposed to deprecating the method, but we now have a fixed implementation and I don't foresee the method causing a lot of problems in the future, so I don't see a strong cause for inconveniencing users by deprecating it.

[serhiy-storchaka]

It is pretty complex now, with complex tests and documentation, and I am not even sure that it is correct. It has high maintenance cost, and its usefulness is questionable.

Actually, I just found a bug in it:

[serhiy-storchaka]

import symtable
st = symtable.symtable("""
class X:
    x = (x for x in range(10))
    genexpr = 1
""", '?', 'exec')
cls = st.get_children()[0]
print(cls.get_methods())

(For some reasons Firefox crashed on my computer when paste from clipboard. After updating it this was fixed.)

[picnixz]

Ah I think I forgot that case where a symbol is called 'genexpr'. Actually, I also have this issue:

import symtable
st = symtable.symtable("""
class X:
    genexpr = (x for x in range(10))
""", '?', 'exec')
cls = st.get_children()[0]
print(cls.get_methods())

Ideally, if we could rename 'genexpr' by '.genexpr', it could make the name special. Because otherwise the following 'genexpr' would not be picked up... (I cannot just say that the identifier is invalid, for 'lambda' I can since it's a keyword but not for 'genexpr').

class X:
	def genexpr(): pass

[picnixz]

Re-opening until the fix of the fix (+backports) lands and the deprecation notice is merged.

[serhiy-storchaka]

I opened also #121914.