Some flags are not publicly exported by symtablemodule.c

[picnixz]

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

• gh-120029: export DEF_TYPE_PARAM compiler flag #120028
• gh-120029: make symtable.Symbol.__repr__ correctly reflect the compiler's flags  #120099
• gh-120029: refactor symtable.c (remove deprecated macros and update some values) #120218
• gh-120029: remove unused macros in symtable.c #120222

[JelleZijlstra]

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.

[picnixz]

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.

[JelleZijlstra]

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]].

[picnixz]

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?

[carljm]

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.

[picnixz]

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.

[carljm]

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.

[carljm]

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.

[JelleZijlstra]

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.

[picnixz]

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).

[JelleZijlstra]

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

[picnixz]

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?

[picnixz]

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