From 1ed725d7cd4dc5073a6c8de08eda995da63476d4 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Mon, 18 May 2009 14:59:16 -0700 Subject: [PATCH] - changed parser to return os.Error, removed ErrorHandler - added IsValid predicate to token.Position - updated pretty, godoc, gobuild - updated/expanded test cases R=rsc DELTA=265 (97 added, 78 deleted, 90 changed) OCL=28961 CL=29005 --- src/cmd/gobuild/util.go | 11 +-- src/lib/go/parser/parser.go | 151 +++++++++++++++++++------------ src/lib/go/parser/parser_test.go | 67 +++++++++----- src/lib/go/token/token.go | 8 ++ usr/gri/pretty/godoc.go | 63 ++++--------- usr/gri/pretty/pretty.go | 45 +++------ 6 files changed, 182 insertions(+), 163 deletions(-) diff --git a/src/cmd/gobuild/util.go b/src/cmd/gobuild/util.go index 43d7aed798..462d2dc953 100644 --- a/src/cmd/gobuild/util.go +++ b/src/cmd/gobuild/util.go @@ -179,9 +179,6 @@ func (s MakeString) String() string { return dollarString(string(s), "(", ")"); } -// TODO(rsc): parse.Parse should return an os.Error. -var ParseError = os.NewError("parse errors"); - // TODO(rsc): Should this be in the AST library? func LitString(p []*ast.StringLit) (string, os.Error) { s := ""; @@ -201,9 +198,9 @@ func PackageImports(file string) (pkg string, imports []string, err1 os.Error) { return "", nil, err } - prog, ok := parser.Parse(f, nil, parser.ImportsOnly); - if !ok { - return "", nil, ParseError; + prog, err := parser.Parse(f, parser.ImportsOnly); + if err != nil { + return "", nil, err; } // Normally one must consult the types of decl and spec, @@ -214,7 +211,7 @@ func PackageImports(file string) (pkg string, imports []string, err1 os.Error) { for _, spec := range decl.(*ast.GenDecl).Specs { str, err := LitString(spec.(*ast.ImportSpec).Path); if err != nil { - return "", nil, ParseError; // ParseError is better than os.EINVAL + return "", nil, os.NewError("invalid import specifier"); // better than os.EINVAL } PushString(&imp, str); } diff --git a/src/lib/go/parser/parser.go b/src/lib/go/parser/parser.go index c766526af7..8663289f8b 100644 --- a/src/lib/go/parser/parser.go +++ b/src/lib/go/parser/parser.go @@ -16,16 +16,41 @@ import ( "go/scanner"; "go/token"; "io"; + "os"; ) -// An implementation of an ErrorHandler may be provided to the parser. -// If a syntax error is encountered and a handler was installed, Error -// is called with a position and an error message. The position points -// to the beginning of the offending token. +// A parser error is represented by an Error node. The position Pos, if +// valid, points to the beginning of the offending token, and the error +// condition is described by Msg. // -type ErrorHandler interface { - Error(pos token.Position, msg string); +type Error struct { + Pos token.Position; + Msg string; +} + + +func (e *Error) String() string { + pos := ""; + if e.Pos.IsValid() { + pos = fmt.Sprintf("%d:%d: ", e.Pos.Line, e.Pos.Column); + } + return pos + e.Msg; +} + + +// Parser errors are returned as an ErrorList. +type ErrorList []*Error + + +// ErrorList implements the SortInterface. +func (p ErrorList) Len() int { return len(p); } +func (p ErrorList) Swap(i, j int) { p[i], p[j] = p[j], p[i]; } +func (p ErrorList) Less(i, j int) bool { return p[i].Pos.Offset < p[j].Pos.Offset; } + + +func (p ErrorList) String() string { + return fmt.Sprintf("%d syntax errors", len(p)); } @@ -36,9 +61,8 @@ type interval struct { // The parser structure holds the parser's internal state. type parser struct { + errors vector.Vector; scanner scanner.Scanner; - err ErrorHandler; // nil if no handler installed - hasErrors bool; // Tracing/debugging mode uint; // parsing mode @@ -185,11 +209,14 @@ func (p *parser) next() { } -func (p *parser) error(pos token.Position, msg string) { - if p.err != nil { - p.err.Error(pos, msg); +// The parser implements scanner.Error. +func (p *parser) Error(pos token.Position, msg string) { + // Don't collect errors that are on the same line as the previous error + // in the hope to reduce the number of spurious errors due to incorrect + // parser synchronization. + if p.errors.Len() == 0 || p.errors.Last().(*Error).Pos.Line != pos.Line { + p.errors.Push(&Error{pos, msg}); } - p.hasErrors = true; } @@ -203,7 +230,7 @@ func (p *parser) error_expected(pos token.Position, msg string) { msg += " " + string(p.lit); } } - p.error(pos, msg); + p.Error(pos, msg); } @@ -470,7 +497,7 @@ func (p *parser) tryParameterType(ellipsis_ok bool) ast.Expr { p.next(); if p.tok != token.RPAREN { // "..." always must be at the very end of a parameter list - p.error(pos, "expected type, found '...'"); + p.Error(pos, "expected type, found '...'"); } return &ast.Ellipsis{pos}; } @@ -1115,7 +1142,7 @@ func (p *parser) checkExprOrType(x ast.Expr) ast.Expr { } case *ast.ArrayType: if len, is_ellipsis := t.Len.(*ast.Ellipsis); is_ellipsis { - p.error(len.Pos(), "expected array length, found '...'"); + p.Error(len.Pos(), "expected array length, found '...'"); x = &ast.BadExpr{x.Pos()}; } } @@ -1223,7 +1250,7 @@ func (p *parser) parseSimpleStmt(label_ok bool) ast.Stmt { return &ast.LabeledStmt{label, p.parseStatement()}; } } - p.error(x[0].Pos(), "illegal label declaration"); + p.Error(x[0].Pos(), "illegal label declaration"); return &ast.BadStmt{x[0].Pos()}; case @@ -1236,13 +1263,13 @@ func (p *parser) parseSimpleStmt(label_ok bool) ast.Stmt { p.next(); y := p.parseExpressionList(); if len(x) > 1 && len(y) > 1 && len(x) != len(y) { - p.error(x[0].Pos(), "arity of lhs doesn't match rhs"); + p.Error(x[0].Pos(), "arity of lhs doesn't match rhs"); } return &ast.AssignStmt{x, pos, tok, y}; } if len(x) > 1 { - p.error(x[0].Pos(), "only one expression allowed"); + p.Error(x[0].Pos(), "only one expression allowed"); // continue with first expression } @@ -1343,7 +1370,7 @@ func (p *parser) makeExpr(s ast.Stmt) ast.Expr { if es, is_expr := s.(*ast.ExprStmt); is_expr { return p.checkExpr(es.X); } - p.error(s.Pos(), "expected condition, found simple statement"); + p.Error(s.Pos(), "expected condition, found simple statement"); return &ast.BadExpr{s.Pos()}; } @@ -1846,7 +1873,7 @@ func (p *parser) parsePackage() *ast.Program { // Don't bother parsing the rest if we had errors already. // Likely not a Go source file at all. - if !p.hasErrors && p.mode & PackageClauseOnly == 0 { + if p.errors.Len() == 0 && p.mode & PackageClauseOnly == 0 { // import decls list := vector.New(0); for p.tok == token.IMPORT { @@ -1886,32 +1913,28 @@ func (p *parser) parsePackage() *ast.Program { // ---------------------------------------------------------------------------- // Parsing of entire programs. -func readSource(src interface{}, err ErrorHandler) []byte { - errmsg := "invalid input type (or nil)"; - - switch s := src.(type) { - case string: - return io.StringBytes(s); - case []byte: - return s; - case *io.ByteBuffer: - // is io.Read, but src is already available in []byte form - if s != nil { - return s.Data(); - } - case io.Reader: - var buf io.ByteBuffer; - n, os_err := io.Copy(s, &buf); - if os_err == nil { - return buf.Data(); +func readSource(src interface{}) ([]byte, os.Error) { + if src != nil { + switch s := src.(type) { + case string: + return io.StringBytes(s), nil; + case []byte: + return s, nil; + case *io.ByteBuffer: + // is io.Read, but src is already available in []byte form + if s != nil { + return s.Data(), nil; + } + case io.Reader: + var buf io.ByteBuffer; + n, err := io.Copy(s, &buf); + if err != nil { + return nil, err; + } + return buf.Data(), nil; } - errmsg = os_err.String(); } - - if err != nil { - err.Error(noPos, errmsg); - } - return nil; + return nil, os.ErrorString("invalid source"); } @@ -1919,25 +1942,26 @@ func readSource(src interface{}, err ErrorHandler) []byte { // // The program source src may be provided in a variety of formats. At the // moment the following types are supported: string, []byte, and io.Read. +// The mode parameter controls the amount of source text parsed and other +// optional parser functionality. // -// The ErrorHandler err, if not nil, is invoked if src cannot be read and -// for each syntax error found. The mode parameter controls the amount of -// source text parsed and other optional parser functionality. -// -// Parse returns an AST and the boolean value true if no errors occured; -// it returns a partial AST (or nil if the source couldn't be read) and -// the boolean value false to indicate failure. +// Parse returns a complete AST if no error occured. Otherwise, if the +// source couldn't be read, the returned program is nil and the error +// indicates the specific failure. If the source was read but syntax +// errors were found, the result is a partial AST (with ast.BadX nodes +// representing the fragments of erroneous source code) and an ErrorList +// describing the syntax errors. // -// If syntax errors were found, the AST may only be constructed partially, -// with ast.BadX nodes representing the fragments of erroneous source code. -// -func Parse(src interface{}, err ErrorHandler, mode uint) (*ast.Program, bool) { - data := readSource(src, err); +func Parse(src interface{}, mode uint) (*ast.Program, os.Error) { + data, err := readSource(src); + if err != nil { + return nil, err; + } // initialize parser state var p parser; - p.scanner.Init(data, err, mode & ParseComments != 0); - p.err = err; + p.errors.Init(0); + p.scanner.Init(data, &p, mode & ParseComments != 0); p.mode = mode; p.trace = mode & Trace != 0; // for convenience (p.trace is used frequently) p.comments.Init(0); @@ -1946,5 +1970,14 @@ func Parse(src interface{}, err ErrorHandler, mode uint) (*ast.Program, bool) { // parse program prog := p.parsePackage(); - return prog, p.scanner.ErrorCount == 0 && !p.hasErrors; + // convert errors list, if any + if p.errors.Len() > 0 { + errors := make(ErrorList, p.errors.Len()); + for i := 0; i < p.errors.Len(); i++ { + errors[i] = p.errors.At(i).(*Error); + } + return prog, errors; + } + + return prog, nil; } diff --git a/src/lib/go/parser/parser_test.go b/src/lib/go/parser/parser_test.go index e35d18799c..887fcf80fa 100644 --- a/src/lib/go/parser/parser_test.go +++ b/src/lib/go/parser/parser_test.go @@ -12,36 +12,57 @@ import ( ) -func TestParse0(t *testing.T) { - // test nil []bytes source - var src []byte; - prog, ok := Parse(src, nil, 0); - if ok { - t.Errorf("parse should have failed"); - } +var illegalInputs = []interface{} { + nil, + 3.14, + []byte(nil), + "foo!", } -func TestParse1(t *testing.T) { - // test string source - src := `package main import "fmt" func main() { fmt.Println("Hello, World!") }`; - prog, ok := Parse(src, nil, 0); - if !ok { - t.Errorf("parse failed"); +func TestParseIllegalInputs(t *testing.T) { + for _, src := range illegalInputs { + prog, err := Parse(src, 0); + if err == nil { + t.Errorf("Parse(%v) should have failed", src); + } } } -func TestParse2(t *testing.T) { - // test io.Read source - filename := "parser_test.go"; - src, err := os.Open(filename, os.O_RDONLY, 0); - defer src.Close(); - if err != nil { - t.Errorf("cannot open %s (%s)\n", filename, err.String()); + +var validPrograms = []interface{} { + `package main`, + `package main import "fmt" func main() { fmt.Println("Hello, World!") }`, +} + + +func TestParseValidPrograms(t *testing.T) { + for _, src := range validPrograms { + prog, err := Parse(src, 0); + if err != nil { + t.Errorf("Parse(%q) failed: %v", src, err); + } } +} + + +var validFiles = []string { + "parser.go", + "parser_test.go", +} + + +func TestParse3(t *testing.T) { + for _, filename := range validFiles { + src, err := os.Open(filename, os.O_RDONLY, 0); + defer src.Close(); + if err != nil { + t.Fatalf("os.Open(%s): %v\n", filename, err); + } - prog, ok := Parse(src, nil, 0); - if !ok { - t.Errorf("parse failed"); + prog, err := Parse(src, 0); + if err != nil { + t.Errorf("Parse(%q): %v", src, err); + } } } diff --git a/src/lib/go/token/token.go b/src/lib/go/token/token.go index b71d0f03d3..a70a75a540 100644 --- a/src/lib/go/token/token.go +++ b/src/lib/go/token/token.go @@ -324,6 +324,8 @@ func (tok Token) IsKeyword() bool { // Token source positions are represented by a Position value. +// A Position is valid if the line number is > 0. +// type Position struct { Offset int; // byte offset, starting at 0 Line int; // line number, starting at 1 @@ -337,3 +339,9 @@ type Position struct { func (pos *Position) Pos() Position { return *pos; } + + +// IsValid returns true if the position is valid. +func (pos *Position) IsValid() bool { + return pos.Line > 0 +} diff --git a/usr/gri/pretty/godoc.go b/usr/gri/pretty/godoc.go index 1848b58a22..dcf3aa1028 100644 --- a/usr/gri/pretty/godoc.go +++ b/usr/gri/pretty/godoc.go @@ -123,34 +123,6 @@ func ReadFile(name string) ([]byte, os.Error) { // ---------------------------------------------------------------------------- // Parsing -type rawError struct { - pos token.Position; - msg string; -} - - -type rawErrorVector struct { - vector.Vector; -} - - -func (v *rawErrorVector) At(i int) rawError { return v.Vector.At(i).(rawError) } -func (v *rawErrorVector) Less(i, j int) bool { return v.At(i).pos.Offset < v.At(j).pos.Offset; } - - -func (v *rawErrorVector) Error(pos token.Position, msg string) { - // only collect errors that are on a new line - // in the hope to avoid most follow-up errors - lastLine := 0; - if n := v.Len(); n > 0 { - lastLine = v.At(n - 1).pos.Line; - } - if lastLine != pos.Line { - v.Push(rawError{pos, msg}); - } -} - - // A single error in the parsed file. type parseError struct { src []byte; // source before error @@ -183,25 +155,28 @@ func parse(path string, mode uint) (*ast.Program, *parseErrors) { return nil, &parseErrors{path, errs, nil}; } - var raw rawErrorVector; - prog, ok := parser.Parse(src, &raw, mode); - if !ok { + prog, err := parser.Parse(src, mode); + if err != nil { // sort and convert error list - sort.Sort(&raw); - errs := make([]parseError, raw.Len() + 1); // +1 for final fragment of source - offs := 0; - for i := 0; i < raw.Len(); i++ { - r := raw.At(i); - // Should always be true, but check for robustness. - if 0 <= r.pos.Offset && r.pos.Offset <= len(src) { - errs[i].src = src[offs : r.pos.Offset]; - offs = r.pos.Offset; + if errors, ok := err.(parser.ErrorList); ok { + sort.Sort(errors); + errs := make([]parseError, len(errors) + 1); // +1 for final fragment of source + offs := 0; + for i, r := range errors { + // Should always be true, but check for robustness. + if 0 <= r.Pos.Offset && r.Pos.Offset <= len(src) { + errs[i].src = src[offs : r.Pos.Offset]; + offs = r.Pos.Offset; + } + errs[i].line = r.Pos.Line; + errs[i].msg = r.Msg; } - errs[i].line = r.pos.Line; - errs[i].msg = r.msg; + errs[len(errors)].src = src[offs : len(src)]; + return nil, &parseErrors{path, errs, src}; + } else { + // TODO should have some default handling here to be more robust + panic("unreachable"); } - errs[raw.Len()].src = src[offs : len(src)]; - return nil, &parseErrors{path, errs, src}; } return prog, nil; diff --git a/usr/gri/pretty/pretty.go b/usr/gri/pretty/pretty.go index 90cc96dbbc..56b36e45a3 100644 --- a/usr/gri/pretty/pretty.go +++ b/usr/gri/pretty/pretty.go @@ -5,17 +5,17 @@ package main import ( + "astprinter"; "flag"; "fmt"; + "format"; "go/ast"; "go/parser"; "go/token"; "io"; "os"; + "sort"; "tabwriter"; - - "astprinter"; - "format"; ) @@ -70,32 +70,9 @@ func makeTabwriter(writer io.Writer) *tabwriter.Writer { } -// TODO(gri) move this into parser as default handler -type ErrorHandler struct { - filename string; - lastline int; -} - - -func (h *ErrorHandler) Error(pos token.Position, msg string) { - // only report errors that are on a new line - // in the hope to avoid most follow-up errors - if pos.Line == h.lastline { - return; - } - h.lastline = pos.Line; - - // report error - fmt.Fprintf(os.Stderr, "%s:%d:", h.filename, pos.Line); - if columns { - fmt.Fprintf(os.Stderr, "%d:", pos.Column); - } - fmt.Fprintf(os.Stderr, " %s\n", msg); -} - - func isValidPos(w io.Writer, env, value interface{}, name string) bool { - return value.(token.Position).Line > 0; + pos := value.(token.Position); + return pos.IsValid(); } @@ -159,8 +136,16 @@ func main() { continue; // proceed with next file } - prog, ok := parser.Parse(src, &ErrorHandler{filename, 0}, mode); - if !ok { + prog, err := parser.Parse(src, mode); + if err != nil { + if errors, ok := err.(parser.ErrorList); ok { + sort.Sort(errors); + for _, e := range errors { + fmt.Fprintf(os.Stderr, "%s:%v\n", filename, e); + } + } else { + fmt.Fprintf(os.Stderr, "%s: %v\n", filename, err); + } exitcode = 1; continue; // proceed with next file } -- 2.48.1