Is it fine to support implicit conversion for C++ class GaiaId in tests

[Mikel Astiz]

Hi chromium C++ style guide folks,

Please route if this is not the right forum to ask.

I am reaching out motivated by the style guide's recommendation as per "Implicit conversions can sometimes be necessary [...]. In that case, contact your project leads to request a waiver of this rule" (source). Beyond the strict scope of the style guide, I would also appreciate feedback on whether the pattern discussed in this thread looks reasonable.

Context: I worked on a patch series to improve type safety by avoiding std::string when representing Google account IDs (Gaia IDs) in the codebase. We now have a class GaiaId, which is similar to having a base::StrongAlias, and encapsulates a std::string.

The problem: Many tests (unit-tests and browser tests) or test-only classes define string literals that represent Gaia IDs in a way that the literal is initialized statically (constexpr or otherwise). There are 500+ explicit casts to GaiaId in tests, excluding iOS code (which hasn't been updated yet). One illustrative example is FakeGaiaMixin::kFakeUserGaiaId, with 24 references, all of which convert to GaiaId. It would be nice (more readable, less error-prone) if those literals could be defined using class GaiaId directly. However, this AFAIK is forbidden by the style guide because it isn't trivially destructible (source). I am not aware of tests being exempt of this rule.

The solution being considered: one idea that I thought of was to defined a test-only class, say class GaiaId::LiteralForTest, that has a trivial destructor (e.g. encapsulates a std::string_view) and converts implicitly to GaiaId. This works (see patch) but goes against the style guide and also generally it isn't clear if it improves or harms readability. The fact that I haven't seen precedents in the codebase, e.g. for GURL, makes me suspicious.

Could you provide guidance?

Thanks,

Mikel

[Peter Kasting]

On Tue, Jan 14, 2025 at 7:41 AM Mikel Astiz <mas...@chromium.org> wrote:

The problem: Many tests (unit-tests and browser tests) or test-only classes define string literals that represent Gaia IDs in a way that the literal is initialized statically (constexpr or otherwise). There are 500+ explicit casts to GaiaId in tests, excluding iOS code (which hasn't been updated yet). One illustrative example is FakeGaiaMixin::kFakeUserGaiaId, with 24 references, all of which convert to GaiaId. It would be nice (more readable, less error-prone) if those literals could be defined using class GaiaId directly.

To push on this a little: Would it be less error-prone? Have you had problems with tests accidentally using the wrong string as a GAIA ID?

For readability, it seems like we have:

constexpr char kId[] = "...";

foo(GaiaId(kId));

vs.

constexpr GaiaIdLiteral kId = "...";

foo(kId);

I do think the latter is better. I don't know that it's a very large difference.

 

The solution being considered: one idea that I thought of was to defined a test-only class, say class GaiaId::LiteralForTest,

Trivial: I'd omit things like "ForTest" from the class name because there's no reason the type cares where it's used, and it makes the code less noisy.

 

that has a trivial destructor (e.g. encapsulates a std::string_view) and converts implicitly to GaiaId. This works (see patch) but goes against the style guide and also generally it isn't clear if it improves or harms readability.

I'm not too worried about the implicit conversion, because these two objects are meant to represent the same conceptual thing, so an implicit conversion is appropriate.

I think the big question is whether this buys much readability and is worth the churn of trying to do this.

A question: how would you feel if there were several hundred instances each of (using this, not using it) in tests? I think whether you consider that world a win over the current one informs how to proceed here.

PK 

[Mikel Astiz]

Thanks for the quick response, answers inline.

On Tue, Jan 14, 2025 at 4:53 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Jan 14, 2025 at 7:41 AM Mikel Astiz <mas...@chromium.org> wrote:

The problem: Many tests (unit-tests and browser tests) or test-only classes define string literals that represent Gaia IDs in a way that the literal is initialized statically (constexpr or otherwise). There are 500+ explicit casts to GaiaId in tests, excluding iOS code (which hasn't been updated yet). One illustrative example is FakeGaiaMixin::kFakeUserGaiaId, with 24 references, all of which convert to GaiaId. It would be nice (more readable, less error-prone) if those literals could be defined using class GaiaId directly.

To push on this a little: Would it be less error-prone? Have you had problems with tests accidentally using the wrong string as a GAIA ID?

I came across a few examples (e.g. this browser test) that appear suspicious (e-mail used instead of Gaia ID), but I can't say if having literals would have prevented those cases.

I'm not done with the patch to assess if it surfaces additional cases where a gaia ID literal is passed as something that is not a gaia ID, but IIRC I spotted at least one case so far (which I can't find right now).

 

For readability, it seems like we have:

constexpr char kId[] = "...";

foo(GaiaId(kId));

vs.

constexpr GaiaIdLiteral kId = "...";

foo(kId);

Correct.

 

I do think the latter is better. I don't know that it's a very large difference.

 

The solution being considered: one idea that I thought of was to defined a test-only class, say class GaiaId::LiteralForTest,

Trivial: I'd omit things like "ForTest" from the class name because there's no reason the type cares where it's used, and it makes the code less noisy.

Ack.

 

 

that has a trivial destructor (e.g. encapsulates a std::string_view) and converts implicitly to GaiaId. This works (see patch) but goes against the style guide and also generally it isn't clear if it improves or harms readability.

I'm not too worried about the implicit conversion, because these two objects are meant to represent the same conceptual thing, so an implicit conversion is appropriate.

Ack, thanks.

 

I think the big question is whether this buys much readability and is worth the churn of trying to do this.

The churn, if you mean one-off effort to rewrite code, is quite comparable because I still need to update the iOS code (and tests) to adopt class GaiaId. Having the ability to define constexpr literals reduces the amount of work to port iOS tests (as it becomes a drop-in replacement for const char[]).

  

A question: how would you feel if there were several hundred instances each of (using this, not using it) in tests? I think whether you consider that world a win over the current one informs how to proceed here.

I myself would have no concerns with partial adoption of these literals, specially for file-internal occurrences (rather than shared classes, e.g. 1, 2, 3, 4). 

Cheers,

Mikel

 

PK 

[Danil Chapovalov]

Note that std::string has constexpr constructor and destructor since c++20

Have you tried to declare GaiaId constructor (and destructor) constexpr?

--
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/CAAy826cZFXgiHpWwro_cMGMujh1zSAW94TrPnqYikSfJVwNpBA%40mail.gmail.com.

[John Admanski]

Note that std::string (or any class) having a constexpr constructor and destructor is not sufficient for it to be usable for making static constexpr objects. If your class dynamically allocates memory then it can't be used as a constexpr variable regardless of the constexpr-ness of its construction and destruction.

[Daniel Cheng]

How many of the tests in question can declare GaiaId's as fields of their fixture? It's not the greatest workaround, but it's one way I've used to work around having test constants with non-trivial destructors.

Daniel

P.S. you didn't hear it from me but we don't actually warn about exit-time destructors in tests. The style guide does not provide an exception though :)

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/17dde71f-0427-4f1d-b6f4-78a0f5ab8aean%40chromium.org.

[Mikel Astiz]

Hi,

Thanks everyone for your inputs, answers inline.

On Wed, Jan 15, 2025 at 6:18 PM Daniel Cheng <dch...@chromium.org> wrote:

How many of the tests in question can declare GaiaId's as fields of their fixture?

I expect all cases can be. If there are exceptions to this, we could always have a standalone function that returns an instance instead. 

 

It's not the greatest workaround, but it's one way I've used to work around having test constants with non-trivial destructors.

There are several downsides with this approach: those constants get buried lower down in the file and, if related constants exist, such as (in this case quite common) const char kEmail[], they would either be separated or all need to move to class-level members (so the workaround gets viral).

Daniel

P.S. you didn't hear it from me but we don't actually warn about exit-time destructors in tests. The style guide does not provide an exception though :)

