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
NeedsFix release-blocker WaitingForInfo
Milestone

Comments

@rsc
Copy link
Contributor

@rsc 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 label May 24, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Aug 25, 2021

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

@toothrot
Copy link
Contributor

@toothrot toothrot commented Aug 25, 2021

@griesemer @findleyr

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 26, 2021

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

@mdempsky mdempsky commented Aug 26, 2021

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 toothrot commented Sep 8, 2021

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

@mdempsky mdempsky added the WaitingForInfo label Sep 8, 2021
@heschi
Copy link
Contributor

@heschi heschi commented Sep 29, 2021

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

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 29, 2021

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

@mdempsky
Copy link
Member

@mdempsky 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix release-blocker WaitingForInfo
Projects
None yet
Development

No branches or pull requests

7 participants