[Development] On Removing Public Undocumented/\internal APIs

[Volker Hilsheimer]

> On 13 Mar 2024, at 08:30, Marc Mutz via Development <development at qt-project.org> wrote:
> 
> TL;DR: Make qdoc openly document \internal members of documented 
> classes/namespaces or defined in public headers as "Internal. Subject to 
> change without notice. If you feel this function should be public, file 
> a bug report with your use-case."?


Sounds like a good idea for public and protected member functions of otherwise documented classes, at least to help us understand the scope of the problem, and perhaps trigger a CI-blocking qdoc warning to raise awareness of the possible breakage.

It doesn’t help with types like QObjectData and QArrayData, which are not documented as \internal.


> On 07.03.24 10:09, Volker Hilsheimer wrote:
>>> On 4 Mar 2024, at 10:57, Marc Mutz via Development <development at qt-project.org> wrote:
>>> 
>>> Hi,
>>> 
>>> TL;DR:
>>> - Treat all APIs not clearly marked as private (private access, *Private
>>>   namespace or "We mean it!" comment) as public, in particular keep SC/BC
>>>   and deprecate before remove.
>>> - Avoid adding APIs that aren't clearly private or public in the future.
>> 
>> […]
>> 
>>> I would suggest that we treat any entity defined in a header file that
>>> is not in a *Private namespace or under the umbrella of an "We mean it!"
>>> commant, in short: anything that isn't _visible_ as non-public API _from
>>> the header file_, as semi-public, and apply the same rules as for public
>>> API: keep BC/SC, deprecate first, remove second, and adjust QUIP-6
>>> accordingly.
>> 
>> I think we have to acknowledge that Qt is 30 years old this year, and over the decades the capabilities of tools, the features of the languages, and the best practices, skills, and awareness of the people working on Qt have changed a lot. What usually hasn’t changed is the *intent* of the class author - even though it might be buried deep in the historical repositories. Stating now that everything that has the technical appearance of a public API cannot be changed, and ignoring the author’s intent, does not seem constructive.
> 
> Two users have come out and said that they think public undocumented API 
> is fair game. If there's a difference between the expectations of users 
> and expectations of API authors, do you _really- propose to pick the 
> author's?


This is not about picking sides. We have enough undocumented/not-designed-for-public-consumption API in public headers (but also not in QtPrivate or otherwise marked as internal) to establish that the status quo is: use undocumented API at your own risk; they might go away or change in arbitrary ways.

I agree that this is not as good as it could be, by today’s standards, so I’m happy that we are trying to find practical ways to improve.


>>> ¹ Off the top of my head: QString::isSimpleText(), QPen::data_ptr(),
>>> QDeferredDeleteEvent
>> 
>> Undocumented APIs in any SDK are, IMHO, “use at your own risk”. I (or my IDE) might find something promising in some public header, but if I have no information about what the author of that thing is promising me, should I use it in my code? Sometimes it’s more obvious than other times, but what on earth is the semantics of "QString::isSimpleText”? What is that “DataPtr” reference returned by "QPen::data_ptr”? Can anyone use QDeferredDeleteEvent::loopLevel() in a meaningful way without studying the event delivery code?
> 
> It's not for me to prove that these functions are useful or not. It's 
> for the changers and removers of these functions to prove that no-one 
> uses them.
> 
> But ok, let's see:
> 
> - the data_ptr() could be used as a null-check (there's no QPen::isNull())

There’s also no way to create a null-pen, from what I can see. Both QPen() and QPen(Qt::NoPen) create a pen with a valid data_ptr(). A moved-from QPen will have a null-data_ptr(), but calling data_ptr() on a moved-from object would already be UB.

> - QDeferredDeleteEvent could have been posted to an object in another 
> thread, because there was doubt as to whether deleteLater() was actually 
> thread-safe, but posting events is known to be (_I_ had to look that up 
> myself just now).


Which makes my point: to use those APIs you have to read the code. And then you’ll see that they are either undocumented, or documented as \internal.

You can still use them, but you know that they might change.


> I have no use-case for isSimpleText(), but what, exactly, did the Qt 
> project gain by removing it instead of deprecating it as "We have found 
> no use for this function. If you use it, please report a bug."?
> 
> The question that remains unanswered is this: Given that _we_ know how 
> to deal with change and _we_ control the changelog, why should our 
> _users_ be subjected to unannounced breaks of in-good-faith code?
> 
> There's a time for such changes: a major release. A minor release is - 
> IMHO - not a time for such changes.
> 
> We have all the tools we need to not step on our users' toes here, while 
> still pre-programming for a smooth transition for us come Qt 7. If we 
> decide to nevertheless do these changes before Qt 7, what, exactly, does 
> that say about our priorities?


On that one, I agree with you that deprecating it would have been preferable; the function was presumably removed because we didn’t use it ourselves anymore, so deprecating it wouldn’t have had any additional fallout within Qt.


>>> At the same time, we should make sure that we don't accept such
>>> semi-public APIs going forward anymore.
>> 
>> I do agree that we shouldn’t add more such stuff, at least not without making it visible in the header, ideally in a way that makes it visible to code completion (if that’s possible at all).
>> 
>> For stuff that is there today (e.g. various container types’ “isSharedWith” public member function, usually documented explicitly as \internal, and often used in other modules or in tests so not really practical to do it any other way), should we add a "// internal” comment in the header file?
> 
> Why is isSharedWith() \internal in the first place? The containers are 
> publicly documented to be implicitly shared, so the likes of isShared(), 
> isSharedWith() and detach() should be public documented functions, IMHO.

No strong opinion on these specific APIs.

How does isShared or isSharedWith work wrt concurrency? The pointer comparison might be atomic, but the code that relies on it is usually not, so code that calls this function needs to have control over all possible shared copies.

Which is perhaps enough reason to strongly discourage using it, by whatever means we can agree to use for that ("not documenting” having been one of those means so far).

> For new \internal API, something like _internal as part of the function 
> name is probably a good idea. Then qdoc could automatically skip them. 
> Oh, that gives me an idea: QDoc could take \internal members of a 
> documented class/namespace (or defined in a public header) and 
> explicitly document them as "Internal, subject to change w/o notice. If 
> you feel this entity should be public API, report a bug <link>.". That 
> has the advantage that it solves the problem for all existing functions, 
> in one fell swoop, even for past Qt versions. Give it an LTS cycle to 
> sink in, and then we grant ourselves the freedom to act on it again. 
> Even so: deprecating instead of removing should remain the gold 
> standard. But QPen::data_ptr() would have been covered by this, at least.

Note that QPen::data_ptr is still there; but it changed in BiC ways because of an implementation detail: from QSharedDataPointer to QExplicitlySharedDataPtr.

https://codereview.qt-project.org/c/qt/qtbase/+/503657

I don’t think we should have to carry around essentially two implementations of QPen to maintain the compatibility in that one.

Volker