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

Jump to bottom

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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.19

Comments

@mdempsky
Copy link
Member

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 The path to resolution is known, but the work has not been done. label Mar 16, 2022
@mdempsky mdempsky added this to the Go1.19 milestone Mar 16, 2022
@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
@mdempsky
cmd/compile: detect invalid NIH conversions within unified IR
a1bf50e 
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
@mdempsky
cmd/compile: Fatalf in Type.SetBroke(true) and Node.SetDiag(true)
1024503 
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
@mdempsky
cmd/compile: remove OIOTA
d661bde 
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
@mdempsky
cmd/compile: remove unused code from typecheckdef
81d3c25 
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

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
@mdempsky
cmd/compile: remove typecheckdef and Name.Walkdef
1cfe100 
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

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

@gopherbot
Copy link

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
@cuonglm
cmd/compile: remove t.Broke() deadcode paths
da9649e 
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
@cuonglm
cmd/compile: remove n.Diag() deadcode paths
7ca6902 
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

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
@mdempsky
cmd/compile/internal/types: remove Sym.Block and Sym.Lastlineno
79103fa 
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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
@cuonglm
cmd/compile: remove reportTypeLoop
7e0a1d3 
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
@cuonglm
cmd/compile: remove types.Type.SetBroken
515135f 
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
@cuonglm
cmd/compile: remove ir.Node.SetDiag
c10fb50 
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
@cuonglm
cmd/compile: remove AllowsGoVersion checks in old typechecker
810868c 
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
@cuonglm
cmd/compile/internal/types: remove anyBroke
2792a41 
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
@cuonglm
cmd/compile: remove types.Field.{Broken,SetBroken}
0eb2c77 
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 commented Mar 28, 2022

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

@cuonglm
Copy link
Member

cuonglm commented May 17, 2022

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

kindly ping 😃

@mdempsky
Copy link
Member Author

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

@rsc rsc unassigned mdempsky and cuonglm Jun 22, 2022
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
3 participants
@mdempsky @cuonglm @gopherbot