absl TBD features _NOT_ past their two-year limit

[Peter Kasting]

The other thread I just started covers absl features that we need to make calls on, but this prompted examining the other TBD absl features. There are three: nullability annotations, Overload, and NoDestructor. We could make decisions on these too.

Starting point proposal:

1. Nullability annotations: Ban? Because these aren't types, they provide no benefit other than information to a human, which seems misleading -- people would expect their use to add safety, and attempt to lean on them for trustworthiness. CHECK(t) or using a [const] T& seem better. Also allowing these invites inconsistent usage across the codebase and endless bikeshedding. I would be reluctant to allow unless we mass-converted, and that's not feasible.
2. Overload: Allow and migrate existing base::Overloaded usage.
3. NoDestructor: Allow and migrate existing base::NoDestructor usage.

PK

[Danil Chapovalov]

Actually nullability annotations are no longer just for humans: clang does understand them and does issue a warning (becoming an error) when you try to pass a value annotated as Nullable to a parameter annotated as Nonnull.

They do not replace CHECKs (and CHECKS are not discouraged when you use annotation), but they nicely replace pure comments.

"[const] T&" is still better, but doesn't replace all scenarios. e.g. nullability annotations can be used with smart pointers, including (with a small patch) chromium-specific smart pointers.

[Peter Kasting]

On Tue, Dec 17, 2024 at 2:22 AM Danil Chapovalov <dani...@webrtc.org> wrote:

Actually nullability annotations are no longer just for humans: clang does understand them and does issue a warning (becoming an error) when you try to pass a value annotated as Nullable to a parameter annotated as Nonnull.

Do they also catch passing nullptr directly?

How does one have to "annotate values as nullable"?

PK 

[Danil Chapovalov]

Yes, passing nullptr directly is catched.

But looks like I'm wrong about catching passing nullable pointer where nonnull pointer is expected.

That does compile (yet, I hope. That particular check is not fully implemented/not fully released):

https://godbolt.org/z/TWjs3qEze

values are annotated by annotating their type, but once you check the value for nullability, it automatically 'becomes' nonnull.

More details can be found in go/totw/230 but it wasn't released to the public.

[Avi Drissman]

Speaking of TOTW 230, it directly calls out the use of absl::Nonnull with "Note: The above example may occur in a legacy API that cannot accommodate references. For new code, just use a reference in this case." And as for the usage of a nullable pointer for a non-nullable parameter, it notes that is a clang-tidy check that seems to be Google-only for now.

Right now I'm leaning towards banning for those reasons and those that PK gave.

Avi

--
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 visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACQca%3DfR-U0bJ5ik2169q6wWSK%3DNY7Q%2Be1fqxXe3MhB14jwosg%40mail.gmail.com.

[Danil Chapovalov]

Personally I like nullability annotation as documentation of the intent. (e.g., Nonnull<unique_ptr<T>> - can CHECK and move on, Nullable<unique_ptr<T>> - have to treat nullptr as a valid value)

Same time it sure has the cost of extra vocabulary construct every developer needs to be aware of what it is and what it is not. Especially since this annotation looks like an extra type while it is not.

As I do not work much in the chromium codebase, I have no opinion if Nullability annotations should be banned, allowed, or left TBD until better compiler support.

I just want to provide a bit more information about this construct so that you can make better informed decisions.

Danil

[David Benjamin]

Getting some kind of story for null would be pretty cool. I don't think our current story is very good. Pointer vs refs don't come with any static checking. (From trying to enable UBSan, folks bind null to a reference *all* the time.) I also personally really don't like that going from pointer to reference also picks up a whole suite of other behaviors (not being able to re-assign them, the weird implicit syntax, etc.), when all we might have wanted was a non-null pointer. It's also really problematic for incrementally changing things because changing the function requires touching every call site. So I think there's a lot of room for something better.

At the same time, any story here would cut through the whole codebase so probably needs to be done more intentionally than adding by default. E.g. with someone looking through options, playing with it, figuring out what our convention should be, what we get from it, and how to transition to it. So I would suggest not allowing it for now, but maybe with a note that this warrants some further investigation and discussion.

