gap-system / gap

Main development repository for GAP - Groups, Algorithms, Programming, a System for Computational Discrete Algebra
https://www.gap-system.org
GNU General Public License v2.0
788 stars 160 forks source link

Change `PLAIN_LIST` (and similar in-place converters) to only work on mutable objects, reject immutable ones #3373

Open fingolfin opened 5 years ago

fingolfin commented 5 years ago

Motivation: all kinds of problems can crop up if e.g. an immutable string object is silently converted to a plain list. It also means that all places in the kernel that currently expose immutable string objects to the user are in danger, as users could cause those strings to be turned into plists (e.g. as in issue #815), which then later leads to crashes when some other kernel code accesses those string, expecting them to be still string objects.

Therefore, I think PLAIN_LIST should not accep immutable objects, but rather trigger an error for them. Of course it's not as simple as that: we'd also have to audit all calls to PLAIN_LIST to ensure they only are performed for immutable inputs. Some approaches that can be used to this end:

  1. change the code calling PLAIN_LIST to switch from plist APIs like ELM_PLIST to ELM_LIST etc.;
  2. make two variants of the calling code, one using ELM_PLIST etc. and one using ELM_LIST etc. (using C++ templates, this could be achieved with little code duplication);
  3. instead of calling PLAIN_LIST, we could instead call a function which creates a new plist with the content of the old list (at least for immutable inputs)
  4. we could outright reject (immutable) non-plists, and get people to fix their code calling into the kernel (of course that's a backward breaking change, so one should be very careful about doing that).

All of this could of course be applied to other in-place conversion functions; to this end, it'd be interesting to collect an overview of them (in particular, I am not sure if any exist that do something to string objects?).

ChrisJefferson commented 5 years ago

I think my preference would be a mixture of (1) and (2), in many cases I think PLAIN_LIST is an optimisation. If we could write a C++ template list wrapper, it would be easy to optimise many algorithms for plists vs normal lists.

fingolfin commented 5 years ago

Related issue: #3155

fingolfin commented 5 years ago

Also somewhat related: #815

stevelinton commented 5 years ago

This is about the definition of immutability. At the moment immutable objects are explicitly allowed to change representation, add stored attributes, etc. as long as they do not cease to be equal to their old values. So at the moment, kernel code that assumes that this will not happen is actually buggy.

We could change our mind on this and (for instance) forbid immutable objects from changing representation in-place. That would forbid switching permutations from perm2 to perm4 format, for instance. We could just impose that rule on lists.

Alternatively, we could just try to reduce the number occasions on which these changes happen, but then the kernel code is still buggy, but harder to detect.

My point being that if we want to change something in this area we should be clear what we are doing and whether it is guidance or policy.