Conform to CustomDebugStringConvertible for Debug only

[dflems]

Resolves #1245

This boxes all references to CustomDebugStringConvertible and debugDescription in #if checks to exclude them from the release binary. The existence of this conformance and default implementation on the Message protocol (and some other types) adds up quite considerably. Across our large iOS app, we can attribute 6.7 MB of the uncompressed binary size to generated swift protos. This change cuts that back by 780 KB (11.6%). Other efforts like #1240 will probably also help trim this back even further.

I tried to do this with as little diff as possible. Added some inline comments on some things I'm unsure about.

Special thanks to @bubski for the idea.

[Lukasa]

I'll note here that this change can only be made in a major release, so we can only merge this into the proposed 2.0 release.

[thomasvl]

My first thought (and haven't really thought about this) - Is it better to only have the conformance in DEBUG builds or is it better that the implementation be different in DEBUG builds? i.e. - how would this impact other code? Does this force folks to wrap all the places that would end up using this conformance also in guards?

[bubski]

The documentation of CustomDebugStringConvertible states that:

Because the String(reflecting:) initializer works for instances of any type, returning an instance’s debugDescription if the value passed conforms to CustomDebugStringConvertible, accessing a type’s debugDescription property directly or using CustomDebugStringConvertible as a generic constraint is discouraged.

which makes me believe that it should not be a problem, and in cases where engineers do rely on CustomDebugStringConvertible or debugString they should rather refactor their code to use String(reflecting:)/debugPrint(_:) (which would compile for both debug and release builds) instead of wrapping their code in #if checks.

---

As for having conformance for both configurations, but with different implementations, I suspect it not to change much, if anything.
As far as I understand the cost comes mainly from type metadata, rather than specifics of the implementation.
Each type then also implements debugDescription which only forwards to the default implementation shared between all of them.

[Lukasa]

Whether engineers should obey the rules above is largely immaterial IMO: this breaks the API. Code that used to work would update to this version and break, and while we can say that the engineers who did that work were holding CustomDebugStringConvertible wrong, they were doing it nonetheless.

We can choose to accept the breakage if we want to, but it would be better if we did this as part of a 2.0 release where that kind of breakage is expected.

[thomasvl]

Now would be the time, since our next release off main is going to be 2.0 since decided to do some of the breaks now.

[bubski]

@Lukasa To be clear, I do agree that this, in its current form, is an API breaking change and should be treated as such. I didn't mean to suggest otherwise.
I only wanted to share some perspective on how reasonable/fitting this approach could be.

[Lukasa]

@bubski Fantastic. I think this approach is entirely fine by me, though it might be nice to be able to allow users to override this behaviour if they so choose by replacing the definition with #if DEBUG || PROTOBUF_ENABLE_DEBUG_STRINGS. Otherwise I'm +1 on the general shape.

[bubski]

@Lukasa in case you haven't seen it, the reason we went with DEBUG alone here were concerns around having custom compilation flags brought up in #1245.
Having more granular control over this does sound pleasing, but I am not sure what would be most ergonomic to engineers.

[thomasvl]

@tbkka @Lukasa I'm not sure if @allevato has any strong opinions, but personally, I'll defer to you all on landing something like this.

If we find a solution for #18 (finishing out #1240), it should get but would also raise the question where/how these methods should be implemented.

[tbkka]

I think we should take this for 2.0.

[Lukasa]

Agree.

[allevato]

No concerns here.

[thomasvl]

Looks like the sanitizer configs are the only things that catch building the tests in release. There's a bunch of tests that will need some work to deal with building in both debug and release.

[dflems]

@thomasvl RE: tests that access debugDescription should I box those out with an #if DEBUG? should probably be able to find them easily enough by running the tests in the release config

[tbkka]

Yeah, disabling the debugDescription tests in release builds seems like the right answer.

[dflems]

Added an assertDebugDescription() variant where you can pass in an existing SwiftProtobuf.Message. Both of the functions no-op for non-debug builds.

[dflems]

@thomasvl @tbkka made the requested changes and fixed the tests

[tbkka]

Bleargh. Bug in the upstream conformance test.

In file included from conformance_cpp.cc:54:0:
conformance_cpp.cc: In member function 'google::protobuf::util::statusor_internal::StatusOr<bool> google::protobuf::{anonymous}::Harness::ServeConformanceRequest()':
conformance_cpp.cc:235:61: error: invalid conversion from 'const char*' to 'char*' [-fpermissive]
   RETURN_IF_ERROR(ReadFd(STDIN_FILENO, serialized_input.data(), in_len));
                                        ~~~~~~~~~~~~~~~~~~~~~^~
../src/google/protobuf/stubs/status_macros.h:58:10: note: in definition of macro 'RETURN_IF_ERROR'
         (expr);                                                              \
          ^~~~

[thomasvl]

upstream conformance test finally fixed, I re-triggered CI.

[dflems]

@tbkka Is there going to be an alpha branch for 2.0? Or is main going to track 2.x and 1.x will branch off from main?

[thomasvl]

main is already tracking towards 2.0.

Personally, I don't think we'll have another 1.0 release unless there is some issue that comes up. But that's ultimately up to the Apple folks.

[thomasvl]

Did this need anything before landing (since main headed to 2.0 and thus accepting breaking changes) aside from approvals by @Lukasa @tbkka ?

[tbkka]

Nice work! I've not been able to review the actual implementation as closely as I'd like, but the idea is good and I'm happy taking this for 2.0.

[thomasvl]

@Lukasa this ok to merge?

[Lukasa]

LGTM, I'm happy with it. Nice patch!

[thomasvl]

Ok, merging…

Thanks for the patch!

[acecilia]

👋 Sorry I am a bit confused: seems like this change never made it to the public right? As the latest releases are version 1.X, but this change is marked as v2.0?

By looking at the commit, looks to be only present in main, and not being part of any of the releases 🤔

[thomasvl]

Correct, it wasn't put on the 1.x branch as it could be considered a breaking change since it changes behaviors in a Release build, so the major change seemed safer.

[acecilia]

Thanks! In case I would like to build SwiftProtobuf with any similar breaking changes done in the past 2 years, is there a recommended way? Could you confirm: since there hasn't been any 2.X release yet, looks like the only option would be to build from master?

[thomasvl]

Yes, main would be the only way to get something with those changes, but as there has been no release, you could encounter a break if something else changes on main in the future.

As far as how to build off main, that depends on your specific project, and I'm not sure what the steps would be for CocoaPods or SwiftPM, can't say I've personally tried it.