+1 for absl::Overload and absl::NoDestructor. Looking at their implementations, it looks like the only difference with absl::Overload is this constructor. The comment sounds like this is only because Abseil still supports C++17 but otherwise it should behave the same? Guessing it's fine, though we could confirm this by making base::Overloaded forward to absl::Overload and seeing if it works.

https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/functional/overload.h;drc=f9f22d5b8874e32153d0a15fc5bbc63c52815357;l=56

The NoDestructor implementations are a bit longer, so harder to compare them, but I assume they're comparable. The two notable differences I noticed are:

1. The Abseil one has a std::launder call, while we're missing one. We don't build with strict aliasing, but that still seems a positive.

2. The base one has a workaround for LeakSanitizer. Unclear if the workaround is still needed. crbug.com/40562930 hasn't had activity for a while.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACQca%3Dfv_s9wGPaKv9wjQjXLDz%2B6EZarCVddha-sXeQ0Sc98Zg%40mail.gmail.com.

[Peter Kasting]

I'm not opposed to allowing nullability annotations if someone wants to actually ponder what our story should be. That doesn't mean committing to any implementation work, just writing a one-pager saying where we ought to go with things and why. Perhaps some security-minded folk would be interested in this? +CC tsepez?

Only David has responded to Overload and NoDestructor. Other people want to weigh in on those?

As with the "past two year limit" thread, it might be nice to get some kind of consensus here in the next two weeks.

PK

[Adam Rice]

My two cents:

Nullability annotations: ban. Too much visual noise for too little concrete benefit.

Overload: Allow and migrate

NoDestructor: Allow and migrate

--

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 visit https://groups.google.com/a/chromium.org/d/msgid/cxx/9586cd75-04a5-4ccd-9fe7-e3d1fb111b61n%40chromium.org.

[Daniel Cheng]

The main issue with migrating NoDestructor is there are a lot of classes that friend NoDestructor. Those will all need to be updated; preferably to use the pass key idiom, since *how* NoDestructor constructs the type is an internal implementation detail, and absl::NoDestructor happens to use a helper class to instantiate the type.

Daniel

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAC_ixdzNPURd7FAUSTwd3MGw1nqpi0WyBOeoikm1ns-yxAjQVA%40mail.gmail.com.

[Peter Kasting]

On Sunday, January 5, 2025 at 8:27:06 AM UTC-8 Daniel Cheng wrote:

The main issue with migrating NoDestructor is there are a lot of classes that friend NoDestructor. Those will all need to be updated; preferably to use the pass key idiom, since *how* NoDestructor constructs the type is an internal implementation detail, and absl::NoDestructor happens to use a helper class to instantiate the type.

Looks like there are around 1450, which is IMO too many to do without a bit of tooling assistance. Ugh. Either someone would need to write some, or we'd need to ban until/unless someone migrates first. (Such a migration could be done incrementally ahead of time; changing from friending NoDestructor to using a PassKey is a transform that can be done at any time. However, this probably merits a discussion about whether we WANT to encourage PassKey usage over friending NoDestructor, since while I personally think it's an improvement, I know some folks dislike PassKeys in general.)

PK

[David Benjamin]

Aww yuck, nice catch. Yeah, that's a reason to not use the Abseil one yet. :-(

I generally agree that PassKey is a better pattern. I sampled a few callers, and every case I found was actually a singleton made by some static T::GetInstance() method or so.

https://source.chromium.org/chromium/chromium/src/+/main:components/metrics/structured/structured_metrics_client.cc;drc=a37ae0ab835f1fee5c20f0ef1cef7d597ed0e2b9;l=20

https://source.chromium.org/chromium/chromium/src/+/main:components/history/core/browser/history_backend.cc;drc=7704f726295cc9cdfae697ac5b3a489ce4e79f48;l=118

In that case, base::PassKey<T> not only works great but is actually an improvement. The NoDestructor friend allows any code to instantiate the type, as long as they wrap it in NoDestructor. That doesn't mean much (see below). PassKey<T> lets you actually enforce that T is a singleton. And then instead of a private `~HistoryPathsTracker() = default`, it should probably be `~HistoryPathsTracker() = delete` to enforce that T never accidentally makes it a non-singleton.

