Styleguide proposal: implement toString() overrides behind BuildConfig.ENABLE_ASSERTS

[Nate Fischer]

Hi all,

I'd like to recommend that `toString()` overrides be implemented behind `BuildConfig.ENABLE_ASSERTS` (or, `BuildConfig.IS_FOR_TEST`). The goal is to balance the usefulness of writing a custom `toString()` implementation while minimizing the binary size impact. crrev/c/5447835 is the proposed styleguide change (here is a rendered preview of the change).

Some background for how I got here: when working with custom types in tests, it's beneficial to override `toString()`. This can significantly improve the quality of the error output. Consider GURL.java as an example:

// Without a custom toString implementation: Assert.assertEquals(new GURL("https://a.com/"), new GURL("https://b.com/")); // Yields failure output like: java.lang.AssertionError: expected:<org.chromium.url.GURL@37a7dcdd> but was:<org.chromium.url.GURL@568b1403>

// With a custom toString implementation: Assert.assertEquals(new GURL("https://a.com/"), new GURL("https://b.com/")); // Yields failure output like: java.lang.AssertionError: expected:<GURL(https://a.com/)> but was:<GURL(https://b.com/)>

While adding toString() overrides is good for testing and debugging, these toString() overrides are often not needed in the real product. Overrides are not trivial to optimize away, which means these overrides increase binary size. This topic was discussed on an internal mailing list (http://shortn/_UkKdrD49L1) where we aligned that R8 can still optimize the code if we write the custom toString() implementation behind BuildConfig.DCHECK_IS_ON. Although this has been the de facto best practice for a while, I would like to make this practice more discoverable and formalize this with a styleguide rule.

• DCHECK_IS_ON has since been renamed to ENABLE_ASSERTS
• +Andrew Grieve recommended IS_FOR_TEST as another gating condition

Please let me know what you think of this styleguide suggestion, and please feel free to share feedback by replying here or sharing your feedback on crrev/c/5447835.

Nate Fischer | Software Engineer | ntf...@google.com

[Ted Choc]

SGTM -- Will defer to Andrew on the correct way of ensuring this is removed from release builds, but I think the utility in tests is clear enough to me to support this usecase.

- Ted

Nate Fischer | Software Engineer | ntf...@google.com

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CACiwn8bddw-p_M39O_CfQS_GodrdDx3XVsM35gC%3D8MNR2jiX%3DA%40mail.gmail.com.

[Henrique Nakashima]

Some small cons:

1. There's an increased risk of diverging behavior between ENABLE_ASSERTS == true and false making tests not reflect real world behavior. Especially for commonly used classes like GURL, the increased risk is likely not worth the resource savings.  For less commonly used classes, it's a proportionally smaller concern.

2. Make debugging bug reports from logcats harder, though it's hard to measure by how much, and some examples the data would've been elided anyway.

3. More complexity for the style guide.

It has not been the de facto practice, though, as far as I can tell it's done in 2 classes (GURL and VariationsSeedFetcher) out of ~100:

https://source.chromium.org/search?q=%22String%20tostring()%22%20f:java%5Cb%20-f:third_party&sq=&ss=chromium

4. Does not match current state.

I'm lukewarm on adding the recommendation; as none of those are deal breakers, if the pros are significant, I support it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAFs4gPrsoNZyXXEggcC24Y1R%2BYuV7xF_h9-iCKqDYg1i593%2BHA%40mail.gmail.com.

[Andrew Grieve]

I think in the case of GURL, it would probably make sense to just implement toString unconditionally, since it's basically a string wrapper anyways.

Where I think this approach makes the most sense is when the toString is clearly a debug string. E.g. "MyClass(field1=foo, field2=bar)"

As for complexity - I kind of see that as a positive. It's a chance to show how to write debug-only code that R8 will optimize away. I think the more everyone understands how our code is built, the more everyone will be able to write optimized code.

I wonder if ENABLE_ASSERTS and IS_FOR_TEST are not the best fits for the condition check... Could also go with Log.isLoggable(Log.DEBUG). Or maybe we should add a BuildConfig.IS_DEBUG? Could also have a IS_DEBUG_OR_TEST.

[Nate Fischer]

I think the suggestion of "commonly used classes" may be hard to follow in practice. What's the threshold for what we consider commonly used? This rule would be open to interpretation, which means it will be applied inconsistently.

My main motivation is to encourage toString() overrides for the benefit of testing. The current revision of the style guide allows some exceptions, however I read this as largely discouraging toString(): "Use explicit serialization methods... instead of toString() when dynamic dispatch is not required." Would people feel better if we just update the proposal to just explicitly mention testing as one of the cases where dynamic dispatch is required?

Nate Fischer | Software Engineer | ntf...@google.com

[Andrew Grieve]

Regardless of whether a class is "common", I think the most important guidance here is to not introduce unstrippable "toString()" methods if they are not used in the actual app. The language of "dynamic dispatch" alludes to runtime-performance concerns, which I don't think we really have (I think I might have added that... so critiquing myself here).

How about:

Do not add "toString() / "hashCode()" / "equals()" overrides unless they are used in a production environment (not only in tests). If they are helpful to have for testing purposes, then add them in such a way that R8 can remove them. E.g.: ...

[Andrew Grieve]

Note: we have a BuildConfig.ENABLE_DEBUG_LOGS now, which seems like it'd be a good flag to use for this.