[Bugfix] Fix errant const methods

[Skylion007]

Description

• Clang-Tidy accidentally declared some methods const that shouldn't be because the underlying ptr was edited, but not the object itself. I am not sure if there is a better way to fix this warning (ie. with a mutable keyword or something on the ptr, but open to ideas). I just went through manually and suppressed the const functions that didn't make sense.
• Please let me know if I missed any.
• See here for a summary of logically vs physically const: https://medium.com/@manoj563125/c-deep-dive-const-const-correctness-and-other-snags-307c01edbd3b and why it's better to follow logical const
• PyBind11 isn't very const correct (since we cast away constness quite often), but this should at least be a good first step.
• Added a couple of nolints to some methods that were false positives and incoorectly marked as const by clang-tidy for being logically const. These either modified the public vars of stucts or deallocated memory so actually are not logically const, only physically.

Suggested changelog entry:

* Remove const modifier from certain C++ methods on python collections (list, set, dict) such as  (clear(), append(), insert(), etc...) and annotated them with py-non-const.
* Revert a few incorrect constness clang-tidy changes and added NOLINTS to those cases.

[Skylion007]

Seems like there were a few more incorrect ones added in: #3052

[Skylion007]

Current failures are flakes.

[Skylion007]

Ping @henryiii @rwgk . Any other issues here? Any methods I might have missed?

[rwgk]

I'd say: merge! :-)
If we notice other py-non-const methods later we can take care of them as we discover them. Better than spending time looking for needles in the haystack.

[jbms]

The collection methods like append, clear, etc. should actually be const. (The deallocation methods should stay non-const.)

The reason is that dict, list, set (like all types that inherit from pybind11::object) have shared pointer semantics, not value semantics. If you copy a list object, you get another pointer to the same list, not an independent list with the same contents.

Note that I'm referring specifically to the semantics of the type, not the implementation. A type like std::vector internally uses a pointer to the data, but it still has value semantics and it makes sense for the methods that mutate the vector to be non-const.

The only non-const methods of a type with pointer semantics should be ones that actually modify the pointer itself, not the pointee. That is the only approach that is consistent. The way it is currently, you can trivially "defeat" the const by making a copy:

void foo(const pybind11::dict &x) {
  pybind11::dict copy = x;
  copy.clear();
}

Essentially, pybind11::dict is similar to a std::shared_ptr<PyDictObject> with a deleter that calls Py_DECREF. A "const" dict would be represented by std::shared_ptr<const PyDictObject>, not const std::shared_ptr<PyDictObject>.

Since Python itself always uses pointer semantics, and doesn't have const objects other than the specialized frozen collections, I don't think it is particularly helpful to try to add an equivalent of std::shared_ptr<const PyDictObject> to pybind11. But in any case, for consistent semantics it should be a separate const_dict type.

[rwgk]

We had a little bit of a discussion in the pybind11 slack channel before this PR was merged. I'm copying a selected comment from back then below.

I don't think there is a way to make this "right". I agree with @jbms' observations, but feel it's best to not kick up more dust over this.

Ralf Grosse-Kunstleve Aug 12, 2021, 12:46 PM

Of course ... this happily compiles ... I keep forgetting how annoying the C++ const semantics are:

inline void modify(int *ptr) { *ptr = 1; }

struct ptr_owner {
  int *m_ptr;
  void call_modify() const {
    modify(m_ptr);
  }
};

void call_modify(const ptr_owner& ownr) {
  ownr.call_modify();
}

int main() {
  ptr_owner ownr;
  ownr.m_ptr = new int{3};
  ownr.call_modify();
  call_modify(ownr);
  return 0;
}

[jbms]

It is true that the lack of built-in support for deep const in C++ leaves something to be desired. But I would still suggest being consistent with how const propagation works in C++. For pointer-semantic classes templated on a type parameter, it usually works well to use const-qualification of the type parameter to indicate inner-const, consistent with built-in pointer types and std::shared_ptr.

template <typename T>
using dumb_pointer = T*;

void foo(dumb_pointer<const int>& x) {
  x = nullptr; // allowed
  *x = 5; // not allowed
}

void foo(const dumb_pointer<int>& x)  {
  x = nullptr; // not allowed
  *x = 5; // allowed
}

When dealing with non-templated pointer-semantic types, it is unfortunately more annoying to also define an inner-const version. You could either use a separate name, like const_dict, or add an extra template parameter, e.g. a bool to indicate inner-const. For pointer-semantic types there is actually usually another way in which you want to parameterize, owned vs unowned. The "owned" type behaves like pybind11::object, and increments/decrements the reference count, while the unowned type behaves like pybind11::handle and is cheap to copy since it just holds a raw pointer.

In any case, for pybind11 I would argue that inner-const wrappers of Python types are not really needed since Python itself does not really support const except via separate types like frozenset and tuple.

[rwgk]

In any case, for pybind11 I would argue that inner-const wrappers of Python types are not really needed since Python itself does not really support const except via separate types like frozenset and tuple.

Yeah, adding const_xxx versions for everything seems like a lot more trouble than it is worth.

I'm fine either way, with what we have, or adding the const back. I'll happily go with what you and @Skylion007 agree on.

[henryiii]

I'm not very invested in this at the moment, but from general principles, I'm mildly in favor of following C++ here. We should follow the language most of the time (the only exception is the const return for Eigen, but there we are intentionally using a language construct for a feature).