I tried to find where this pattern originally came from, and I think it might be descended from our pre-NoDestructor singleton patterns. (Though I'm not positive. I just did `git log -S'friend class base::NoDestructor' --grep=NoDestructor` just to see whether there was a habit of doing this when converting.)

https://chromium-review.googlesource.com/c/chromium/src/+/1411786

https://chromium-review.googlesource.com/c/chromium/src/+/1426682

https://chromium-review.googlesource.com/c/chromium/src/+/935841/7/base/trace_event/trace_log.h

But unlike DefaultSingletonTraits, NoDestructor does not enforce any singleton aspects, so the new NoDestructor pattern was actually a regression in enforcement. Switching to PassKey recovers it. So I think PassKey is clearly the way to go for this pattern.

If there are cases where the object is created out of a random accessor function, not specifically tied to T, how to move to base::PassKey is less clear. There isn't a clear class to specify in the passkey. But this is also ultimately another way to specify friends, but up a layer. So all the discussion about long-distance friendships and implementation details in https://abseil.io/tips/134#why-cant-i-just-friend-stdmake_unique-or-abslmake_unique still apply.

If the allowed creators are truly not tied to T, I think the TOTW has it right:

> Finally, by friending make_unique, you’ve allowed anyone to create your objects that way, so why not just declare your constructors public and avoid the problem altogether?

If T truly just wanted to allow anyone to create NoDestructor<T>, and just cared about the NoDestructor part, there wasn't any point in making the constructor private in the first place. It could have just made the constructor public. With a private or deleted destructor, that already enforces that T is never destroyed. (https://godbolt.org/z/jqE8c5r3o)

So I think that means:

1. At minimum, we can just make ctors public, make sure dtors are indeed private (better: deleted), and delete the friend declaration. That does basically the same thing.

2. But since all (of sample size of 2) instances are actually just making singletons, a better change would actually be base::PassKey<T>, where feasible, and then fall back to (1) when not.

--

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 visit https://groups.google.com/a/chromium.org/d/msgid/cxx/7059292c-74db-4620-9902-9d0bef5c8909n%40chromium.org.

[Greg Thompson]

This reminds me: I think static local NoDestructor is heavily overused. If there should only be one instance of some Foo in the browser, then I think it's preferable to find a logical place during browser startup to create (and, if relevant, destroy) the Foo. If you need a Foo::GetInstance for convenience, then have the ctor make the one instance available for retrieval by GetInstance. Don't want devs to accidentally create a second instance? Then document this and CHECK in the ctor that the one instance hasn't already been created.

Use of static local NoDestructor makes testing harder, since you're forced to either ensure only one test per process (fine for browser tests, but not unit tests) or you need to add methods to reset state between tests. Bleh.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF8qwaDK1u-q2zTQErHPJi_eXdWgdjFiJpCHF%2Bgufu4LnAHk0Q%40mail.gmail.com.

[Sylvain Defresne]

Static local NoDestructor may be overused due to the comment at the top of base/memory/singleton.h

// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

// PLEASE READ: Do you really need a singleton? If possible, use a

// function-local static of type base::NoDestructor<T> instead:

//

// Factory& Factory::GetInstance() {

//   static base::NoDestructor<Factory> instance;

//   return *instance;

// }

// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

To me, this reads like this recommends writing a factory using a static local NoDestructor.

It is the pattern used by many (most) KeyedService factories based on that recommendation (previously the code was using LeakySingleton). Note that for KeyedService factories, it is not necessary for the factory to be a singleton (I think a few tests create non-singleton factories), but the factory registered with the DependencyManager has to be static. They could be singleton and thus the absl NoDestructor + PassKey pattern would work.

-- Sylvain

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKQnr0R8cB0-bNZQ49Jyp7ygR_KpSjZo7ZSP%3DuK6urF1Avqg2Q%40mail.gmail.com.

[Daniel Cheng]

+1 to this. NoDestructor has its uses, but my personal feeling is that there are a lot of places that use it unnecessarily. I made an attempt at cleaning up some of these last quarter: https://crbug.com/375364966

Daniel

[Alex Cooper]

I think static local NoDestructor also gets used due to the prohibition on static-initialized data, so if there's e.g. some source-of-truth vector/map that you want to be a constant I believe the guidance is to make a Getter function with a static local NoDestructor. Though the 1450 results definitely seems like a lot of potentially singleton-like classes....

I wonder if it seems that absl::NoDestructor most people are agreed in theory that switching to it would be good, but there's a bit of a discussion to be had on *how* to do so, that may warrant a separate thread so people don't simply ignore this discussion?

Back to the other two proposals:

FWIW, it sounds like while there may be *some* value in the nullability annotations, that the style guide *already* has recommendations that would/should make them redundant, so I agree with Ban there.

For Overload agree that porting seems fine.

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

[Avi Drissman]

That comment was added with https://crrev.com/c/869351 with the creation of NoDestructor. I'm pretty sure it was not written as an encouragement to use NoDestructor, but as a discouragement from using Singleton, as a similar comment exists for LazyInstance.

Yes, we should remove unneeded uses of NoDestructor, but honestly, we need to also finish up removal of Singleton and LazyInstance. We already have essentially three different ways of doing the same thing; if we're cleaning up, the end-goal should be to have just one.

Avi

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

[Lei Zhang]

It is likely one can avoid the NoDestructor usage with std::array and base::fixed_flat_{map,set}. An example of removing NoDestructor: https://crrev.com/976313

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGOLbz0ywvmDP4Jx%2B5v%2BtRWiEOaT%3DGpDYYdRYiV-vemmU2GoPA%40mail.gmail.com.

[Peter Kasting]

Here's what I'm hearing:

• Nullability annotations: Consensus to ban.
• Overload: Consensus to allow.
• NoDestructor: In theory consensus to allow, but migration may be painful. We have two choices:
  1. File migration bug to replace friending with e.g. PassKey, ban with reference to that being completed.
  2. Allow Abseil version, ban further use of base:: version, add PRESUBMIT warning, file bug on migrating existing instances, try to find owners (maybe code health rotation?).

I'm weakly Inclined toward #2, but maybe the churn isn't worth it.

PK

[Joe Mason]

My main question for #2 is, do we want to just convert existing uses of base::NoDestructor to absl::NoDestructor, which sounds fairly mechanical and easily doable with the code health rotation, or is it worth taking this opportunity to find over-uses of NoDestructor and replace with another idiom?

--

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 visit https://groups.google.com/a/chromium.org/d/msgid/cxx/acd57f90-1dad-442f-930d-6b46cb430714n%40chromium.org.

[Peter Kasting]

The issue is friend declarations, which cannot necessarily be fixed in a trivial mechanical way and must be rewritten to use a different idiom.

PK

[Joe Mason]

Sorry, I meant to write "straightforward", not "mechanical".

[Peter Kasting]

There's been no further discussion of this. Unless a volunteer wants to drive NoDestructor option #2, I think we fall into option #1 by default -- this touches too much stuff to just leave in an "old thing deprecated, new thing never tested" state.

PK

[Daniel Cheng]

A PassKey rewrite can probably be largely automated if someone is interested. I am happy to help provide guidance if someone wants to try that route out.

Daniel

--

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 visit https://groups.google.com/a/chromium.org/d/msgid/cxx/a98e7b94-e9cc-4150-b35b-1e19b9d1db19n%40chromium.org.

[Devon Loehr]

To add to Daniel's offer, I've been trying to put together a minimal "template" example of what an automated rewriter might look like, with the goal of teaching how to use clang's AST matchers in a chromium context, and providing an easy starting point for writing new ones. If anyone is interested in learning how to do so, I'd also be happy to help.

--Devon

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKpuWad3Y0iXDC2GOFC_1SPSkqecz632s4Qv%3DSvn55N1_A%40mail.gmail.com.

[Daniel Cheng]

If you have that template somewhere, we should link up resources at some point :)

I put together https://chromium-review.googlesource.com/c/chromium/src/+/6006214 and https://chromium-review.googlesource.com/c/chromium/src/+/5784484 as examples of more scalable and modern ways of doing clang tooling in Chrome, but I haven't had time to turn it into a form suitable for landing yet.

Daniel