DEPR: deprecate direct mutations of TableElement.fields by making it an owned list

[neutrinoceros]

Description

Fixes #16314

The deprecation process is quite involved since we need to raise a warning for external usage of, say, table.fields.append, but still allow this operation to be performed silently internally.
My solution is to make TableElement.fields a "owned list" (my wording), that is, a list that has an owner object (namely the TableElement instance it belongs to), which is the only one allowed to directly mutate it (using a private keyword argument _owned=True). Mutations from other actors are still allowed as a temporary measure, but now raise a DeprecationWarning.

The commit history is meant to allow reviewing my process step by step. I'll also add some comments to the diff if I can help anticipate some questions from reviewers.

• By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

TODO

• comment diff
• handle magic methods too (__iadd__, __imul__, __setitem__ and __delitem__)
• Add docstrings to _OwnedList and _OwnedHomegeneousList (inheritance order matters !)
• open follow up issue to keep track of what to do when the deprecation ends (or we could just keep API: Make fields property immutable #16314 opened while the deprecation is on-going ?)

[github-actions]

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[github-actions]

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

[neutrinoceros]

A note on type annotations: I didn't intend to add them, but got them for free from my editor's supercharged auto complete.

[neutrinoceros]

It's not intended as a new feature, which is why I'm making it private by default, but I could just as easily make this class public, or move it to a different module, closer to where it's used.

[neutrinoceros]

here the diff looks fishy because it's collapsing 2 independent changes

• I added a new test whose preamble is based on table_from_scratch
• I removed table_from_scratch when I found that it wasn't used anywhere

I could split this cleanup into its own PR if desired.

[neutrinoceros]

I found that this line was not needed and in fact caused some bugs that are revealed by existing tests once we start using add_field instead of direct mutations. This is a regression from #15959, which can easily be fixed independently in a -1/+0 separate PR if desired.

[eerovaher]

Having a subclass that must disable a large number of its inherited methods doesn't look right. Have you considered implementing HomogeneousTuple instead?

[neutrinoceros]

Unfortunately mutability is still needed internally, so I don't think a tuple would suitably replace the existing data structure.

[eerovaher]

I'm not sure what the best solution here would be, but disabling this many methods makes it apparent that you don't want this to be a list, so why would you inherit from it?

[neutrinoceros]

I still want it to be a list, I just don't want it mutated by anyone who's not the container object.

[eerovaher]

I still want it to be a list, I just don't want it mutated...

So you don't want it to be a list.

[neutrinoceros]

Well to take a different angle, we're talking about an attribute that's both a list (mutable) and a property (read-only), which one could argue is already a contradiction, but that ship sailed long ago. I don't think however that it's fundamentally contradictory, the language just doesn't offer a simple way to build such properties.

[eerovaher]

Couldn't you have a private list and have the users access its contents through a public property that converts the list to a tuple? If the list is private then there is no need to deprecate any of its methods.

[neutrinoceros]

Whatever we do needs to go through a gradual deprecation process. We can't make public API private or broken in one go.

[eerovaher]

A vanilla tuple wouldn't be appropriate immediately, it would be necessary to implement some weird subclass of tuple that would be aware of the private list it is representing and it should also have list methods that would emit deprecation warnings. That does not sound more appealing than the weird list subclass you are creating here. The difference is that such a tuple subclass could be removed when the deprecation period is over, whereas your _OwnedList is something we would be stuck with.

[neutrinoceros]

Couldn't you have a private list and have the users access its contents through a public property that converts the list to a tuple?

The difference is that such a tuple subclass could be removed when the deprecation period is over

I think I see your point. However it doesn't feel right to me that the proposed intermediate step involves a "mutable tuple" of sorts. I also think such a class would end up looking a lot like _OwnedList. Actually, now that I think about it, wouldn't it make sense to keep _OwnedList as the intermediate state and then implement your proposed final state when we complete the deprecation cycle ? The one downside I see with this approach is that the public attribute would move from being a list to a tuple over a single commit, but that's also true with your proposal taken as a whole, and the alternative (having an intermediate state where it's both) seems worse (?).

[eerovaher]

The important thing for me is that we don't end up with a permanent immutable list subclass. Implementing something strange for the transition period can be acceptable as long as it is temporary and I don't really care about details beyond that.

[neutrinoceros]

ok then I'll add a note to #16314 and rework the warning message here so that it avoids making promises about which kind of error will replace it in the future.

[neutrinoceros]

@tomdonaldson I think you're in a great position to review this, but no rush, I'm just letting you know :)