Use of [[deprecated]] attribute

[James Lee]

Hi folks,

As far as I could find, there's no guidance on how to mark C++ Chromium classes/methods deprecated (in the sense of "the use is allowed, but discouraged"). As a result it can be easy to inadvertently use legacy interfaces rather than the latest recommendations.

What do people think about doing the following:

• Recommending that the standard C++14 [[deprecated]] attribute (cppreference) is used
• Disabling this warning in our clang configuration (-Wno-deprecated-declarations and friends) so that we don't get build errors or warnings for existing usages
• Enabling the corresponding lint check in clang-tidy, so that this can surface warnings for new usages in CLs

Thanks,

James

[Peter Boström]

I would prefer a compile-time enforced variant where "existing uses are allowed until migrated, new uses are no good even if you ignore clang-tidy". For instance, we could make use of a PassKey parameter during migration and then only let a few allowlisted friend-class exception create that PassKey.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/2a2bbc7e-3ca9-4302-b7bd-f7d28611a814n%40chromium.org.

[Łukasz Anforowicz]

On Wed, Mar 27, 2024 at 9:27 AM Peter Boström <pb...@chromium.org> wrote:

I would prefer a compile-time enforced variant where "existing uses are allowed until migrated, new uses are no good even if you ignore clang-tidy". For instance, we could make use of a PassKey parameter during migration and then only let a few allowlisted friend-class exception create that PassKey.

<meme>Why not both?</meme>

I agree that an actual compile-time enforcement is more desirable than a review-time suggestion.  But the barrier to entry is higher for the PassKey approach (IIUC one has to migrate all the existing callers to use the PassKey) and therefore maybe the proposed review-time approach is also helpful in some cases.

 

On Wed, Mar 27, 2024 at 8:20 AM 'James Lee' via cxx <c...@chromium.org> wrote:

Hi folks,

As far as I could find, there's no guidance on how to mark C++ Chromium classes/methods deprecated (in the sense of "the use is allowed, but discouraged"). As a result it can be easy to inadvertently use legacy interfaces rather than the latest recommendations.

What do people think about doing the following:

• Recommending that the standard C++14 [[deprecated]] attribute (cppreference) is used
• Disabling this warning in our clang configuration (-Wno-deprecated-declarations and friends) so that we don't get build errors or warnings for existing usages
• Enabling the corresponding lint check in clang-tidy, so that this can surface warnings for new usages in CLs

Thanks,

James

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/2a2bbc7e-3ca9-4302-b7bd-f7d28611a814n%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sEkw3%2B3UNmA-1%2BkFDuexX%2BTuE0BGbPE2xDcbesjt5yKdg%40mail.gmail.com.

[Mark Mentovai]

I don’t think we should do this. We rely on compile-time enforcement against use of declarations marked deprecated on macOS, for example, where the OS SDK makes heavy use of equivalent attributes. We do want to know when we’re introducing a new call to something that’s gone out of OS vendor support, so that we can generally avoid it. It would be detrimental to downgrade this to a clang-tidy check.

One thing we’ve done in the past is rename interfaces we’re trying to move away from so that it’s obvious that they’re deprecated. For example, base::bits::SignedIntegerDeprecatedDoNotUse and friends. Since we own the entire codebase, we can do a mass rename (as in the change that introduced that deprecation) quickly and easily without breaking anything, giving us time to work down the list of existing uses at our leisure (tracked by a bug like this) while being named in a way that clearly indicates to authors and reviewers to avoid introducing new transgressions during that time.

--

[Peter Boström]

+1 to Mark's points. More explicitly I am also against adding any `-Wno-deprecated` and friends. It also has implications on any third_party code etc.

