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
cmd/compile: remove assert in internal/noder #46329
Comments
The Go 1.18 tree is now open, so it may make sense to address this issue now. |
@mdempsky @danscales I've looked at the noder code but I'm not sure what there is to do here. This can't be about removing all asserts, and I didn't see an obvious TODO related to a specific assert in a quick search. |
I think Russ is saying we should remove the Assert function and all its uses eventually (possibly replacing many of them with a panic with an error message). Maybe we can check with him again on this. In any case, I think we can wait on this until near the end of the release cycle. |
There's base.Assertf now, so we can more easily include more details in the panic message. But in general, the asserts in noder are being used like the asserts in types2: as extra consistency checks that should never fail; or if they do, then the error message isn't going to be actionable by end users anyway. |
Checking in again, as this issue is labeled a release blocker. |
Ping again from the release team: what information is this issue waiting on? |
I believe this can be closed, but I'll leave to @mdempsky do decide. |
I don't think there's any action to take here. The asserts are just internal consistency checks. They're not expected to fire on user code. |
There's a func assert in cmd/compile/internal/noder with a TODO to remove.
(It would be a bad experience for users to hit the panic instead of a good error message.)
It's only reachable in the generics implementation, so it doesn't need to be
removed until Go 1.18. Filing this issue to make sure we remember.
The text was updated successfully, but these errors were encountered: