fix: checks for assignment sources (#429)

* Add test cases

This adds a small number of test cases which illustrate the problem.

* Fix invalid source columns

This prevents source columns from being bound to definitions which are
not appropriate.  For example, it prevents the source column of an
interleaving from being bound to a constant definition.

pkg/corset/parser.go

@@ -293,7 +293,7 @@ func (p *Parser) parseDefAlias(functions bool, elements []sexp.SExp) (Declaratio
 			errors = append(errors, *p.translator.SyntaxError(elements[i+1], "invalid alias definition"))
 		} else {
 			alias := &DefAlias{elements[i].AsSymbol().Value}
-			name := &Name{elements[i+1].AsSymbol().Value, functions, nil}
+			name := NewName[Binding](elements[i+1].AsSymbol().Value, functions)
 			p.mapSourceNode(elements[i], alias)
 			p.mapSourceNode(elements[i+1], name)
 			//
@@ -454,7 +454,7 @@ func (p *Parser) parseDefInterleaved(module string, elements []sexp.SExp) (Decla
 			return nil, p.translator.SyntaxError(ith, "malformed source column")
 		}
 		// Extract column name
-		sources[i] = &Name{ith.AsSymbol().Value, false, nil}
+		sources[i] = NewColumnName(ith.AsSymbol().Value)
 		p.mapSourceNode(ith, sources[i])
 	}
 	//
@@ -538,10 +538,10 @@ func (p *Parser) parseDefPermutation(module string, elements []sexp.SExp) (Decla
 	return &DefPermutation{targets, sources, signs}, nil
 }
 
-func (p *Parser) parsePermutedColumnDeclaration(signRequired bool, e sexp.SExp) (*Name, bool, *SyntaxError) {
+func (p *Parser) parsePermutedColumnDeclaration(signRequired bool, e sexp.SExp) (*ColumnName, bool, *SyntaxError) {
 	var (
 		err  *SyntaxError
-		name *Name
+		name *ColumnName
 		sign bool
 	)
 	// Check whether extended declaration or not.
@@ -559,11 +559,11 @@ func (p *Parser) parsePermutedColumnDeclaration(signRequired bool, e sexp.SExp)
 			return nil, false, err
 		}
 		// Parse column name
-		name = &Name{l.Get(1).AsSymbol().Value, false, nil}
+		name = NewColumnName(l.Get(1).AsSymbol().Value)
 	} else if signRequired {
 		return nil, false, p.translator.SyntaxError(e, "missing sort direction")
 	} else {
-		name = &Name{e.String(false), false, nil}
+		name = NewColumnName(e.String(false))
 	}
 	// Update source mapping
 	p.mapSourceNode(e, name)

pkg/corset/resolver.go

@@ -238,7 +238,7 @@ func (r *resolver) declarationDependenciesAreFinalised(scope *ModuleScope,
 			errors = append(errors, *r.srcmap.SyntaxError(symbol, "unknown symbol"))
 			// not finalised yet
 			finalised = false
-		} else if !symbol.Binding().IsFinalised() {
+		} else if symbol.IsResolved() && !symbol.Binding().IsFinalised() {
 			// no, not finalised
 			finalised = false
 		}

pkg/corset/scope.go

@@ -133,9 +133,7 @@ func (p *ModuleScope) Bind(symbol Symbol) bool {
 		// Extract binding
 		binding := p.bindings[bid]
 		// Resolve symbol
-		symbol.Resolve(binding)
-		// Success
-		return true
+		return symbol.Resolve(binding)
 	}
 	// failed
 	return false
@@ -286,9 +284,7 @@ func (p LocalScope) Bind(symbol Symbol) bool {
 	// Check whether this is a local variable access.
 	if id, ok := p.locals[symbol.Name()]; ok && !symbol.IsFunction() && !symbol.IsQualified() {
 		// Yes, this is a local variable access.
-		symbol.Resolve(&ParameterBinding{id})
-		// Success
-		return true
+		return symbol.Resolve(&ParameterBinding{id})
 	}
 	// No, this is not a local variable access.
 	return p.enclosing.Bind(symbol)

pkg/corset/symbol.go

@@ -0,0 +1,135 @@
+package corset
+
+import "github.com/consensys/go-corset/pkg/sexp"
+
+// Symbol represents a variable or function access within a declaration.
+// Initially, such the proper interpretation of such accesses is unclear and it
+// is only later when we can distinguish them (e.g. whether its a column access,
+// a constant access, etc).
+type Symbol interface {
+	Node
+	// Determines whether this symbol is qualfied or not (i.e. has an explicitly
+	// module specifier).
+	IsQualified() bool
+	// Indicates whether or not this is a function.
+	IsFunction() bool
+	// Checks whether this symbol has been resolved already, or not.
+	IsResolved() bool
+	// Optional module qualification
+	Module() string
+	// Name of the symbol
+	Name() string
+	// Get binding associated with this interface.  This will panic if this
+	// symbol is not yet resolved.
+	Binding() Binding
+	// Resolve this symbol by associating it with the binding associated with
+	// the definition of the symbol to which this refers.  Observe that
+	// resolution can fail if we cannot bind the symbol to the given binding
+	// (e.g. a function binding was provided, but we're expecting a column
+	// binding).
+	Resolve(Binding) bool
+}
+
+// SymbolDefinition represents a declaration (or part thereof) which defines a
+// particular symbol.  For example, "defcolumns" will define one or more symbols
+// representing columns, etc.
+type SymbolDefinition interface {
+	Node
+	// Name of symbol being defined
+	Name() string
+	// Indicates whether or not this is a function definition.
+	IsFunction() bool
+	// Allocated binding for the symbol which may or may not be finalised.
+	Binding() Binding
+}
+
+// ColumnName represents a name used in a position where it can only be resolved
+// against a column.
+type ColumnName = Name[*ColumnBinding]
+
+// NewColumnName construct a new column name which is (initially) unresolved.
+func NewColumnName(name string) *ColumnName {
+	return &ColumnName{name, false, nil, false}
+}
+
+// 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.
+type Name[T Binding] struct {
+	// Name of symbol
+	name string
+	// Indicates whether represents function or something else.
+	function bool
+	// Binding constructed for symbol.
+	binding T
+	// Indicates whether resolved.
+	resolved bool
+}
+
+// NewName construct a new name which is (initially) unresolved.
+func NewName[T Binding](name string, function bool) *Name[T] {
+	// Default value for type T
+	var empty T
+	// Construct the name
+	return &Name[T]{name, function, empty, false}
+}
+
+// IsQualified determines whether this symbol is qualfied or not (i.e. has an
+// explicit module specifier).  Column names are never qualified.
+func (e *Name[T]) IsQualified() bool {
+	return false
+}
+
+// IsFunction indicates whether or not this symbol refers to a function (which
+// of course it never does).
+func (e *Name[T]) IsFunction() bool {
+	return e.function
+}
+
+// IsResolved checks whether this symbol has been resolved already, or not.
+func (e *Name[T]) IsResolved() bool {
+	return e.resolved
+}
+
+// Module returns the optional module qualification.  This always panics because
+// column name's are never qualified.
+func (e *Name[T]) Module() string {
+	panic("undefined")
+}
+
+// Name returns the (unqualified) name of the column to which this symbol
+// refers.
+func (e *Name[T]) Name() string {
+	return e.name
+}
+
+// Binding gets binding associated with this interface.  This will panic if this
+// symbol is not yet resolved.
+func (e *Name[T]) Binding() Binding {
+	if !e.resolved {
+		panic("name not yet resolved")
+	}
+	//
+	return e.binding
+}
+
+// Resolve this symbol by associating it with the binding associated with
+// the definition of the symbol to which this refers.
+func (e *Name[T]) Resolve(binding Binding) bool {
+	var ok bool
+	//
+	if e.resolved {
+		panic("name already resolved")
+	}
+	// Attempt to assign binding.
+	e.binding, ok = binding.(T)
+	e.resolved = ok
+	//
+	return ok
+}
+
+// Lisp converts this node into its lisp representation.  This is primarily used
+// for debugging purposes.
+func (e *Name[T]) Lisp() sexp.SExp {
+	return sexp.NewSymbol(e.name)
+}

pkg/test/invalid_corset_test.go

@@ -292,6 +292,10 @@ func Test_Invalid_Permute_07(t *testing.T) {
 	CheckInvalid(t, "permute_invalid_07")
 }
 
+func Test_Invalid_Permute_08(t *testing.T) {
+	CheckInvalid(t, "permute_invalid_08")
+}
+
 // ===================================================================
 // Lookups
 // ===================================================================
@@ -367,6 +371,18 @@ func Test_Invalid_Interleave_09(t *testing.T) {
 	CheckInvalid(t, "interleave_invalid_09")
 }
 
+func Test_Invalid_Interleave_10(t *testing.T) {
+	CheckInvalid(t, "interleave_invalid_10")
+}
+
+func Test_Invalid_Interleave_11(t *testing.T) {
+	CheckInvalid(t, "interleave_invalid_11")
+}
+
+func Test_Invalid_Interleave_12(t *testing.T) {
+	CheckInvalid(t, "interleave_invalid_12")
+}
+
 // ===================================================================
 // Functions
 // ===================================================================

testdata/interleave_invalid_11.lisp

@@ -0,0 +1,3 @@
+(defcolumns X)
+(defconst Y 1)
+(definterleaved Z (X Y))

testdata/interleave_invalid_12.lisp

@@ -0,0 +1,3 @@
+(defcolumns Y)
+(defconst X 1)
+(definterleaved Z (X Y))

testdata/permute_invalid_08.lisp

@@ -0,0 +1,2 @@
+(defconst X 100)
+(defpermutation (Y) ((+ X)))