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 support for "broken" IR and types #51691

Closed
mdempsky opened this issue Mar 16, 2022 · 17 comments
Closed

cmd/compile: remove support for "broken" IR and types #51691

mdempsky opened this issue Mar 16, 2022 · 17 comments
Assignees
Labels
NeedsFix
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 16, 2022

Now that -G=0 mode is gone, types.Type.SetBroke and ir.Node.SetDiag should never be called with true as an argument. I suggest:

  1. We change these methods to base.Fatalf if called with true, and change Broke/Diag to always return false. (We may need to land some fixes first; e.g., seems like test/typeparam/mdempsky/11.go is failing with unified IR currently.)
  2. Do a global replacement of t.Broke() -> false and n.Diag() -> false. (Along with followup simplifications like !false -> true and false && x -> false.)
  3. Remove any if false { ... } code paths.
  4. Replace any t.SetBroke(true) or n.SetDiag(true) code paths with base.Fatalf instead.
@mdempsky mdempsky added the NeedsFix label Mar 16, 2022
@mdempsky mdempsky added this to the Go1.19 milestone Mar 16, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2022

Change https://go.dev/cl/392917 mentions this issue: cmd/compile: detect invalid NIH conversions within unified IR

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2022

Change https://go.dev/cl/392918 mentions this issue: cmd/compile: Fatalf in Type.SetBroke(true) and Node.SetDiag(true)

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2022

Change https://go.dev/cl/393255 mentions this issue: cmd/compile: remove OIOTA

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2022

Change https://go.dev/cl/393256 mentions this issue: cmd/compile: remove unused code from typecheckdef

gopherbot pushed a commit that referenced this issue Mar 16, 2022
Unified IR currently relies on typecheck to diagnose invalid
//go:notinheap conversions, which prevents removing all of
its (otherwise) dead error-reporting code.

This CL updates the unified IR reader to instead proactively diagnose
these invalid conversions. This logic can be removed again once #46731
is implemented, but in the mean time it allows progress on #51691.

Updates #46731.
Updates #51691.

Change-Id: Ifae81aaad770209ec7a67bc10b55660f291e403e
Reviewed-on: https://go-review.googlesource.com/c/go/+/392917
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
gopherbot pushed a commit that referenced this issue Mar 16, 2022
Type.Broke and Node.Diag were used in the legacy typechecker to allow
reporting of multiple errors in a compilation unit, while suppressing
unhelpful follow-on errors. However, that's no longer needed now that
types2 handles (most) user-visible diagnostics.

Updates #51691.

Change-Id: I919c1598d8acebe5703939256bdca3e8d021f7ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/392918
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
gopherbot pushed a commit that referenced this issue Mar 16, 2022
OIOTA used to be used to represent "iota" in the pre-typechecked IR,
before we knew whether it was safe to replace it with a constant
(because it could be redefined as a global symbol later).

However, now types2 handles constant folding, including handling of
"iota". So this can go away.

Updates #51691.

Change-Id: I3cec45b22c4c8f1c357dcc4003292c21ae32aa90
Reviewed-on: https://go-review.googlesource.com/c/go/+/393255
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
gopherbot pushed a commit that referenced this issue Mar 16, 2022
typecheckdef used to be used to handle references to package-level
declarations that hadn't yet been typechecked yet. It's no longer
needed, as the current IR frontends construct package-level
declarations with proper types upfront.

Exception: this code is still used for compiler-generated function
declarations, so that code needs to be kept. Eventually that code can
be moved elsewhere, but for now this CL makes it obvious that the rest
of the code paths really are unused.

Updates #51691.

Change-Id: I5322edb686aaf5dc4627288f3d9ba910a017b41d
Reviewed-on: https://go-review.googlesource.com/c/go/+/393256
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2022

Change https://go.dev/cl/393367 mentions this issue: cmd/compile: remove typecheckdef and Name.Walkdef

gopherbot pushed a commit that referenced this issue Mar 17, 2022
The only remaining use for typecheckdef after CL 393256 is to
typecheck the ONAME node that represents function names, so we might
as well just move that code into tcFunc instead.

Updates #51691.

Change-Id: Icbca51d4b0fb33c90faa95f16254c7171b171d8a
Reviewed-on: https://go-review.googlesource.com/c/go/+/393367
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 18, 2022