Mass-rename to DeprecatedDoNotUse is a reasonable bar to clear for deprecating an interface if PassKeys seem too onerous (though I would recommend that once you get down to a low enough number if you're unable to finish the migration at that point). We do bypass things that are "just warnings" for expediency though so it's good if an allowlist (list of friends) is controlled by OWNERS having a vested interest in the migration rather than client code. It shouldn't be a client-code decision in isolation.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHk1GDyVS_DFpDYcP2ui%3DsE1mNyo-5f7_BPHD3KOeRgQVqgr1g%40mail.gmail.com.

[Roland McGrath]

Another possibility is to have `[[deprecated]]` (or better, `[[deprecated("explanation why")]]`) and keep the warning/error enabled, but then add site-specific warning suppressions around known existing uses.  The `#pragma GCC diagnostics` stuff (which can be macroized using `_Pragma`) makes it possible to surround a few lines with magic that prevents the warning when used there, but not elsewhere.  It might be feasible to have a clang-tidy fix-it that could insert use of such a macro in all the existing places where the warning is generated. That would allow a procedure where a thing is deprecated, along with all existing uses being marked with the macro magic to prevent `-Werror=deprecated` errors; and then existing uses can be migrated to remove the macro magic at whatever pace, but reviewers would not allow people to add new uses since it's an obvious red flag to insert new code that uses the warning-suppression macros.

[Nico Weber]

We can't disable Wdeprecate-declarations since that informs us of deprecated platform APIs.

My suggestion for this is always "don't deprecate things". It's very easy to mark some existing function as deprecated, but often very hard to remove uses of it. So the deprecated thing stays around forever, and not too much is won by marking it deprecated -- no problems really disappear. The big win is if you can also remove all uses, but then you can delete the old API, and you don't need an attribute.

In other words, if it's not worth cleaning up callers, most of the time it's not worth deprecating either. In many (most?) cases, "remove all calls and then the function" or "don't deprecate the function, even if you don't like it" is all you need.

The one exception is a long-running migration from old api A to new api B where a team owns the migration, but new uses of old api A are introduced at a high rate. Here, what Roland suggests or a presubmit can help.

There's the situation where you introduce a better way to do something but don't think removing the old way is really worth it. Here, you might argue that marking the old thing as deprecated but not doing anything about it will lead to the new way being adopted over time. I think experience shows that isn't what happens -- the old thing will stay around forever, as will the new way. IMHO having a new better and an old worse way is surprisingly often not better than having just one way of doing something. So this is a pattern I think we'd want to discourage. (Not ban outright, but I think we tend to do this kind of thing too often, not too rarely.)

On Wed, Mar 27, 2024 at 11:20 AM 'James Lee' via cxx <c...@chromium.org> wrote:

--

[Gabriel Charette]

We have multiple success stories from mass-renaming widely-used //base APIs as FooDeprecated(), the latest of which is RunLoop::QuitCurrent(WhenIdle)Deprecated() which was just completed by an external contributor.

I'm a strong proponent of that paradigm where appropriate.

In the case of RunLoop::QuitCurrent*(), we wanted to drive new usage away from the problematic (flakiness inducing) pattern but also didn't have the bandwidth to refactor existing tests (which were probably correct if not flaky). Dedicating a team to refactoring would have cost eng time to rewrite code that likely wasn't read often. The deprecated API phased out naturally as old tests were deleted or updated. It eventually became a low hanging fruit that some external contributor decided to fix :).

Moving base::MessageLoop::current()->Quit() under a RunLoop API and making it opinionated about the preference to use QuitClosure() was key to immediately getting a world where new tests were more readable and eventually truly removing the old API. Not deprecating it at the time would have been a mistake (and no we couldn't own that migration, this was layers deep in the refactorings blocking the migration to base::ThreadPool).

I agree that it's better to phase out than deprecate APIs when possible, but when it's not feasible or it's out-of-scope, I believe deprecating is better than leaving a subpar API or having two paths (an opinionated codebase is less confusing for new writers).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CADZ1XibhD%2B4xC9OW8RLtR4TzQAxU1wh5FbAUBxmkj_eiArSLXQ%40mail.gmail.com.

[Harald Alvestrand]

WebRTC is heavily used by both Chrome and Google3.

We're using [[deprecated]] heavily to signal to Google3 folks that "you'd better clean this up"  (and have some rudimentary dashboards for tracking the remaining usages) - if we couldn't mark them as deprecated, the refactoring into "the new pattern" would be either considerably more onerous (having to clean up dozens of Google3 call sites) or leaving the API in a state where deprecated functions are not visible to a compiler ("deprecated" in the form of comments).

Chromium's insistence on not allowing deprecations is a burden, because developers have to wade into unknown code areas and fix usages there before they can submit their WebRTC refactorings.

I would like Chrome to have a per-dependency policy on whether [[deprecated]] in dependendancies is allowed or not - the way dependencies on MacOS API changes are handled indicates that "all dependencies are allowed to deprecate" and "no dependencies are allowed to deprecate" are both too heavy-handed.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LL0XRFRV3CbWB1Fop5Vt_k6WSe55HZwQ1kVtGqErpdx7g%40mail.gmail.com.

[Peter Kasting]

We can't really stop third-party deps from doing what they want, if they use [[deprecated]] in APIs only called from within their dependency. In that case just toggling -Wno-deprecated or whatever in the appropriate .gni is enough.

If the [[deprecated]] APIs are on code called from (and thus compiled as part of) Chrome, then there's no easy way to have a "per-dependency policy", as in that case the warnings have to be toggled on the Chrome side.

Thus, code exposed as part of a public API is more challenging to deprecate than code consumed internally. Which, IMO, is as it should be.

PK