Establish principles for handling DCHECK / s2debug

[alan-strohm]

In the spirit of reserving issues for actionable tasks, I'm creating a discussion about how we are and should be handling DHECKs / s2debug when porting logic from the C++ version.

Background

• In the C++ version, DCHECKs are macros which are not compiled in "prod" binaries. In "debug" binaries (including tests) DCHECKs crash the program if they evaluate to false.
• C++ has an s2debug flag which turns on extra validation globally for the program.
• Several C++ classes (S2Loop, S2Polyline, S2Polygon and S2PolylineVectorLayer) have an s2debug_override option which enables / disables extra validation.
• Our s2.Polyline is just []Point so we can't prevent clients from constructing a polyline by casting (or add an s2debug_override member 😢 ). Polygon and Loop are structs with private fields, so we have more options there.
• Some of these types aren't completely immutable; Polygon.Loops() and Loop.Vertices() return slices that could be modified by the caller, so having called Validate() once isn't a guarantee that the value will be forever valid.

Requirements

• Some of this validation logic is very expensive. Clients should have control over when expensive logic is run.
• If a check is cheap, we should always run it and return an error or invalid object if it fails.
• The design of some objects intentionally allows for them to represent "invalid" values with validity only being required by APIs which need it. This may be a design error: our experience is that clients only create S2 objects when they want to use S2 APIs. However, it is a decision the other versions of s2 aren't forcing clients to move away from.
• We should do what's idiomatic for the language. Go style generally prefers errors that are either made explicit or defined out of existence.

Options

More preferred options are listed first. We would like to avoid introducing modes and flags since those come at a real cost to the API. Those options are listed last for completeness.

Cap parameters / fail gracefully

We could treat erroneous inputs the same as their nearest extreme. Examples:

• We changed AllNeighbors to return nil (e.g. an empty slice) if the caller asks for a level greater than the level of the object.
• siTiToST returns 1 if called with a value larger than its max.
• For algorithms that could get stuck in loops on invalid inputs, we could try to detect that and bail out more gracefully.

Return an error / Must

We could adjust signatures such that Foo() returns (T, error) and MustFoo() returns T or panics and it's the caller's responsibility to know the panic isn't possible. This is an option in cases where the validity check isn't expensive. We could continue to return non-nil objects for clients who don't care about the error. regexp.Compile/regexp.MustCompile are a good example of this pattern.

Add s2debug_override parameters to factory functions and objects

We should probably mirror the C++ where possible. This might be the right thing to do for s2.Polygon.

Add an s2debug library-wide parameter

This could be a flag or something else.

Run extra validation code during tests.

Go has a Testing function to tell whether the current code is running in a test.

Introduce build tags.

The Go build system supports build constraints. We could introduce a tag to represent additional checking. It would be nice to have a pointer to prior art in a different library before embarking on this.

Add comments

At a minimum, we should document the assumptions using comments. It seems like we could do this more consistently.

Example issues demonstrating the problem

• Infinite loop in RegionCoverer Interior Covering #152
• CellId.AllNeighbors has documented but unchecked precondition #104

[alan-strohm]

@flwyd since a lot of the above comes from his comments that I've pulled from other places.

[nes1983]

We've ported a lot of code over from c++. There were dchecks in there. In each case, we thought about how to deal with it in go and different things came out. But there were some constants I'd like to maintain: No difference between production and "debug" code. The go compiler doesn't even have a debug mode. And similarly old gophers don't like modes. If a check is optional, let the client either call it or not. If it's cheap, run it always. But modes and flags are not good. We got here without them and we have a fairly clean api still.

…

On Thu, 24 Apr 2025, 22:16 Alan Strohm, ***@***.***> wrote: @flwyd <https://github.com/flwyd> since a lot of the above comes from his comments that I've pulled from other places. — Reply to this email directly, view it on GitHub <#165 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAY353S73PTOWCPKZ43WVL23FBDXAVCNFSM6AAAAAB3Z6WH6SVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTEOJTHE3DAMI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[alan-strohm]

Thanks for the feedback! If you have good examples of places where you think we've handled this well, let me know. I can highlight the patterns for others to emulate.

I've updated the proposal:

• Sorted the options in order of preference.
• Put all mode and flag options at the end.
• Stated explicitly that we'd like to avoid those. Ideally we'd have a crisper explanation of "why", but I agree with you intuitively.
• Added a requirement "If a check is cheap, we should always run it and return an error or invalid object if it fails."

I'm thinking we can leave this discussion to collect other examples / learnings and to reference. If we continue to observe confusion, we can summarize our principles into a doc.