From 44d1a8523a50c30354e0b1ef70953567c26eed1a Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Fri, 8 Jan 2021 19:28:24 -0800 Subject: [PATCH] [dev.typeparams] cmd/compile/internal/types2: fixes for all.bash This CL implements a number of minor fixes that were discovered in getting -G=3 working for running all.bash. 1. Field tags were handled incorrectly. If a struct type had some fields with tags, but later fields without tags, the trailing tag-less fields would all copy the tag of the last tagged field. Fixed by simply reinitializing `tag` to "" for each field visited. 2. Change the ending of switch case clause scopes from the end of the last statement to the next "case" token or the switch-ending "}" token. I don't think this is strictly necessary, but it matches my intuition about where case-clause scopes end and cmd/compile's current scoping logic (admittedly influenced by the former). 3. Change select statements to correctly use the scope of each individual communication clause, instead of the scope of the entire select statement. This issue appears to be due to the original go/types code being written to rebind "s" from the *SelectStmt to the Stmt in the range loop, and then being further asserted to "clause" of type *CommClause. In most places within the loop body, "clause" was used, but the rebound "s" identifier was used for the scope boundaries. However, in the syntax AST, SelectStmt directly contains a []*CommClause (rather than a *BlockStmt, with []Stmt), so no assertion is necessary and instead of rebinding "s", the range loop was updated to directly declare "clause". 4. The end position for increment/decrement statements (x++/x--) was incorrectly calculated. Within the syntax AST, these are represented as "x += ImplicitOne", and for AssignStmts types2 calculated the end position as the end position of the RHS operand. But ImplicitOne doesn't have any position information. To workaround this, this CL detects ImplicitOne and then computes the end position of the LHS operand instead, and then adds 2. In practice this should be correct, though it could be wrong for ill-formatted statements like "x ++". Change-Id: I13d4830af39cb3f3b9f0d996672869d3db047ed2 Reviewed-on: https://go-review.googlesource.com/c/go/+/282914 Trust: Matthew Dempsky Trust: Robert Griesemer Reviewed-by: Robert Griesemer --- src/cmd/compile/internal/types2/api_test.go | 4 +-- src/cmd/compile/internal/types2/pos.go | 4 +++ src/cmd/compile/internal/types2/stmt.go | 31 +++++++++++++++------ src/cmd/compile/internal/types2/typexpr.go | 1 + 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/cmd/compile/internal/types2/api_test.go b/src/cmd/compile/internal/types2/api_test.go index d9647b94324..81fc1243e96 100644 --- a/src/cmd/compile/internal/types2/api_test.go +++ b/src/cmd/compile/internal/types2/api_test.go @@ -799,10 +799,10 @@ func TestScopesInfo(t *testing.T) { "file:", "func:", }}, {`package p15; func _(c chan int) { select{ case <-c: } }`, []string{ - "file:", "func:c", "select:", + "file:", "func:c", "comm:", }}, {`package p16; func _(c chan int) { select{ case i := <-c: x := i; _ = x} }`, []string{ - "file:", "func:c", "select:i x", + "file:", "func:c", "comm:i x", }}, {`package p17; func _() { for{} }`, []string{ "file:", "func:", "for:", "block:", diff --git a/src/cmd/compile/internal/types2/pos.go b/src/cmd/compile/internal/types2/pos.go index 4dd839b7dce..0a19cd1a239 100644 --- a/src/cmd/compile/internal/types2/pos.go +++ b/src/cmd/compile/internal/types2/pos.go @@ -286,6 +286,10 @@ func endPos(n syntax.Node) syntax.Pos { return n.Pos() case *syntax.AssignStmt: m = n.Rhs + if m == syntax.ImplicitOne { + p := endPos(n.Lhs) + return syntax.MakePos(p.Base(), p.Line(), p.Col()+2) + } case *syntax.BranchStmt: if n.Label != nil { m = n.Label diff --git a/src/cmd/compile/internal/types2/stmt.go b/src/cmd/compile/internal/types2/stmt.go index 3463cfdf57a..52b9794c102 100644 --- a/src/cmd/compile/internal/types2/stmt.go +++ b/src/cmd/compile/internal/types2/stmt.go @@ -156,7 +156,11 @@ func (check *Checker) multipleSelectDefaults(list []*syntax.CommClause) { } func (check *Checker) openScope(node syntax.Node, comment string) { - scope := NewScope(check.scope, node.Pos(), endPos(node), comment) + check.openScopeUntil(node, endPos(node), comment) +} + +func (check *Checker) openScopeUntil(node syntax.Node, end syntax.Pos, comment string) { + scope := NewScope(check.scope, node.Pos(), end, comment) check.recordScope(node, scope) check.scope = scope } @@ -522,7 +526,7 @@ func (check *Checker) stmt(ctxt stmtContext, s syntax.Stmt) { check.multipleSelectDefaults(s.Body) - for _, clause := range s.Body { + for i, clause := range s.Body { if clause == nil { continue // error reported before } @@ -552,8 +556,11 @@ func (check *Checker) stmt(ctxt stmtContext, s syntax.Stmt) { check.error(clause.Comm, "select case must be send or receive (possibly with assignment)") continue } - - check.openScope(s, "case") + end := s.Rbrace + if i+1 < len(s.Body) { + end = s.Body[i+1].Pos() + } + check.openScopeUntil(clause, end, "case") if clause.Comm != nil { check.stmt(inner, clause.Comm) } @@ -631,14 +638,16 @@ func (check *Checker) switchStmt(inner stmtContext, s *syntax.SwitchStmt) { check.invalidASTf(clause, "incorrect expression switch case") continue } - check.caseValues(&x, unpackExpr(clause.Cases), seen) - check.openScope(clause, "case") + end := s.Rbrace inner := inner if i+1 < len(s.Body) { + end = s.Body[i+1].Pos() inner |= fallthroughOk } else { inner |= finalSwitchCase } + check.caseValues(&x, unpackExpr(clause.Cases), seen) + check.openScopeUntil(clause, end, "case") check.stmtList(inner, clause.Body) check.closeScope() } @@ -681,15 +690,19 @@ func (check *Checker) typeSwitchStmt(inner stmtContext, s *syntax.SwitchStmt, gu var lhsVars []*Var // list of implicitly declared lhs variables seen := make(map[Type]syntax.Pos) // map of seen types to positions - for _, clause := range s.Body { + for i, clause := range s.Body { if clause == nil { check.invalidASTf(s, "incorrect type switch case") continue } + end := s.Rbrace + if i+1 < len(s.Body) { + end = s.Body[i+1].Pos() + } // Check each type in this type switch case. cases := unpackExpr(clause.Cases) T := check.caseTypes(&x, xtyp, cases, seen, false) - check.openScope(clause, "case") + check.openScopeUntil(clause, end, "case") // If lhs exists, declare a corresponding variable in the case-local scope. if lhs != nil { // spec: "The TypeSwitchGuard may include a short variable declaration. @@ -701,6 +714,8 @@ func (check *Checker) typeSwitchStmt(inner stmtContext, s *syntax.SwitchStmt, gu T = x.typ } obj := NewVar(lhs.Pos(), check.pkg, lhs.Value, T) + // TODO(mdempsky): Just use clause.Colon? Why did I even suggest + // "at the end of the TypeSwitchCase" in #16794 instead? scopePos := clause.Pos() // for default clause (len(List) == 0) if n := len(cases); n > 0 { scopePos = endPos(cases[n-1]) diff --git a/src/cmd/compile/internal/types2/typexpr.go b/src/cmd/compile/internal/types2/typexpr.go index 910db0819f7..32377ed3f45 100644 --- a/src/cmd/compile/internal/types2/typexpr.go +++ b/src/cmd/compile/internal/types2/typexpr.go @@ -1110,6 +1110,7 @@ func (check *Checker) structType(styp *Struct, e *syntax.StructType) { typ = check.varType(f.Type) prev = f.Type } + tag = "" if i < len(e.TagList) { tag = check.tag(e.TagList[i]) }