From 3aed5d4ed67bd7f57778d4e96f7d2a6763eb22f8 Mon Sep 17 00:00:00 2001 From: David Pearce Date: Sun, 22 Dec 2024 13:26:26 +1300 Subject: [PATCH] feat: improve error reporting around functions (#494) Improve error reporting around functions This improves the error reporting around functions, such as arising from defun and defpurefun. Specifically, it highlights only the function name when there is a general error related to the function (e.g. its redeclared, etc). --- pkg/corset/declaration.go | 24 +++++++++++------------- pkg/corset/parser.go | 21 ++++++++++++--------- pkg/corset/resolver.go | 2 +- pkg/corset/symbol.go | 9 +++++++++ testdata/purefun_invalid_10.lisp | 2 +- testdata/purefun_invalid_11.lisp | 2 +- testdata/purefun_invalid_12.lisp | 2 +- 7 files changed, 36 insertions(+), 26 deletions(-) diff --git a/pkg/corset/declaration.go b/pkg/corset/declaration.go index 5285df6..33b0a38 100644 --- a/pkg/corset/declaration.go +++ b/pkg/corset/declaration.go @@ -815,11 +815,9 @@ func (p *DefProperty) Lisp() sexp.SExp { // parameters). In contrast, an impure function can access those columns // defined within its enclosing context. type DefFun struct { - name string + symbol *FunctionName // Parameters parameters []*DefParameter - // - binding DefunBinding } // IsFunction is always true for a function definition! @@ -831,7 +829,7 @@ func (p *DefFun) IsFunction() bool { // which is not permitted to access any columns from the enclosing environment // (either directly itself, or indirectly via functions it calls). func (p *DefFun) IsPure() bool { - return p.binding.pure + return p.symbol.binding.pure } // Parameters returns information about the parameters defined by this @@ -842,30 +840,30 @@ func (p *DefFun) Parameters() []*DefParameter { // Body Access information about the parameters defined by this declaration. func (p *DefFun) Body() Expr { - return p.binding.body + return p.symbol.binding.body } // Binding returns the allocated binding for this symbol (which may or may not // be finalised). func (p *DefFun) Binding() Binding { - return &p.binding + return p.symbol.binding } // Name of symbol being defined func (p *DefFun) Name() string { - return p.name + return p.symbol.name } // Definitions returns the set of symbols defined by this declaration. Observe // that these may not yet have been finalised. func (p *DefFun) Definitions() util.Iterator[SymbolDefinition] { - iter := util.NewUnitIterator(p) - return util.NewCastIterator[*DefFun, SymbolDefinition](iter) + iter := util.NewUnitIterator(p.symbol) + return util.NewCastIterator[*FunctionName, SymbolDefinition](iter) } // Dependencies needed to signal declaration. func (p *DefFun) Dependencies() util.Iterator[Symbol] { - deps := p.binding.body.Dependencies() + deps := p.symbol.binding.body.Dependencies() ndeps := make([]Symbol, 0) // Filter out all parameters declared in this function, since these are not // external dependencies. @@ -881,13 +879,13 @@ func (p *DefFun) Dependencies() util.Iterator[Symbol] { // Defines checks whether this declaration defines the given symbol. The symbol // in question needs to have been resolved already for this to make sense. func (p *DefFun) Defines(symbol Symbol) bool { - return &p.binding == symbol.Binding() + return p.symbol.binding == symbol.Binding() } // IsFinalised checks whether this declaration has already been finalised. If // so, then we don't need to finalise it again. func (p *DefFun) IsFinalised() bool { - return p.binding.IsFinalised() + return p.symbol.binding.IsFinalised() } // Lisp converts this node into its lisp representation. This is primarily used @@ -895,7 +893,7 @@ func (p *DefFun) IsFinalised() bool { func (p *DefFun) Lisp() sexp.SExp { return sexp.NewList([]sexp.SExp{ sexp.NewSymbol("defun"), - sexp.NewSymbol(p.name), + sexp.NewSymbol(p.symbol.name), sexp.NewSymbol("..."), // todo }) } diff --git a/pkg/corset/parser.go b/pkg/corset/parser.go index 4c2a103..256bd75 100644 --- a/pkg/corset/parser.go +++ b/pkg/corset/parser.go @@ -714,7 +714,7 @@ func (p *Parser) parseDefProperty(elements []sexp.SExp) (Declaration, []SyntaxEr // Parse a permutation declaration func (p *Parser) parseDefFun(pure bool, elements []sexp.SExp) (Declaration, []SyntaxError) { var ( - name string + name *sexp.Symbol ret Type params []*DefParameter errors []SyntaxError @@ -741,11 +741,14 @@ func (p *Parser) parseDefFun(pure bool, elements []sexp.SExp) (Declaration, []Sy } // Construct binding binding := NewDefunBinding(pure, paramTypes, ret, body) + fn_name := NewFunctionName(name.Value, &binding) + // Update source mapping + p.mapSourceNode(name, fn_name) // - return &DefFun{name, params, binding}, nil + return &DefFun{fn_name, params}, nil } -func (p *Parser) parseFunctionSignature(elements []sexp.SExp) (string, Type, []*DefParameter, []SyntaxError) { +func (p *Parser) parseFunctionSignature(elements []sexp.SExp) (*sexp.Symbol, Type, []*DefParameter, []SyntaxError) { var ( params []*DefParameter = make([]*DefParameter, len(elements)-1) ) @@ -761,13 +764,13 @@ func (p *Parser) parseFunctionSignature(elements []sexp.SExp) (string, Type, []* } // Check for any errors arising if len(errors) > 0 { - return "", nil, nil, errors + return nil, nil, nil, errors } // return name, ret, params, nil } -func (p *Parser) parseFunctionNameReturn(element sexp.SExp) (string, Type, bool, []SyntaxError) { +func (p *Parser) parseFunctionNameReturn(element sexp.SExp) (*sexp.Symbol, Type, bool, []SyntaxError) { var ( err *SyntaxError name sexp.SExp @@ -786,7 +789,7 @@ func (p *Parser) parseFunctionNameReturn(element sexp.SExp) (string, Type, bool, // Check what we have if symbol == nil { err := p.translator.SyntaxError(element, "modifier expected") - return "", nil, false, []SyntaxError{*err} + return nil, nil, false, []SyntaxError{*err} } else if i == 0 { name = symbol } else { @@ -795,7 +798,7 @@ func (p *Parser) parseFunctionNameReturn(element sexp.SExp) (string, Type, bool, forced = true default: if ret, _, err = p.parseType(element); err != nil { - return "", nil, false, []SyntaxError{*err} + return nil, nil, false, []SyntaxError{*err} } } } @@ -803,11 +806,11 @@ func (p *Parser) parseFunctionNameReturn(element sexp.SExp) (string, Type, bool, } // if isFunIdentifier(name) { - return name.AsSymbol().Value, ret, forced, nil + return name.AsSymbol(), ret, forced, nil } else { // Must be non-identifier symbol err = p.translator.SyntaxError(element, "invalid function name") - return "", nil, false, []SyntaxError{*err} + return nil, nil, false, []SyntaxError{*err} } } diff --git a/pkg/corset/resolver.go b/pkg/corset/resolver.go index 56de1d6..287cf0e 100644 --- a/pkg/corset/resolver.go +++ b/pkg/corset/resolver.go @@ -455,7 +455,7 @@ func (r *resolver) finaliseDefFunInModule(enclosing Scope, decl *DefFun) []Synta datatype, errors := r.finaliseExpressionInModule(scope, decl.Body()) // Finalise declaration if len(errors) == 0 { - decl.binding.Finalise(datatype) + decl.symbol.binding.Finalise(datatype) } // Done return errors diff --git a/pkg/corset/symbol.go b/pkg/corset/symbol.go index d1109b7..e81e523 100644 --- a/pkg/corset/symbol.go +++ b/pkg/corset/symbol.go @@ -65,6 +65,15 @@ func NewColumnName(name string) *ColumnName { return &ColumnName{name, false, nil, false} } +// FunctionName represents a name used in a position where it can only be +// resolved as a function. +type FunctionName = Name[*DefunBinding] + +// NewFunctionName construct a new column name which is (initially) unresolved. +func NewFunctionName(name string, binding *DefunBinding) *FunctionName { + return &FunctionName{name, true, binding, true} +} + // Name represents a name within some syntactic item. Essentially this wraps a // string and provides a mechanism for it to be associated with source line // information. diff --git a/testdata/purefun_invalid_10.lisp b/testdata/purefun_invalid_10.lisp index 3ac76c9..da346be 100644 --- a/testdata/purefun_invalid_10.lisp +++ b/testdata/purefun_invalid_10.lisp @@ -1,3 +1,3 @@ -;;error:3:1-29:symbol * already declared +;;error:3:14-15:symbol * already declared ;; Attempt to overload intrinsic. (defpurefun (* x y) (* x y)) diff --git a/testdata/purefun_invalid_11.lisp b/testdata/purefun_invalid_11.lisp index a98f5de..86c5650 100644 --- a/testdata/purefun_invalid_11.lisp +++ b/testdata/purefun_invalid_11.lisp @@ -1,4 +1,4 @@ -;;error:4:1-50:symbol eq already declared +;;error:4:14-16:symbol eq already declared ;; Duplicate overload is always a syntax error. (defpurefun (eq (x :binary) (y :binary)) (- x y)) (defpurefun (eq (x :binary) (y :binary)) (+ x y)) diff --git a/testdata/purefun_invalid_12.lisp b/testdata/purefun_invalid_12.lisp index 088c8df..a55fbe2 100644 --- a/testdata/purefun_invalid_12.lisp +++ b/testdata/purefun_invalid_12.lisp @@ -1,4 +1,4 @@ -;;error:4:1-25:symbol eq already declared +;;error:4:9-11:symbol eq already declared ;; Cannot overload pure with impure, and vice versa. (defpurefun (eq (x :binary) (y :binary)) (- x y)) (defun (eq x y) (+ x y))