Skip to content

Some flags are not publicly exported by symtablemodule.c #120029

Closed
Closed
Some flags are not publicly exported by symtablemodule.c#120029
Labels
type-bugAn unexpected behavior, bug, or error

Description

@picnixz
picnixz
opened on Jun 4, 2024

Bug report

Bug description:

The symtablemodule.c file does not export DEF_COMP_ITER, DEF_TYPE_PARAM and DEF_COMP_CELL. Those flags seem to have been added after the original ones so they were probably missed/forgotten. Here is a MWE for DEF_TYPE_PARAM:

>>> import symtable
>>> s = symtable.symtable("class A[T]: pass", "?", "exec")
>>> s.get_children()[0].lookup('T')
<symbol 'T': LOCAL, DEF_LOCAL>

By the way, there are tests that are missing for those cases in test_symtable, so I can also add the corresponding test. I'm opening a PR now, but feel free to close it if you do not want to expose too many compiler flags (though, I fail to understand why you would do so).

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

Activity

  1. picnixz
    added
    type-bugAn unexpected behavior, bug, or error
    on Jun 4, 2024
  2. JelleZijlstra
    added a commit that references this issue on Jun 4, 2024

    gh-120029: export `DEF_TYPE_PARAM` compiler flag (#120028)

    ff1857d

  3. JelleZijlstra commented on Jun 4, 2024

    @JelleZijlstra
    JelleZijlstra
    on Jun 4, 2024
    Member

    Thanks! I merged your PR exposing DEF_TYPE_PARAM; that should indeed have been public.

    The other two sound related to comprehension inlining; cc @carljm.

  4. picnixz commented on Jun 4, 2024

    @picnixz
    picnixz
    on Jun 4, 2024
    MemberAuthor

    Yes, that's why I didn't touch them for now. I can easily find an example where exposing DEF_COMP_ITER can be useful (for instance, for IntelliSense so that PyCharm doesn't tell me "hey, your variable doesn't exist", but I'm not even sure it uses the symbol table) but I have no idea how to create a symbol with DEF_COMP_CELL.

  5. JelleZijlstra commented on Jun 4, 2024

    @JelleZijlstra
    JelleZijlstra
    on Jun 4, 2024
    Member

    I haven't checked the code recently but I assume it's used if there's an inner scope inside the listcomp, e.g. [(lambda: x) for x in [1]].

  6. picnixz commented on Jun 4, 2024

    @picnixz
    picnixz
    on Jun 4, 2024
    MemberAuthor

    I see. For now, I'll wait for Carl's opinion on whether the other two flags should be exported. By the way, there are actually two flags that are exported but not imported (and thus not exposed in repr), namely DEF_FREE and DEF_FREE_CLASS. Should they be exposed as well?

  7. carljm commented on Jun 4, 2024

    @carljm
    carljm
    on Jun 4, 2024
    Member

    My default position would be that the symtable module should accurately reflect the symbol table as seen by CPython, so I would export all of these and reflect them in the symbol repr.

  8. picnixz commented on Jun 5, 2024

    @picnixz
    picnixz
    on Jun 5, 2024
    MemberAuthor

    By the way, what is the purpose of the DEF_FREE flag? I've only found a single usage in inline_comprehension which is:

    int only_flags = comp_flags & ((1 << SCOPE_OFFSET) - 1);
...
only_flags &= ~DEF_FREE;

    but AFAICT, DEF_FREE is never set so clearing its corresponding bit is a no-op. Similarly, the macros GENERATOR and GENERATOR_EXPRESSION are defined but never used, so I'm wondering whether they are actually needed.

  9. barneygale
    added a commit that references this issue on Jun 5, 2024

    pythongh-120029: export `DEF_TYPE_PARAM` compiler flag (python#120028)

    f71ed10

  10. carljm commented on Jun 6, 2024

    @carljm
    carljm
    on Jun 6, 2024
    Member

    DEF_FREE is supposed to mark a name as free in a scope. But it looks to me like you're right, it's never set; instead in analyze_name we just directly set scope to FREE for free names. A quick examination of the git history suggests to me that DEF_FREE dates back very early in Python's history, but a rewrite of the compiler 19 years ago in 3e0055f rendered it unused, but kept it around?

    I'm currently investigating an issue (#119666) related to that line in inline_comprehension where we unset DEF_FREE, so I'll likely remove it in the PR to fix that issue. Or we can remove it separately. Personally it seems fine to me to just remove DEF_FREE altogether, since it's unused; keeping it around will just lead to confusion. It doesn't seem to be currently exposed in the Python symtable module. And it's defined in an internal-only header file. Curious what @JelleZijlstra thinks here.

    I don't have any context on the GENERATOR and GENERATOR_EXPRESSION macros, but if they are unused, I think they could be removed.

  11. carljm commented on Jun 6, 2024

    @carljm
    carljm
    on Jun 6, 2024
    Member

    I guess DEF_FREE is already visible in _symtable module, so it's possible someone is importing it from there. Maybe that's enough reason to keep it around, even if it's never used. Though I don't know why anyone would be importing it, since it's never set on a symbol.

  12. JelleZijlstra commented on Jun 6, 2024

    @JelleZijlstra
    JelleZijlstra
    on Jun 6, 2024
    Member

    I looked at the code too and agree DEF_FREE doesn't appear to be doing anything useful. It's publicly exposed but undocumented and has no conceivable use.

    I think we can remove it in 3.14.

  13. picnixz commented on Jun 7, 2024

    @picnixz
    picnixz
    on Jun 7, 2024
    MemberAuthor

    I have a question on that. Since removing DEF_FREE would cause a "hole" in the flags, am I allowed to change the order of the flags in such a way the USE flag is the first one?

    The rationale is that the repr() of a symbol uses a very specific order (actually, it's the order in which the DEF_* constants are imported) and with USE as the very first possible flag. However, USE = 2 << 3 and DEF_GLOBAL = 1. I'd suggest changing the current order as follows:

    /* current order */
#define DEF_GLOBAL 1         
#define DEF_LOCAL 2          
#define DEF_PARAM (2<<1)     
#define DEF_NONLOCAL (2<<2)  
#define USE (2<<3)           
#define DEF_FREE (2<<4)      
#define DEF_FREE_CLASS (2<<5)
#define DEF_IMPORT (2<<6)    
#define DEF_ANNOT (2<<7)     
#define DEF_COMP_ITER (2<<8) 
#define DEF_TYPE_PARAM (2<<9)
#define DEF_COMP_CELL (2<<10)

/* suggested order */
#define USE (1 << 0)
#define DEF_ANNOT (1 << 1)

#define DEF_IMPORT (1 << 2)
#define DEF_GLOBAL (1 << 3)
#define DEF_NONLOCAL (1 << 4)
#define DEF_LOCAL (1 << 5)

#define DEF_PARAM (1 << 6)     
#define DEF_TYPE_PARAM (1 << 7)

#define DEF_FREE_CLASS (1 << 8)
#define DEF_COMP_ITER (1 << 9) 
#define DEF_COMP_CELL (1 << 10)

    The SCOPE_OFFSET would be changed updated since we are removing a flag and changing their order (actually, just removing it is fine, (and we would have holes), but changing the order makes some weird assertion failures, probably because there are some assumptions I haven't caught yet).

  14. JelleZijlstra commented on Jun 7, 2024

    @JelleZijlstra
    JelleZijlstra
    on Jun 7, 2024
    Member

    What is the problem with leaving a hole? I'd rather not change the values without a compelling reason.

  15. picnixz commented on Jun 7, 2024

    @picnixz
    picnixz
    on Jun 7, 2024
    MemberAuthor

    What is the problem with leaving a hole? I'd rather not change the values without a compelling reason.

    Well.. I find it a bit weird to have a hole (but I can revert that change). Actually, I took the opportunity of removing that hold to change the order of the flags (so that the USE flag is at the top and not in the middle, since we anyway construct the repr() of symbol's flags with USE at the beginning, e.g., USE|DEF_...).

    I thought that it could have been a bit nicer to have flags grouped by what they are meant to represent (one specific reason is that I have the TYPE_PARAM flag way after the PARAM flag, which I find a bit weird).

    If you want to, I split the PR into two, with one PR only removing the deprecated values and another making the change order?

  16. JelleZijlstra
    added a commit that references this issue on Jun 12, 2024

    gh-120029: make `symtable.Symbol.__repr__` correctly reflect the comp…

    755dab7
  17. carljm
    added a commit that references this issue on Jun 12, 2024

    gh-120029: remove unused macros in ``symtable.c`` (#120222)

    32d3e05

  18. picnixz commented on Jun 12, 2024

    @picnixz
    picnixz
    on Jun 12, 2024
    MemberAuthor

    I think we are done with this one or is there something else to do?

  19. carljm
    closed this as completedon Jun 12, 2024
  20. mrahtz
    added 2 commits that reference this issue on Jun 30, 2024

    pythongh-120029: make `symtable.Symbol.__repr__` correctly reflect th…

    c7091e5

    pythongh-120029: remove unused macros in ``symtable.c`` (python#120222)

    7dea1af
  21. noahbkim
    added 3 commits that reference this issue on Jul 11, 2024

    pythongh-120029: export `DEF_TYPE_PARAM` compiler flag (python#120028)

    e9c0749

    pythongh-120029: make `symtable.Symbol.__repr__` correctly reflect th…

    d44891d

    pythongh-120029: remove unused macros in ``symtable.c`` (python#120222)

    2843168
  22. estyxx
    added 3 commits that reference this issue on Jul 17, 2024

    pythongh-120029: export `DEF_TYPE_PARAM` compiler flag (python#120028)

    8df8add

    pythongh-120029: make `symtable.Symbol.__repr__` correctly reflect th…

    620b931

    pythongh-120029: remove unused macros in ``symtable.c`` (python#120222)

    60e268e
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

    type-bugAn unexpected behavior, bug, or error

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions