Nested class name mangling can be wrong in case of double nesting

[hivert]

In the following class tree:

class Bla(UniqueRepresentation):
    class Bla1(UniqueRepresentation):
        class Bla11:
            Pass
    class Bla2:
        class Bla21:
            Pass

The names are set to

        sage: Bla.Bla1.__name__
        'Bla.Bla1'
        sage: Bla.Bla2.__name__
        'Bla.Bla2'
        sage: Bla.Bla2.Bla21.__name__
        'Bla.Bla2.Bla21'

But

        sage: Bla.Bla1.Bla11.__name__
        'Bla1.Bla11'

whereas one would expect 'Bla.Bla1.Bla11'
This breaks a lot of doc in categories and in particular in functorial constructions.

Apply

• attachment: trac9107_nesting_nested_classes.patch
• attachment: trac_9107_fix_cross_reference.patch

Depends on #12808

CC: @simon-king-jena @zabrocki

Component: categories

Author: Simon King, Nicolas M. Thiéry

Branch: 8b14e05

Reviewer: Volker Braun, Florent Hivert, Travis Scrimshaw

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

[simon-king-jena]

Dependencies: #12808

[simon-king-jena]

comment:2

I think we should make this depend on #12808, because it cythonises nested classes.

Here is my analysis:

1. In sage.misc.nested_class.modify_for_nested_pickling, only those attributes of a class are (recursively) renamed that are instances of type or of ClassType. However, ironically, instances of NestedClassMetaclass are ignored.
2. It is verified that the name of the to-be-changed class is identical with its name as an attribute of the calling class. But the renaming breaks the identity.

[simon-king-jena]

comment:3

I think the attached patch solves the problem. I get:

sage: class Bla(UniqueRepresentation):
....:     class Bla1(UniqueRepresentation):
....:         class Bla11:
....:             pass
....:     class Bla2:                   
....:         class Bla21:   
....:             pass
....:         
sage: Bla.Bla1.Bla11
<class __main__.Bla.Bla1.Bla11 at 0x46e7808>

The change is in modify_for_nested_pickle, which is called recursively. The idea is that the function should have an extra argument first_run, that is True by default. If the extra argument is False, then it is assumed that it is not applied for the first time.

Here: Since Bla.Bla1 is an instance of NestedClassMetaclass, modify_for_nested_pickle is called on Bla.Bla1.Bla11, resulting in Bla.Bla1.Bla11.__name__=='Bla1.Bla11'. However, since Bla is an instance of NestedClassMetaclass as well, the function is applied to Bla.Bla1 and thus recursively to Bla.Bla1.Bla11 another time.

Now, without my patch, in the second run, modify_for_nested_pickle would be confused by the fact that Bla.Bla1.__dict__ lists Bla.Bla1.Bla11 under the name Bla11, but Bla11.__name__=='Bla1.Bla11'. With my patch, modify_for_nested_pickle expects exactly that naming scheme, and is thus changing Bla.Bla1.Bla11.__name__ into "Bla.Bla1.Bla11".

Much BlaBla, but I think it works...

Potential problems

sage: module = sys.modules['__main__']
sage: getattr(module, 'Bla1.Bla11')                      
<class __main__.Bla.Bla1.Bla11 at 0x46e7808>
sage: getattr(module, 'Bla.Bla1.Bla11')
<class __main__.Bla.Bla1.Bla11 at 0x46e7808>

Hence, Bla.Bla1.Bla11 is listed in the module under two different names. If you think it is bad, then one could probably modify the function when first_run is false, such that the name given in the first run is erased from the module.

Moreover, the reviewer will likely find a speed regression, when excessively creating nested unique representations. But that's hardly realistic...

[simon-king-jena]

Author: Simon King

[simon-king-jena]

comment:4

Another problem: Source inspection does not work yet in the following example.

sage: cython_code = [
... "from sage.structure.unique_representation import UniqueRepresentation",
... "class A1(UniqueRepresentation):",
... "    class B1(UniqueRepresentation):",
... "        class C1: pass",
... "    class B2:",
... "        class C2: pass"]
sage: import os
sage: cython(os.linesep.join(cython_code))
sage: A1.B1.C1??          
Error getting source: class A1.B1.C1 has no attribute '__class__'
Type:		classobj
String Form:	_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx_0.A1.B1.C1
Namespace:	Interactive
Loaded File:	/mnt/local/king/.sage/temp/mpc622/6475/spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx_0.so
Source File:	/mnt/local/king/.sage/temp/mpc622/6475/spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx_0.so

Even #11768 does not solve the problem.

Shall that be dealt with on a different ticket? Or in one go?

Probably on a different ticket, since I just find that even source inspection for A1 (which has a usual name) does not work...

[simon-king-jena]

comment:5

For the record: If #11791 is applied after this ticket, source inspection in the example above works (and is doctested).

[simon-king-jena]

comment:6

Is there a reviewer to fix name mangling of nested classes (needed in the category framework)?

[saliola]

comment:7

This patch also fixes an issue that came up in #8899 regarding documentation of nested classes not appearing in the reference manual.

See here for a description of the issue, see the thread on sage-combinat-devel.

See here for the confirmation that this works: #8899 comment:31

[vbraun]

comment:8

LGTM!

[vbraun]

Reviewer: Volker Braun

[jdemeyer]

comment:9

This causes trouble when building the documentation from scratch (i.e. after deleting 'devel/sage/doc/output`):

/usr/local/src/sage-5.5.rc1/local/lib/python2.7/site-packages/sage/categories/algebras_with_basis.py:docstring of sage.categories.algebras_with_basis.AlgebrasWithBasis.CartesianProducts.ParentMethods.one_from_cartesian_product_of_one_basis:3: WARNING: more than one target found for cross-reference u'one_basis': sage.combinat.sf.new_kschur.KBoundedSubspaceBases.ParentMethods.one_basis, sage.categories.algebras_with_basis.AlgebrasWithBasis.ParentMethods.one_basis, sage.combinat.ncsf_qsym.generic_basis_code.BasesOfQSymOrNCSF.ParentMethods.one_basis, sage.algebras.steenrod.steenrod_algebra.SteenrodAlgebra_generic.one_basis, sage.categories.examples.with_realizations.SubsetAlgebra.Fundamental.one_basis, sage.combinat.root_system.weyl_characters.WeightRing.one_basis, sage.categories.monoids.Monoids.Algebras.ParentMethods.one_basis, sage.categories.examples.hopf_algebras_with_basis.MyGroupAlgebra.one_basis, sage.categories.algebras_with_basis.AlgebrasWithBasis.TensorProducts.ParentMethods.one_basis, sage.algebras.affine_nil_temperley_lieb.AffineNilTemperleyLiebTypeA.one_basis, sage.categories.examples.algebras_with_basis.FreeAlgebra.one_basis, sage.combinat.symmetric_group_algebra.SymmetricGroupAlgebra_n.one_basis, sage.algebras.iwahori_hecke_algebra.IwahoriHeckeAlgebraT.one_basis, sage.algebras.group_algebra_new.GroupAlgebra.one_basis, sage.combinat.sf.sfa.SymmetricFunctionsBases.ParentMethods.one_basis, sage.combinat.root_system.weyl_characters.WeylCharacterRing.one_basis, sage.combinat.combinatorial_algebra.CombinatorialAlgebra.one_basis

[simon-king-jena]

comment:11

Jeroen, can you elaborate a bit what went wrong?

[simon-king-jena]

comment:12

Aha, now I see that the very long single line contains warnings about cross references that were not found. I'll try to identify them.

[simon-king-jena]

comment:13

Aha, here is an example:

The docstring of sage.categories.algebras_with_basis.AlgebrasWithBasis.CartesianProducts.ParentMethods.one_from_cartesian_product_of_one_basis is as follows:

            @cached_method   # todo: reinstate once #5843 is fixed
            def one_from_cartesian_product_of_one_basis(self):
                """
                Returns the one of this cartesian product of algebras, as per ``Monoids.ParentMethods.one``

                It is constructed as the cartesian product of the ones of the
                summands, using their :meth:`.one_basis` methods.

                This implementation does not require multiplication by
                scalars nor calling cartesian_product. This might help keeping
                things as lazy as possible upon initialization.
...

Could this simply be a misspelling? Note that it is written

:meth:`.one_basis`

but should certainly be

:meth:`one_basis`

If that's the case for the other warnings as well, then my patch would just uncover mistakes that happened earlier.

[jhpalmieri]

comment:14

The same issue arose in #13851 (see comment 10). I'm not sure why those dots are there, and I personally think they should be removed, but someone intentionally put them there.

[simon-king-jena]

comment:15

Replying to @jhpalmieri:

The same issue arose in #13851 (see comment 10). I'm not sure why those dots are there, and I personally think they should be removed, but someone intentionally put them there.

I think the dot is simply wrong - or is it ignored by Sphinx?

Actually here it is even worse. The text is documentation of a functorial construction, but refers to a parent method - that can't possibly work without an explicit reference to the method which must include the class which the method belongs to.

[simon-king-jena]

Attachment: trac_9107_fix_cross_reference.patch.gz

Fix a cross reference in some functorial construction

[simon-king-jena]

comment:16

Does the second patch fix the problem? I am now explicitly referring to the one_basis method of AlgebrasWithBasis.ParentMethods.

[nthiery]

Branch: u/nthiery/nested_class_name_mangling_can_be_wrong_in_case_of_double_nesting

[nthiery]

Commit: 891c3fa

[nthiery]

comment:75

Gosh, it turned out that using \setlistdepth{275} was not sufficient
anymore: I had to use \setlistdepth{2000}! This meant the problem
would just keep going to be worst and worst with time.

So I investigated a further and got lucky this time: if we replace
list by trivlist in the customized Verbatim defined by sphinx.sty,
then our documenation compiles smoothly, without even using enumitem.

I proposed this fix upstream:

https://bitbucket.org/birkenfeld/sphinx/issue/777/latex-output-too-deeply-nested

For the time being, I tweaked our conf.py to redefine and fix sphinx's
Verbatim.

Ok, now there just remains to check that all tests pass, and this will
be a needs review.

Cheers,
Nicolas

---

New commits:

727bd6a	#9107: Enable nesting of a nested class into a nested class
213e3b1	#9107: Fix one cross reference in the documentation of a functorial construction
66b7e72	#9107: fix a docstring (missing example and raw)
891c3fa	#9107: monkey patch a fix for deeply nested pdflatex compilation error until it's merged upstream (Sphinx #777)

[tscrim]

comment:76

Is there a reason why you don't patch the sphinx spkg directly?

[nthiery]

comment:77

Replying to @tscrim:

Is there a reason why you don't patch the sphinx spkg directly?

Lazyness mostly. If someone whats to create a patch, adapt the spkg, ... please go ahead!

[sagetrac-git]

Changed commit from 891c3fa to ed2c704

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

ed2c704

#9107: updated doctests now that the name of deeply nested classes is finaly correct

[nthiery]

comment:79

Ok, there were a couple doctest failures, all to be expected since nested classes now have a correct name. Fixed now. So that's a needs review!

[tscrim]

comment:80

Here's a version with a patched version of the sphinx spkg. Although I think your pull request is based on v1.2 (but it still applied (for me) to our current v1.1). Could someone check to make sure I created the patch correctly (and tell me what I should do instead if it isn't right).

---

New commits:

d6b432b

Moved hack conf.py to proper patch.

[tscrim]

Changed commit from ed2c704 to d6b432b

[tscrim]

Changed branch from u/nthiery/nested_class_name_mangling_can_be_wrong_in_case_of_double_nesting to public/nested_class-9107

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

8b14e05

#9107: improved description of Sphinx's patch nested.patch

[sagetrac-git]

Changed commit from d6b432b to 8b14e05

[nthiery]

comment:82

Replying to @tscrim:

Here's a version with a patched version of the sphinx spkg.

Ah, that's nice: modifying standard spkgs is much simpler now that we have a single repository. Thanks!

Although I think your pull request is based on v1.2 (but it still applied (for me) to our current v1.1). Could someone check to make sure I created the patch correctly (and tell me what I should do instead if it isn't right).

It seems to apply as it's supposed to (modulo trivial line offset).

I have made some minor improvement to the patch description.

I am a bit nervous about the removal of the former change log in SPKG.txt. But if someone can confirm that this is the right thing to do, that's ok for me.

Other than this, this sounds good to go!

Thanks again,
Nicolas

[vbraun]

comment:83

Replying to @nthiery:

I am a bit nervous about the removal of the former change log in SPKG.txt. But if someone can confirm that this is the right thing to do, that's ok for me.

Do it! ;-)

[tscrim]

Changed reviewer from Volker Braun, Florent Hivert to Volker Braun, Florent Hivert, Travis Scrimshaw

[tscrim]

Changed author from Simon King to Simon King, Nicolas M. Thiéry

[tscrim]

comment:84

Then away we go.

[nathanncohen]

comment:85

Thanks for fixing this !!!

[vbraun]

Changed branch from public/nested_class-9107 to 8b14e05

[fchapoton]

Changed commit from 8b14e05 to none