Simplify the logic handling the EvaluationMethods mixin class for Expression

[jdemeyer]

Depends on #20825
Depends on #20686

CC: @nthiery @tscrim

Component: symbolics

Author: Nicolas M. Thiéry

Branch/Commit: u/nthiery/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes @ 5619a7d

Issue created by migration from https://trac.sagemath.org/ticket/20859

[jdemeyer]

Dependencies: #20825, #20686

[jdemeyer]

Branch: u/nthiery/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes

[jdemeyer]

New commits:

44e80b3	EvaluationMethods should be a new-style class
9a1ff6e	Improve getattr_from_other_class
744ffa6	Test that static methods work
f5361d9	Improve _sage_src_lines_()
782f140	20686: minor comments improvements
dc84d88	Remove comments about "private" attributes with two underscores
8a4f48d	Abstract away category lookup in cdef method getattr_from_category
2e93afe	Merge branch 'u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes' of trac.sagemath.org:sage into t/20686/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes
5619a7d	20686: simplified the logic handling the EvaluationMethods mixin class for Expression's (internally backward incompatible)

[jdemeyer]

Commit: 5619a7d

[jdemeyer]

comment:3

You really should use new-style classes. Old-style classes exist only for backwards compatibility and will be gone in Python 3.

[jdemeyer]

comment:4

I just moved your branch from #20686 here. Feel free to rewrite history to make it depend only on #20825.

[jdemeyer]

comment:5

The use of cls.__dict__ seems to preclude the use of inheritance since cls.__dict__ does not do MRO lookup while dir(cls) does.

[nthiery]

comment:6

Thanks for creating the ticket and splitting of the commit! I am fine with this depending on #20686.

[nthiery]

comment:7

Replying to @jdemeyer:

You really should use new-style classes. Old-style classes exist only for backwards compatibility and will be gone in Python 3.

Fun: from the same premises, we arrive at opposite conclusions :-)

Here is my logical chain:

In Python 3 we won't make the inheritance from object explicit: it would be
redundant to write:

        class XXXMethods(object):

Furthermore, for our XXXMethods classes, it does not matter whether
they are old style or new style classes (they just are bags of
methods). Thus it feels natural to use right away the Python 3
idiom. That's what we have been doing in all the categories.

[nthiery]

comment:8

Replying to @jdemeyer:

The use of cls.__dict__ seems to preclude the use of inheritance since cls.__dict__ does not do MRO lookup while dir(cls) does.

That's on purpose, for consistency with what we do in categories: the XXXMethods are meant to be pure mixins / bags of methods; they are not supposed to inherit from anything.

[jdemeyer]

comment:9

Replying to @nthiery:

XXXMethods are meant to be pure mixins / bags of methods;

Given that it's a class, I would expect it to behave like a class. If you insist that it should not behave like a class, then at least make it explicit and invent a new metaclass BagOfMethods which disallows inheritance for example.

they are not supposed to inherit from anything.