I noticed multiple occurrences in chromium tests too, but I would rather pursue a recommended approach for our class that doesn't violate the style guide :-)

 

On Wed, 15 Jan 2025 at 08:52, John Admanski <jadm...@chromium.org> wrote:

Note that std::string (or any class) having a constexpr constructor and destructor is not sufficient for it to be usable for making static constexpr objects. If your class dynamically allocates memory then it can't be used as a constexpr variable regardless of the constexpr-ness of its construction and destruction.

On Wednesday, January 15, 2025 at 8:30:38 AM UTC-8 dani...@webrtc.org wrote:

Note that std::string has constexpr constructor and destructor since c++20

Have you tried to declare GaiaId constructor (and destructor) constexpr?

As per jadmanski@'s response above, I don't think it matters if the constructor is constexpr, as long as the type isn't trivially destructible. I also came across this Google-internal doc that comes to the conclusion that std::string cannot be used for this use-case.

Cheers,

Mikel

[John Admanski]

FWIW weighing in on the general context of whether or not this is okay to use an implicit conversion for, I'd say that this is one of the more straightforward cases where it's reasonable. One fairly good set of criteria for safe conversions as to whether or not a conversion from A -> B is "safe" to be implicit is:

 * The destination type represents the same conceptual value as the src, which is obviously true in the case of GaiaIdLiteral -> GaiaId

 * The range of values of B is a superset of the values of A (i.e. no truncation or data loss), also obviously true here where they represent the same values

 * Conversions are only implicit in one direction, also true as there's no need for a GaiaId -> GaiaIdLiteral conversion

 * The conversion can't fail, again also true here

So this case is a pretty reasonable to argue as being safe. I'd say the only real disadvantage is that it's not cheap, because every conversion is going to do a string copy. Given the context of how this would normally be used though, that seems unlikely to be a problem.

This pattern (having a constexpr-safe literal variation of a type) probably should be more common, but I suspect it isn't mostly because it's just extra work that a lot of people aren't motivated to do. I've considered it and chosen not to do it myself a couple of times, although in those cases this was also influenced by the usage only being limited to a small handful of test files. The more widespread the value type is used, the more beneficial a constexpr-friendly type would be.