[rocky]

Use slots in class Definition; "ruff" changed files definition.py, symbols.py, and test_definition.py.

Additional changes to drop support for Python 3.11.

(Reworked PR from #2130)

[tombouctou]

The ruff modernisation part looks fine, and CI is green. Two things in the slots part look like blockers to me, though: as written the slots make each Definition larger, not smaller, and previously saved user definitions stop loading.

Blocking

1. "__dict__" in Definition.__slots__ makes each Definition bigger

Measured on Python 3.14 against this PR's base commit (0fcb088e2), allocating 20,000 Definition("Global`q") under tracemalloc:

Variant	bytes per Definition
base (no slots)	808
this PR (slots + __dict__)	936
slots with a real literal_index slot, no __dict__	824

• The only thing that needs __dict__ is the literal_index cache (self.__dict__.setdefault("literal_index", {}) and two other places in definitions.py).
• Suggested fix: add a literal_index slot, initialise it to None in __init__ and create the dict lazily. I only measured the eager-dict variant (824); the lazy one should land a little under base.
• Even then the saving is at best a few dozen bytes per definition: modern CPython already stores instance attributes compactly, so the memory case is weak.
• Evaluation speed showed no difference beyond noise on my machine.

2. Old pickles of user definitions no longer load

A get_user_definitions() string produced on master and fed to set_user_definitions() on this branch fails on first use:

AttributeError: 'mathics.core.definitions.Definition' object has no attribute 'ownvalues'

• Cause: the old pickle state is a plain dict, which unpickling writes into the instance __dict__, where the slot descriptors hide it.
• New-format pickles load fine on both this branch and master; only old → new is broken.
• This affects anything that persists that string across an upgrade (I believe mathics-django does so in its sessions, but I did not check that repo). The builtin_filename cache is only protected by its mtime check and could hit the same error.
• Suggested fix: a __setstate__ that accepts both the plain dict and the (dict, slots) form. Dropping __dict__ (item 1) without it would turn this into a different error rather than fix it.

Should fix

3. Typos introduced by c686a4e, all in definitions.py

• in the bfield with the attribute
• we couldb pick the options
• autoload files,bbb and extension modules
• "A Definition is a collection of Rules and attributes are associated with Symbol" — dropping "which" broke the sentence.
• "Definitionobjects" — missing space.

4. Definitions.__slots__ without __dict__ is an API break for little gain

• There is one Definitions per session, so there is no memory to save.
• Per-instance monkeypatching stops working, which is why test_config_values.py had to patch the class instead (the patched test itself is fine).
• Any front-end or pymathics module that sets an ad-hoc attribute on definitions will now raise. In-tree, every assignment is covered by the slot list.
• I'd either drop the slots on Definitions or note the break in CHANGES.rst.

Minor

• symbols.py still imports FrozenSet from typing, now unused; Optional is half-converted (Atom.equal2 and original: Optional["Atom"] remain).
• test/test_session.py gains import_and_load_builtins(). It looks unrelated to slots (presumably makes the file runnable standalone) — worth a mention in the description.
• The alphabetical reordering in __init__ separates the "TraceEvaluation uses these" comment from trace_show_rewrite, and puts rule_index and its comment between ownvalues and subvalues. Mixing the reorder into the slots change also makes the diff harder to read.
• Title typo: "definitons.py".
• itertools.pairwise and X | None are fine with requires-python >= 3.11.

[mmatera]

The ruff modernisation part looks fine, and CI is green.
I Agree

Two things in the slots part look like blockers to me, though: as written the slots make each Definition larger, not smaller, and previously saved user definitions stop loading.

The first part is what I see as more problematic. The second is just a matter of setting the right __setter__ method.

Blocking

1. "__dict__" in Definition.__slots__ makes each Definition bigger

Measured on Python 3.14 against this PR's base commit (0fcb088e2), allocating 20,000 Definition("Global`q") under tracemalloc:
Variant bytes per Definition
base (no slots) 808
this PR (slots + __dict__) 936
slots with a real literal_index slot, no __dict__ 824

Most of the attributes of the class are dictionaries. The part that I like about SLOTS is that prevents the (ab)use of monkeypatching. Most of these dict attributes could be packed into a "dispatch table": ownvalues are just the rules associated to the Definitions name, downvalues to those rules having the same head, upvalues are those that have the same lookup symbol, etc and so on. Then, this part of the complexity of the class would be moved to the dispatch table, and then the class could be more lightweight.

* The only thing that needs `__dict__` is the `literal_index` cache (`self.__dict__.setdefault("literal_index", {})` and two other places in `definitions.py`).

* Suggested fix: add a `literal_index` slot, initialise it to `None` in `__init__` and create the dict lazily. I only measured the eager-dict variant (824); the lazy one should land a little under base.

* Even then the saving is at best a few dozen bytes per definition: modern CPython already stores instance attributes compactly, so the memory case is weak.

* Evaluation speed showed no difference beyond noise on my machine.

2. Old pickles of user definitions no longer load

A get_user_definitions() string produced on master and fed to set_user_definitions() on this branch fails on first use:

AttributeError: 'mathics.core.definitions.Definition' object has no attribute 'ownvalues'

* Cause: the old pickle state is a plain dict, which unpickling writes into the instance `__dict__`, where the slot descriptors hide it.

* New-format pickles load fine on both this branch and master; only old → new is broken.

* This affects anything that persists that string across an upgrade (I believe mathics-django does so in its sessions, but I did not check that repo). The `builtin_filename` cache is only protected by its mtime check and could hit the same error.

* Suggested fix: a `__setstate__` that accepts both the plain dict and the `(dict, slots)` form. Dropping `__dict__` (item 1) without it would turn this into a different error rather than fix it.

Should fix

3. Typos introduced by c686a4e, all in definitions.py

* `in the bfield with the attribute`

* `we couldb pick the options`

* `autoload files,bbb and extension modules`

* "A Definition is a collection of `Rule`s and attributes are associated with `Symbol`" — dropping "which" broke the sentence.

* "`Definition`objects" — missing space.

4. Definitions.__slots__ without __dict__ is an API break for little gain

* There is one `Definitions` per session, so there is no memory to save.

* Per-instance monkeypatching stops working, which is why `test_config_values.py` had to patch the class instead (the patched test itself is fine).

* Any front-end or pymathics module that sets an ad-hoc attribute on `definitions` will now raise. In-tree, every assignment is covered by the slot list.

* I'd either drop the slots on `Definitions` or note the break in `CHANGES.rst`.

Here I would expect a larger improvement, not in memory, but in attribute access. I would like to make the cache access faster, and I expected this change do that. Have you measure it?

[rocky]

3. Typos introduced by c686a4e, all in definitions.py

Fixed in commit 963a893

test/test_session.py gains import_and_load_builtins(). It looks unrelated to slots (presumably makes the file runnable standalone) — worth a mention in the description.

Removed. This was an unintended merge mistake from a stray git pop.

The alphabetical reordering in init separates the "TraceEvaluation uses these" comment from trace_show_rewrite, and puts rule_index and its comment between ownvalues and subvalues. Mixing the reorder into the slots change also makes the diff harder to read.

I think this is technically incorrect. It happens that the two attributes just happen to fit in strict alphabetical order. I've added a space between these settings and made the comment more explicit though.

[rocky]

New-format pickles load fine on both this branch and master; only old → new is broken.

Right now, I don't believe we have pickle files to worry about.

[rocky]

The part that I like about SLOTS is that prevents the (ab)use of monkeypatching.

I believe one of the tests relies on monkey-patching.

Most of these dict attributes could be packed into a "dispatch table": ownvalues are just the rules associated to the Definitions name, downvalues to those rules having the same head, upvalues are those that have the same lookup symbol, etc and so on. Then, this part of the complexity of the class would be moved to the dispatch table, and then the class could be more lightweight.

The one place where I don't think this is right is where we have some sort of literal pattern. Here, a dictionary lookup is in fact the right thing.

Both of these come from @tombouctou's changes. But @tombouctou correct me if I am mistaken here. (I do make lots of mistakes.)

[rocky]

Here I would expect a larger improvement, not in memory, but in attribute access. I would like to make the cache access faster, and I expected this change do that. Have you measure it?

Both you @mmatera and @tombouctou are more facile at this than I am. I'd appreciate it if you'd tell me, and we'll decide what to do.

Personally, I just look at CI execution times and go by that, which admittedly is not very precise.

[rocky]

@mmatera and @tombouctou: I think I've addressed the issues or commented on them. But as you can see, I make mistakes. (AI got its training from me.)

So if I've missed something, there are further items, or something to discuss, chime in.

[mmatera]

We should care about how we are educating AI... :) Tomorrow I will try to run the benchmarks to see the impact of these changes.