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

cmd/compile: remove assert in internal/noder #46329

Closed
rsc opened this issue May 23, 2021 · 9 comments
Closed

cmd/compile: remove assert in internal/noder #46329

rsc opened this issue May 23, 2021 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented May 23, 2021

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.

@rsc rsc added this to the Go1.18 milestone May 23, 2021
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label May 24, 2021
@toothrot
Copy link
Contributor

The Go 1.18 tree is now open, so it may make sense to address this issue now.

@toothrot
Copy link
Contributor

@griesemer @findleyr

@griesemer
Copy link
Contributor

@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.

@danscales
Copy link
Contributor

danscales commented Aug 26, 2021

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.

@mdempsky
Copy link
Member

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.

@toothrot
Copy link
Contributor

toothrot commented Sep 8, 2021

Checking in again, as this issue is labeled a release blocker.

@mdempsky mdempsky added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 8, 2021
@heschi
Copy link
Contributor

heschi commented Sep 29, 2021

Ping again from the release team: what information is this issue waiting on?

@griesemer
Copy link
Contributor

I believe this can be closed, but I'll leave to @mdempsky do decide.

@mdempsky
Copy link
Member

mdempsky commented Oct 4, 2021

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.

@mdempsky mdempsky closed this as completed Oct 4, 2021
@golang golang locked and limited conversation to collaborators Oct 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

8 participants