How is a random Sage developer supposed to know that? That's one thing that I really don't like about the category framework in general: it makes several assumptions which work fine most of the time, but can bite you badly (the automatic binding is another such one). It almost feels like a slightly different programming language (that class is not really a Python class, it's just a dict).

[jdemeyer]

comment:10

Replying to @nthiery:

Furthermore, for our XXXMethods classes, it does not matter whether
they are old style or new style classes

It might matter in more places than you think. There will be some porting effort needed to transition from old-style classes to new-style classes (some issues came up in #20686). It would be better to do this now to avoid unexpected issues with Python 3.

For this reason, I am very against this change:

-    class EvaluationMethods(object):
+    class EvaluationMethods:

[nthiery]

comment:11

Replying to @jdemeyer:

Replying to @nthiery:

Furthermore, for our XXXMethods classes, it does not matter whether
they are old style or new style classes

It might matter in more places than you think. There will be some porting effort needed to transition from old-style classes to new-style classes (some issues came up in #20686). It would be better to do this now to avoid unexpected issues with Python 3.

For this reason, I am very against this change:

-    class EvaluationMethods(object):
+    class EvaluationMethods:

I already did tests with inheriting from object in some XXXMethods,
and the category framework kept working the exact same way. I am
therefore convinced there won't be anything to change for that
specific aspect for Python 3.

Furthermore, the more consistent things will be across the library,
the easier the porting will be.

Cheers,
Nicolas

[jdemeyer]

comment:12

Replying to @nthiery:

I already did tests with inheriting from object in some XXXMethods,
and the category framework kept working the exact same way.

So you did some limited testing now and it worked. That's a good thing, but it doesn't guarantee that it will work in all cases now and in the future. As a general principle, we should try to move Sage as close to Python 3 as possible. Python 3 has only new-style classes, so we should use new-style classes.

Furthermore, the more consistent things will be across the library, the easier the porting will be.

True, but besides the point.

And of course, this just means that we should use new-style classes everywhere in Sage.

[nthiery]

comment:13

Replying to @jdemeyer:

Given that it's a class, I would expect it to behave like a
class. If you insist that it should not behave like a class,
then at least make it explicit and invent a new metaclass
BagOfMethods which disallows inheritance for example.

Using a metaclass would mean one more piece of purely technical
syntax, which is one more chance for the programmer to forget
something. Still you have a good point here: the infrastructure should
check that XXXMethods inherits from nothing, and raise an explanatory
error message if not.

How is a random Sage developer supposed to know that? That's one
thing that I really don't like about the category framework in
general: it makes several assumptions which work fine most of the
time, but can bite you badly (the automatic binding is another such
one). It almost feels like a slightly different programming language
(that class is not really a Python class, it's just a dict).

That's the problem with every framework: a Django programmer needs to
learn some about Django, etc, either from example by looking at
existing code, or by reading the documentation.

Now does Sage really need such a framework? I am convinced enough
about it to have invested altogether one solid year of work into
it. Which does not mean I am not completely wrong :-)

I also believe in concise syntax with minimal boilerplate. Of course
the price is additional complexity for those developers like you that
not only use, but also work on the infrastructure itself.

Cheers,
Nicolas

[nthiery]

comment:14

Replying to @jdemeyer:

And of course, this just means that we should use new-style classes everywhere in Sage.

Everywhere, unless we have a good reason to be convinced this does not make a difference.

Would we really gain something worth the trouble by adding an explicit inheritance from object in all 292 XXXMethods classes in Sage, and then removing them after the switch to Python3, with all the risks of induced syntactical conflicts, when we know that they are treated uniformly?

There are real issues in the switch to Python 3, and I believe this is not one.

Cheers,

[jdemeyer]

comment:15

Replying to @nthiery:

Using a metaclass would mean one more piece of purely technical
syntax, which is one more chance for the programmer to forget
something. Still you have a good point here: the infrastructure should
check that XXXMethods inherits from nothing, and raise an explanatory
error message if not.

I'm not saying a metaclass is the right solution, it was just some proposal. That being said, you could think if there are other ways in which your "bag-of-methods" classes differ from "real" classes. Such differences might be handled cleanly by such metaclass. For example, you could use the metaclass to disable automatic binding of methods or to disallow instantiation of the class.

[jdemeyer]

comment:16

Replying to @nthiery:

I also believe in concise syntax with minimal boilerplate.

I know :-) but sometimes that concise syntax just hides all kinds of stuff which is better made explicit. Like I said before: if I see a class, I expect it to behave like a class.

[jdemeyer]

comment:17

The hypothetical metaclass would also serve as entry point to documentation. If I see

class ElementMethods(BagOfStuff):
    ....

somewhere and want to understand what it does, I could do

sage: BagOfStuff?

which will hopefully explain that this class isn't really a class, but just some syntax to define a dict.

I don't know those XXXMethods classes well enough to say whether it's the right solution, but it's certainly something you should consider.

And this BagOfStuff would of course be a new-style class, rendering the other discussion obsolete :-)

[jdemeyer]

comment:18

I know I am repeating myself, but just to clear, I think there are two possible ways:

1. Either you use a plain class XXXMethods but then it should behave like a class.

2. Or it's something else, say class XXXMethods(BagOfStuff) and then you can have all kinds of strange behaviour that you document in BagOfStuff?.