Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errbase: prevent a call cycle for Formatter/SafeFormatter errors #90

Merged
merged 1 commit into from
Jan 18, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Jan 18, 2022

Fixes #88.

We've found a need somewhere to implement an Error() method on a
leaf by delegating the behavior to the Formatter or SafeFormatter
interfaces. In that case, the errors.IsAny call inside the
special handler causes a call cycle.

This commit breaks the cycle by handling the special cases after
the interfaces.

A possible regression would be that one of the stdlib errors
starts implementing fmt.Formatter, whereby we'd lose the special
handling (which, as of this writing, extracts safe strings).
At the time of this writing, none of the stdlib errors of interest
implement fmt.Formatter, so we're punting dealing with that
when/if it arises in the future.


This change is Reviewable

We've found a need somewhere to implement an `Error()` method on a
leaf by delegating the behavior to the `Formatter` or `SafeFormatter`
interfaces. In that case, the `errors.IsAny` call inside the
special handler causes a call cycle.

This commit breaks the cycle by handling the special cases after
the interfaces.

A possible regression would be that one of the stdlib errors
starts implementing `fmt.Formatter`, whereby we'd lose the special
handling (which, as of this writing, extracts safe strings).
At the time of this writing, none of the stdlib errors of interest
implement `fmt.Formatter`, so we're punting dealing with that
when/if it arises in the future.
@knz knz requested a review from tbg January 18, 2022 13:35
@tbg
Copy link
Member

tbg commented Jan 18, 2022

Thanks! It would take me a long time to really scrutinize the changes to the control flow and I have not attempted to do that. However, I see how you're solving the problem and how the test cases demonstrate that.

@knz
Copy link
Contributor Author

knz commented Jan 18, 2022

Thanks!

@knz knz merged commit 7e57867 into cockroachdb:master Jan 18, 2022
@knz knz deleted the 20220118-fmt-rec branch January 18, 2022 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

infinite recursion for seemingly idiomatic impl of Error() string for redactable error
2 participants