Change https://go.dev/cl/393914 mentions this issue: cmd/compile: remove t.Broke() deadcode paths

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 18, 2022

Change https://go.dev/cl/393915 mentions this issue: cmd/compile: remove n.Diag() deadcode paths

gopherbot pushed a commit that referenced this issue Mar 18, 2022
CL 392918 changed t.Broke() to always return false, we can now get rid
of all its deadcode paths.

Updates #51691

Change-Id: I0a2a13def07364e780e4785621690452948e219a
Reviewed-on: https://go-review.googlesource.com/c/go/+/393914
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 18, 2022
CL 392918 changed n.Diag() to always return false, we can now get rid
of all its deadcode paths.

Updates #51691

Change-Id: I64c07970493e7bdcf89df9508ce88132ef4aa4d7
Reviewed-on: https://go-review.googlesource.com/c/go/+/393915
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 21, 2022

Change https://go.dev/cl/394276 mentions this issue: cmd/compile/internal/types: remove Sym.Block and Sym.Lastlineno

gopherbot pushed a commit that referenced this issue Mar 21, 2022
These fields were used for tracking the last scope/position that an
identifier was declared, so that we could report redeclaration
errors. However, redeclaration errors are now diagnosed by types2 (and
typecheck.Redeclared was removed in CL 388537), so these fields can be
safely pruned.

Updates #51691.

Change-Id: Ifd5ea3f6795fadb420913298d59287c95e4669a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/394276
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 22, 2022

Change https://go.dev/cl/394554 mentions this issue: cmd/compile: remove reportTypeLoop

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 22, 2022

Change https://go.dev/cl/394575 mentions this issue: cmd/compile: remove ir.Node.SetDiag

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 22, 2022

Change https://go.dev/cl/394574 mentions this issue: cmd/compile: remove types.Type.SetBroken

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 22, 2022

Change https://go.dev/cl/394557 mentions this issue: cmd/compile/internal/types: remove anyBroke

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 22, 2022

Change https://go.dev/cl/394556 mentions this issue: cmd/compile: remove AllowsGoVersion checks in old typechecker

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 22, 2022

Change https://go.dev/cl/394558 mentions this issue: cmd/compile: remove types.Field.{Broken,SetBroken}

gopherbot pushed a commit that referenced this issue Mar 23, 2022
reportTypeLoop used to be used to report invalid recursive types in old
typechecker.

However, types2 now reports this error, so this can be removed.

Updates #51691

Change-Id: I5e73369dadafb0cc56e682668b32cbd1e1210f2f
Reviewed-on: https://go-review.googlesource.com/c/go/+/394554
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 23, 2022
And use base.Fatalf in code that use t.SetBroke(true) instead.

Updates #51691

Change-Id: I9f3613379dd82d0dd069cdf7b61cbb281810e2e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/394574
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 23, 2022
And use base.Fatalf in code that use n.SetDiag(true) instead.

Updates #51691

Change-Id: Ib3c0b9c89b8d95717391cbe7d424240e288ada1c
Reviewed-on: https://go-review.googlesource.com/c/go/+/394575
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 23, 2022
types2 handles those checks instead.

The only exception is noder.checkEmbed, since when types2
have not known about "//go:embed" pragma yet.

Updates #51691

Change-Id: I74ded03536023fe838f23fa7421e04513f904f66
Reviewed-on: https://go-review.googlesource.com/c/go/+/394556
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Mar 23, 2022
anyBroke now always return false, we can get rid of it.

Updates #51691

Change-Id: Idab5bc9f9f222cc63e50bdde2b23b9404a4bd74e
Reviewed-on: https://go-review.googlesource.com/c/go/+/394557
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 23, 2022
SetBroken is never called and Broken always returns false.

Updates #51691

Change-Id: I7e3fd3f2121268596d2e93a28c69e895bcf802ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/394558
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@cuonglm
Copy link
Member

@cuonglm cuonglm commented Mar 28, 2022

@mdempsky do you think we can close this issue now?

@cuonglm
Copy link
Member

@cuonglm cuonglm commented May 17, 2022

@mdempsky do you think we can close this issue now?

kindly ping 😃

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented May 17, 2022

@cuonglm Thanks, yeah, seems good to close now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

3 participants