From: Robert Griesemer Date: Tue, 31 Oct 2023 21:22:05 +0000 (-0700) Subject: go/parser: better error messages for incorrect type parameter list X-Git-Tag: go1.22rc1~453 X-Git-Url: http://www.git.cypherpunks.ru/?a=commitdiff_plain;h=285ac5a11e36ca4d85a7e97c9040a1e3de0ecc11;p=gostls13.git go/parser: better error messages for incorrect type parameter list This is a port of CL 538856 from the syntax parser to go/parser. As part of the port, make more portions of parseParameterList matching the equivalent paramList method (from the syntax parser). As a result, this now also produces a better error message in cases where the missing piece might not be a type parameter name but a constraint (this fixes a TODO in a test). Improve comments in the code and adjust the corresponding comments in the syntax parser. Change references to issues to use the format go.dev/issue/ddddd. For #60812. Change-Id: Ia243bd78161ed8543d3dc5deb20ca4a215c5b1e4 Reviewed-on: https://go-review.googlesource.com/c/go/+/538858 Reviewed-by: Robert Griesemer Auto-Submit: Robert Griesemer Reviewed-by: Robert Findley Run-TryBot: Robert Griesemer TryBot-Result: Gopher Robot --- diff --git a/src/cmd/compile/internal/syntax/parser.go b/src/cmd/compile/internal/syntax/parser.go index 140f00537a..1569b5e987 100644 --- a/src/cmd/compile/internal/syntax/parser.go +++ b/src/cmd/compile/internal/syntax/parser.go @@ -2021,7 +2021,7 @@ func (p *parser) paramList(name *Name, typ Expr, close token, requireNames bool) // distribute parameter types (len(list) > 0) if named == 0 && !requireNames { - // all unnamed => found names are named types + // all unnamed and we're not in a type parameter list => found names are named types for _, par := range list { if typ := par.Name; typ != nil { par.Type = typ @@ -2029,32 +2029,38 @@ func (p *parser) paramList(name *Name, typ Expr, close token, requireNames bool) } } } else if named != len(list) { - // some named => all must have names and types - var pos Pos // left-most error position (or unknown) - var typ Expr // current type (from right to left) + // some named or we're in a type parameter list => all must be named + var errPos Pos // left-most error position (or unknown) + var typ Expr // current type (from right to left) for i := len(list) - 1; i >= 0; i-- { par := list[i] if par.Type != nil { typ = par.Type if par.Name == nil { - pos = StartPos(typ) - par.Name = NewName(pos, "_") + errPos = StartPos(typ) + par.Name = NewName(errPos, "_") } } else if typ != nil { par.Type = typ } else { // par.Type == nil && typ == nil => we only have a par.Name - pos = par.Name.Pos() + errPos = par.Name.Pos() t := p.badExpr() - t.pos = pos // correct position + t.pos = errPos // correct position par.Type = t } } - if pos.IsKnown() { + if errPos.IsKnown() { var msg string if requireNames { + // Not all parameters are named because named != len(list). + // If named == typed we must have parameters that have no types, + // and they must be at the end of the parameter list, otherwise + // the types would have been filled in by the right-to-left sweep + // above and we wouldn't have an error. Since we are in a type + // parameter list, the missing types are constraints. if named == typed { - pos = end // position error at closing ] + errPos = end // position error at closing ] msg = "missing type constraint" } else { msg = "missing type parameter name" @@ -2066,7 +2072,7 @@ func (p *parser) paramList(name *Name, typ Expr, close token, requireNames bool) } else { msg = "mixed named and unnamed parameters" } - p.syntaxErrorAt(pos, msg) + p.syntaxErrorAt(errPos, msg) } } diff --git a/src/go/parser/parser.go b/src/go/parser/parser.go index 7d8f727b0c..a28960523e 100644 --- a/src/go/parser/parser.go +++ b/src/go/parser/parser.go @@ -880,26 +880,26 @@ func (p *parser) parseParameterList(name0 *ast.Ident, typ0 ast.Expr, closing tok // Type parameters are the only parameter list closed by ']'. tparams := closing == token.RBRACK - // Type set notation is ok in type parameter lists. - typeSetsOK := tparams - pos := p.pos - if name0 != nil { - pos = name0.Pos() - } + // Note: The code below matches the corresponding code in the syntax + // parser closely. Changes must be reflected in either parser. + // For the code to match, we use the local []field list that + // corresponds to []syntax.Field. At the end, the list must be + // converted into an []*ast.Field. var list []field var named int // number of parameters that have an explicit name and type + var typed int // number of parameters that have an explicit type for name0 != nil || p.tok != closing && p.tok != token.EOF { var par field if typ0 != nil { - if typeSetsOK { + if tparams { typ0 = p.embeddedElem(typ0) } par = field{name0, typ0} } else { - par = p.parseParamDecl(name0, typeSetsOK) + par = p.parseParamDecl(name0, tparams) } name0 = nil // 1st name was consumed if present typ0 = nil // 1st typ was consumed if present @@ -908,6 +908,9 @@ func (p *parser) parseParameterList(name0 *ast.Ident, typ0 ast.Expr, closing tok if par.name != nil && par.typ != nil { named++ } + if par.typ != nil { + typed++ + } } if !p.atComma("parameter list", closing) { break @@ -919,12 +922,9 @@ func (p *parser) parseParameterList(name0 *ast.Ident, typ0 ast.Expr, closing tok return // not uncommon } - // TODO(gri) parameter distribution and conversion to []*ast.Field - // can be combined and made more efficient - - // distribute parameter types - if named == 0 { - // all unnamed => found names are type names + // distribute parameter types (len(list) > 0) + if named == 0 && !tparams { + // all unnamed and we're not in a type parameter list => found names are type names for i := 0; i < len(list); i++ { par := &list[i] if typ := par.name; typ != nil { @@ -932,43 +932,55 @@ func (p *parser) parseParameterList(name0 *ast.Ident, typ0 ast.Expr, closing tok par.name = nil } } - if tparams { - p.error(pos, "type parameters must be named") - } } else if named != len(list) { - // some named => all must be named - ok := true - var typ ast.Expr - missingName := pos + // some named or we're in a type parameter list => all must be named + var errPos token.Pos // left-most error position (or invalid) + var typ ast.Expr // current type (from right to left) for i := len(list) - 1; i >= 0; i-- { if par := &list[i]; par.typ != nil { typ = par.typ if par.name == nil { - ok = false - missingName = par.typ.Pos() + errPos = typ.Pos() n := ast.NewIdent("_") - n.NamePos = typ.Pos() // correct position + n.NamePos = errPos // correct position par.name = n } } else if typ != nil { par.typ = typ } else { // par.typ == nil && typ == nil => we only have a par.name - ok = false - missingName = par.name.Pos() - par.typ = &ast.BadExpr{From: par.name.Pos(), To: p.pos} + errPos = par.name.Pos() + par.typ = &ast.BadExpr{From: errPos, To: p.pos} } } - if !ok { + if errPos.IsValid() { + var msg string if tparams { - p.error(missingName, "type parameters must be named") + // Not all parameters are named because named != len(list). + // If named == typed we must have parameters that have no types, + // and they must be at the end of the parameter list, otherwise + // the types would have been filled in by the right-to-left sweep + // above and we wouldn't have an error. Since we are in a type + // parameter list, the missing types are constraints. + if named == typed { + errPos = p.pos // position error at closing ] + msg = "missing type constraint" + } else { + msg = "missing type parameter name" + // go.dev/issue/60812 + if len(list) == 1 { + msg += " or invalid array length" + } + } } else { - p.error(pos, "mixed named and unnamed parameters") + msg = "mixed named and unnamed parameters" } + p.error(errPos, msg) } } - // convert list []*ast.Field + // Convert list to []*ast.Field. + // If list contains types only, each type gets its own ast.Field. if named == 0 { // parameter list consists of types only for _, par := range list { @@ -978,7 +990,8 @@ func (p *parser) parseParameterList(name0 *ast.Ident, typ0 ast.Expr, closing tok return } - // parameter list consists of named parameters with types + // If the parameter list consists of named parameters with types, + // collect all names with the same types into a single ast.Field. var names []*ast.Ident var typ ast.Expr addParams := func() { @@ -1545,7 +1558,7 @@ func (p *parser) parseIndexOrSliceOrInstance(x ast.Expr) ast.Expr { if ncolons == 2 { slice3 = true // Check presence of middle and final index here rather than during type-checking - // to prevent erroneous programs from passing through gofmt (was issue 7305). + // to prevent erroneous programs from passing through gofmt (was go.dev/issue/7305). if index[1] == nil { p.error(colons[0], "middle index required in 3-index slice") index[1] = &ast.BadExpr{From: colons[0] + 1, To: colons[1]} @@ -2534,7 +2547,7 @@ func (p *parser) parseGenericType(spec *ast.TypeSpec, openPos token.Pos, name0 * closePos := p.expect(token.RBRACK) spec.TypeParams = &ast.FieldList{Opening: openPos, List: list, Closing: closePos} // Let the type checker decide whether to accept type parameters on aliases: - // see issue #46477. + // see go.dev/issue/46477. if p.tok == token.ASSIGN { // type alias spec.Assign = p.pos diff --git a/src/go/parser/parser_test.go b/src/go/parser/parser_test.go index 0848aca22a..e72c03a3d4 100644 --- a/src/go/parser/parser_test.go +++ b/src/go/parser/parser_test.go @@ -319,7 +319,7 @@ const pi = 3.1415 /* 3a */ // 3b /* 3c */ const e = 2.7182 -// Example from issue 3139 +// Example from go.dev/issue/3139 func ExampleCount() { fmt.Println(strings.Count("cheese", "e")) fmt.Println(strings.Count("five", "")) // before & after each rune @@ -335,7 +335,7 @@ func ExampleCount() { {"/* 1a */", "/* 1b */", "/* 1c */", "// 1d"}, {"/* 2a\n*/", "// 2b"}, {"/* 3a */", "// 3b", "/* 3c */"}, - {"// Example from issue 3139"}, + {"// Example from go.dev/issue/3139"}, {"// before & after each rune"}, {"// Output:", "// 3", "// 5"}, } @@ -735,7 +735,7 @@ func TestScopeDepthLimit(t *testing.T) { } } -// proposal #50429 +// proposal go.dev/issue/50429 func TestRangePos(t *testing.T) { testcases := []string{ "package p; func _() { for range x {} }", diff --git a/src/go/parser/resolver.go b/src/go/parser/resolver.go index 1539dcd5c7..d1e1834e53 100644 --- a/src/go/parser/resolver.go +++ b/src/go/parser/resolver.go @@ -136,7 +136,7 @@ func (r *resolver) declare(decl, data any, scope *ast.Scope, kind ast.ObjKind, i obj.Decl = decl obj.Data = data // Identifiers (for receiver type parameters) are written to the scope, but - // never set as the resolved object. See issue #50956. + // never set as the resolved object. See go.dev/issue/50956. if _, ok := decl.(*ast.Ident); !ok { ident.Obj = obj } @@ -209,7 +209,7 @@ func (r *resolver) resolve(ident *ast.Ident, collectUnresolved bool) { } assert(obj.Name != "", "obj with no name") // Identifiers (for receiver type parameters) are written to the scope, - // but never set as the resolved object. See issue #50956. + // but never set as the resolved object. See go.dev/issue/50956. if _, ok := obj.Decl.(*ast.Ident); !ok { ident.Obj = obj } @@ -285,7 +285,7 @@ func (r *resolver) Visit(node ast.Node) ast.Visitor { } for _, e := range n.Elts { if kv, _ := e.(*ast.KeyValueExpr); kv != nil { - // See issue #45160: try to resolve composite lit keys, but don't + // See go.dev/issue/45160: try to resolve composite lit keys, but don't // collect them as unresolved if resolution failed. This replicates // existing behavior when resolving during parsing. if ident, _ := kv.Key.(*ast.Ident); ident != nil { diff --git a/src/go/parser/short_test.go b/src/go/parser/short_test.go index f9575e1d0f..3a34e8c216 100644 --- a/src/go/parser/short_test.go +++ b/src/go/parser/short_test.go @@ -43,7 +43,7 @@ var valids = []string{ `package p; func _() { map[int]int{}[0]++; map[int]int{}[0] += 1 }`, `package p; func _(x interface{f()}) { interface{f()}(x).f() }`, `package p; func _(x chan int) { chan int(x) <- 0 }`, - `package p; const (x = 0; y; z)`, // issue 9639 + `package p; const (x = 0; y; z)`, // go.dev/issue/9639 `package p; var _ = map[P]int{P{}:0, {}:1}`, `package p; var _ = map[*P]int{&P{}:0, {}:1}`, `package p; type T = int`, @@ -172,21 +172,21 @@ var invalids = []string{ `package p; type _ struct{ *( /* ERROR "cannot parenthesize embedded type" */ int) }`, `package p; type _ struct{ *( /* ERROR "cannot parenthesize embedded type" */ []byte) }`, - // issue 8656 + // go.dev/issue/8656 `package p; func f() (a b string /* ERROR "missing ','" */ , ok bool)`, - // issue 9639 + // go.dev/issue/9639 `package p; var x, y, z; /* ERROR "expected type" */`, - // issue 12437 + // go.dev/issue/12437 `package p; var _ = struct { x int, /* ERROR "expected ';', found ','" */ }{};`, `package p; var _ = struct { x int, /* ERROR "expected ';', found ','" */ y float }{};`, - // issue 11611 + // go.dev/issue/11611 `package p; type _ struct { int, } /* ERROR "expected 'IDENT', found '}'" */ ;`, `package p; type _ struct { int, float } /* ERROR "expected type, found '}'" */ ;`, - // issue 13475 + // go.dev/issue/13475 `package p; func f() { if true {} else ; /* ERROR "expected if statement or block" */ }`, `package p; func f() { if true {} else defer /* ERROR "expected if statement or block" */ f() }`, @@ -195,8 +195,7 @@ var invalids = []string{ `package p; var _ func[ /* ERROR "must have no type parameters" */ T any](T)`, `package p; func _[]/* ERROR "empty type parameter list" */()`, - // TODO(rfindley) a better location would be after the ']' - `package p; type _[A /* ERROR "type parameters must be named" */ ,] struct{ A }`, + `package p; type _[A,] /* ERROR "missing type constraint" */ struct{ A }`, `package p; func _[type /* ERROR "found 'type'" */ P, *Q interface{}]()`, diff --git a/src/go/parser/testdata/issue11377.src b/src/go/parser/testdata/issue11377.src index 1c438003eb..a19b86e7ab 100644 --- a/src/go/parser/testdata/issue11377.src +++ b/src/go/parser/testdata/issue11377.src @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Test case for issue 11377: Better synchronization of +// Test case for go.dev/issue/11377: Better synchronization of // parser after certain syntax errors. package p diff --git a/src/go/parser/testdata/issue23434.src b/src/go/parser/testdata/issue23434.src index 24a0832347..f04ca1da89 100644 --- a/src/go/parser/testdata/issue23434.src +++ b/src/go/parser/testdata/issue23434.src @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Test case for issue 23434: Better synchronization of +// Test case for go.dev/issue/23434: Better synchronization of // parser after missing type. There should be exactly // one error each time, with now follow errors. diff --git a/src/go/parser/testdata/issue3106.src b/src/go/parser/testdata/issue3106.src index 2db10be235..37dfb2a52f 100644 --- a/src/go/parser/testdata/issue3106.src +++ b/src/go/parser/testdata/issue3106.src @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Test case for issue 3106: Better synchronization of +// Test case for go.dev/issue/3106: Better synchronization of // parser after certain syntax errors. package main diff --git a/src/go/parser/testdata/issue34946.src b/src/go/parser/testdata/issue34946.src index 6bb15e10c7..87b703d0c9 100644 --- a/src/go/parser/testdata/issue34946.src +++ b/src/go/parser/testdata/issue34946.src @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Test case for issue 34946: Better synchronization of +// Test case for go.dev/issue/34946: Better synchronization of // parser for function declarations that start their // body's opening { on a new line. diff --git a/src/go/parser/testdata/issue44504.src b/src/go/parser/testdata/issue44504.src index 7791f4a809..c46c79f7da 100644 --- a/src/go/parser/testdata/issue44504.src +++ b/src/go/parser/testdata/issue44504.src @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Test case for issue 44504: panic due to duplicate resolution of slice/index +// Test case for go.dev/issue/44504: panic due to duplicate resolution of slice/index // operands. We should not try to resolve a LHS expression with invalid syntax. package p diff --git a/src/go/parser/testdata/issue49175.go2 b/src/go/parser/testdata/issue49175.go2 index cf1c83c633..df303ada40 100644 --- a/src/go/parser/testdata/issue49175.go2 +++ b/src/go/parser/testdata/issue49175.go2 @@ -10,4 +10,4 @@ type _[_ [1]t]t func _[_ []t]() {} func _[_ [1]t]() {} -type t [t /* ERROR "type parameters must be named" */ [0]]t +type t [t /* ERROR "missing type parameter name or invalid array length" */ [0]]t diff --git a/src/go/parser/testdata/resolution/typeparams.go2 b/src/go/parser/testdata/resolution/typeparams.go2 index 7395ca2a34..0f894d7c93 100644 --- a/src/go/parser/testdata/resolution/typeparams.go2 +++ b/src/go/parser/testdata/resolution/typeparams.go2 @@ -47,5 +47,5 @@ func f /* =@f */[T1 /* =@T1 */ interface{~[]T2 /* @T2 */}, T2 /* =@T2 */ any]( var t1var /* =@t1var */ T1 /* @T1 */ } -// From issue #39634 +// From go.dev/issue/39634 func(*ph1[e, e])h(d) diff --git a/src/go/parser/testdata/tparams.go2 b/src/go/parser/testdata/tparams.go2 index 1a9a6c635d..3293b559bb 100644 --- a/src/go/parser/testdata/tparams.go2 +++ b/src/go/parser/testdata/tparams.go2 @@ -4,8 +4,8 @@ package p -type _[a /* ERROR "type parameters must be named" */, b] struct{} -type _[a t, b t, c /* ERROR "type parameters must be named" */ ] struct{} +type _[a, b] /* ERROR "missing type constraint" */ struct{} +type _[a t, b t, c] /* ERROR "missing type constraint" */ struct{} type _ struct { t [n]byte t[a] @@ -25,13 +25,13 @@ type _ interface { } func _[] /* ERROR "empty type parameter list" */ () -func _[a /* ERROR "type parameters must be named" */, b ]() -func _[a t, b t, c /* ERROR "type parameters must be named" */ ]() +func _[a, b ] /* ERROR "missing type constraint" */ () +func _[a t, b t, c] /* ERROR "missing type constraint" */ () // TODO(rfindley) incorrect error message (see existing TODO in parser) func f[a b, 0 /* ERROR "expected '\)', found 0" */ ] () -// issue #49482 +// go.dev/issue/49482 type ( _[a *[]int] struct{} _[a *t,] struct{} @@ -43,7 +43,7 @@ type ( _[a *struct{}|~t] struct{} ) -// issue #51488 +// go.dev/issue/51488 type ( _[a *t|t,] struct{} _[a *t|t, b t] struct{} @@ -52,3 +52,14 @@ type ( _[a ([]t)] struct{} _[a ([]t)|t] struct{} ) + +// go.dev/issue/60812 +type ( + _ [t]struct{} + _ [[]t]struct{} + _ [[t]t]struct{} + _ [t /* ERROR "missing type parameter name or invalid array length" */ [t]]struct{} + _ [t t[t], t /* ERROR "missing type parameter name" */ [t]]struct{} + _ [t /* ERROR "missing type parameter name" */ [t], t t[t]]struct{} + _ [t /* ERROR "missing type parameter name" */ [t], t[t]]struct{} // report only first error +) diff --git a/src/go/parser/testdata/typeset.go2 b/src/go/parser/testdata/typeset.go2 index 7844c22212..3d90d76639 100644 --- a/src/go/parser/testdata/typeset.go2 +++ b/src/go/parser/testdata/typeset.go2 @@ -61,12 +61,12 @@ type ( _[~t|~t] t ) -type _[_ t, t /* ERROR "type parameters must be named" */ ] t -type _[_ ~t, t /* ERROR "type parameters must be named" */ ] t -type _[_ t, ~ /* ERROR "type parameters must be named" */ t] t -type _[_ ~t, ~ /* ERROR "type parameters must be named" */ t] t +type _[_ t, t] /* ERROR "missing type constraint" */ t +type _[_ ~t, t] /* ERROR "missing type constraint" */ t +type _[_ t, ~ /* ERROR "missing type parameter name" */ t] t +type _[_ ~t, ~ /* ERROR "missing type parameter name" */ t] t -type _[_ t|t, t /* ERROR "type parameters must be named" */ |t] t -type _[_ ~t|t, t /* ERROR "type parameters must be named" */ |t] t -type _[_ t|t, ~ /* ERROR "type parameters must be named" */ t|t] t -type _[_ ~t|t, ~ /* ERROR "type parameters must be named" */ t|t] t +type _[_ t|t, t /* ERROR "missing type parameter name" */ |t] t +type _[_ ~t|t, t /* ERROR "missing type parameter name" */ |t] t +type _[_ t|t, ~ /* ERROR "missing type parameter name" */ t|t] t +type _[_ ~t|t, ~ /* ERROR "missing type parameter name" */ t|t] t