Change https://golang.org/cl/274242 mentions this issue: cryptobyte: fix ReadOptionalASN1Boolean

Indeed, ReadOptionalASN1Boolean is completely broken at the moment, and it seems to be fully unused.

I support fixing it before tagging v1 of x/crypto, despite considering x/crypto stable already, because 1) nothing will break as far as I can tell and 2) anything that breaks was very broken already.

The proposal is to change it to this

(s *String) ReadOptionalASN1Boolean(out *bool, tag asn1.Tag, defaultValue bool) bool

Note that adding the tag asn1.Tag argument and breaking the API is not actually necessary to fix the bug, but 1) it makes ReadOptionalASN1Boolean actually useful since OPTIONALs are commonly explicitly tagged, 2) it makes it consistent with other ReadOptionalASN1, and 3) it prevents confusion with the previous ReadOptionalASN1Boolean implementation.

Over to @golang/proposal-review since it's an API change.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Given that the function fails today (does not do what it should) and cannot do what it should without a new argument and for that reason appears to be completely unused, it seems OK to make an API breaking change here.

We did this in the past for a function in syscall that was impossible to call because it took an argument of unexported type. This is not quite that strictly impossible but still seems reasonable.

Have all remaining concerns about this proposal been addressed?

I believe so.

I still had some hesitation about making a breaking change, so I scanned the latest version of each module available on the module proxy for ReadOptionalASN1Boolean (about a million modules). I found 779 copies of the code but zero calls to it. This does seem to be the exceptional case where we can reasonably redefine the existing API.

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

In package cryptobyte, change the (*String).ReadOptionalASN1Boolean method signature to:

(s *String) ReadOptionalASN1Boolean(out *bool, tag asn1.Tag, defaultValue bool) bool

This adds a new “tag” parameter so that the code can tell whether the optional boolean is present or not. The current function signature is unfixably broken and impossible to use correctly.

Even unfixably broken and impossible to use correctly is not enough to merit an actual breaking API change. We also scanned all public Go code in the module proxy and found not a single use of this method, suggesting that it really is safe to redefine.

(It is not surprising no one uses it, since if they did, the code wouldn’t work! But that doesn’t always stop people. In this case, the method is also obscure enough that we seem to have gotten lucky.)

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

In package cryptobyte, change the (*String).ReadOptionalASN1Boolean method signature to:

(s *String) ReadOptionalASN1Boolean(out *bool, tag asn1.Tag, defaultValue bool) bool

This adds a new “tag” parameter so that the code can tell whether the optional boolean is present or not. The current function signature is unfixably broken and impossible to use correctly.

Even unfixably broken and impossible to use correctly is not enough to merit an actual breaking API change. We also scanned all public Go code in the module proxy and found not a single use of this method, suggesting that it really is safe to redefine.

(It is not surprising no one uses it, since if they did, the code wouldn’t work! But that doesn’t always stop people. In this case, the method is also obscure enough that we seem to have gotten lucky.)