Merge branch 'subword-errors' of git://git.gitano.org.uk/personal/richardmaw/lace

8 files changed, 199 insertions, 90 deletions

diff --git a/lib/lace/builtin.lua b/lib/lace/builtin.lua
index b0c7ad1..2785389 100644
--- a/lib/lace/builtin.lua
+++ b/lib/lace/builtin.lua
@@ -35,12 +35,7 @@ local function run_conditions(exec_context, cond, anyof)
       end
       local res, msg = engine.test(exec_context, name)
       if res == nil then
-	 local subwords = msg.words
-	 if subwords and #subwords > 0 then
-	    msg.words = {{nr = i, sub = subwords}}
-	 else
-	    msg.words = {i}
-	 end
+	 msg.words = {err.subwords(msg, i)}
 	 return nil, msg
       end
       if invert then
@@ -102,11 +97,13 @@ local function get_set_last_result(newv)
    return ret
 end
 
-local function _do_return(exec_context, result, reason, cond)
+local function _do_return(exec_context, rule, result, reason, cond)
    local pass, msg = run_conditions(exec_context, cond)
    if pass == nil then
       -- Pass errors
-      err.offset(msg, 2)
+      msg = err.offset(msg, 2)
+      -- Record error source
+      msg = err.augment(msg, rule.source, rule.linenr)
       return nil, msg
    elseif pass == false then
       -- Conditions failed, return true to continue execution
@@ -144,10 +141,11 @@ local function _return(compcontext, result, reason, ...)
    end
    last_result = result
 
-   return {
+   local rule = {
       fn = _do_return,
-      args = { result, reason, cond }
    }
+   rule.args = { rule, result, reason, cond }
+   return rule
 end
 
 builtin.allow = _return
@@ -191,9 +189,14 @@ function builtin.default(compcontext, def, result, reason, unwanted)
    if compcontext._lace.default then
       return err.error("Cannot change the default", {1, 2})
    end
-   
+
    local uncond, last = unconditional_result, last_result
-   compcontext._lace.default = _return(compcontext, result, reason)
+   local default_rule = _return(compcontext, result, reason)
+   -- Normally lace.compiler.internal_compile augments the rules with sources,
+   -- but since this rule is not returned, we have to augment it ourselves.
+   default_rule.source = compcontext._lace.source
+   default_rule.linenr = compcontext._lace.linenr
+   compcontext._lace.default = default_rule
    unconditional_result, last_result = uncond, last
 
    return {
@@ -204,6 +207,18 @@ end
 
 --[ Control types ]--------------------------------------------------
 
+local function _do_any_all_of(exec_context, rule, cond, anyof)
+   local pass, msg = run_conditions(exec_context, cond, anyof)
+   if pass == nil then
+      -- Offset error location by anyof/allof word
+      msg = err.offset(msg, 1)
+      -- Record error source
+      msg = err.augment(msg, rule.source, rule.linenr)
+      return nil, msg
+   end
+   return pass, msg
+end
+
 local function _compile_any_all_of(compcontext, mtype, first, second, ...)
    if type(first) ~= "string" then
       return err.error("Expected at least two names, got none", {1})
@@ -212,18 +227,11 @@ local function _compile_any_all_of(compcontext, mtype, first, second, ...)
       return err.error("Expected at least two names, only got one", {1, 2})
    end
 
-   return {
-      fn = (function(exec_context, cond, anyof)
-         local pass, msg = run_conditions(exec_context, cond, anyof)
-         if pass == nil then
-            -- Offset error location by anyof/allof word
-            err.offset(msg, 1)
-            return nil, msg
-         end
-         return pass, msg
-      end),
-      args = { { first, second, ...}, mtype == "anyof" }
+   local rule =  {
+      fn = _do_any_all_of,
    }
+   rule.args = { rule, { first, second, ...}, mtype == "anyof" }
+   return rule
 end
 
 local builtin_control_fn = {
@@ -242,6 +250,30 @@ local function _controlfn(ctx, name)
    return cfn
 end
 
+local function wrap_call_definition_location(rule, defn)
+   local fn = defn.fn
+   function defn.fn(...)
+      local res, msg = fn(...)
+      if res == nil then
+         msg = err.offset(msg, 2)
+         msg = err.augment(msg, rule.source, rule.linenr)
+         return nil, msg
+      end
+      return res, msg
+   end
+   return defn
+end
+
+local function _do_define(exec_context, rule, name, defn)
+   defn = wrap_call_definition_location(rule, defn)
+   local res, msg = engine.define(exec_context, name, defn)
+   if res == nil then
+      msg = err.augment(msg, rule.source, rule.linenr)
+      return nil, msg
+   end
+   return res, msg
+end
+
 --- Compile a definition command
 --
 -- Definitions are a core behaviour of Lace.  This builtin allows the ruleset
@@ -272,10 +304,11 @@ function builtin.define(compcontext, define, name, controltype, ...)
    if type(controltype) ~= "string" then
       return err.error("Expected control type, got nothing", {1, 2})
    end
-   
+
    local controlfn = _controlfn(compcontext, controltype)
    if not controlfn then
-      return err.error("Unknown control type: " .. controltype, {3})
+      emsg = "%s's second parameter (%s) must be a control type such as anyof"
+      return err.error(emsg:format(define, controltype), {3})
    end
 
    local ctrltab, msg = controlfn(compcontext, controltype, ...)
@@ -286,20 +319,23 @@ function builtin.define(compcontext, define, name, controltype, ...)
    end
 
    -- Successfully created a control table, return a rule for it
-   return {
-      fn = engine.define,
-      args = { name, ctrltab }
+   local rule = {
+      fn = _do_define,
    }
+   rule.args = { rule, name, ctrltab }
+   return rule
 end
 
 builtin.def = builtin.define
 
 --[ Inclusion of rulesets ]-------------------------------------------
 
-local function _do_include(exec_context, ruleset, conds)
+local function _do_include(exec_context, rule, ruleset, conds)
    local pass, msg = run_conditions(exec_context, conds)
    if pass == nil then
-      -- Pass errors
+      -- Propagate errors
+      msg = err.offset(msg, 2)
+      msg = err.augment(msg, rule.source, rule.linenr)
       return nil, msg
    elseif pass == false then
       -- Conditions failed, return true to continue execution
@@ -309,6 +345,10 @@ local function _do_include(exec_context, ruleset, conds)
    local result, msg = engine.internal_run(ruleset, exec_context)
    if result == "" then
       return true
+   elseif result == nil then
+      msg.words = {err.subwords(msg, 2)}
+      msg = err.augment(msg, rule.source, rule.linenr)
+      return nil, msg
    end
    return result, msg
 end
@@ -367,10 +407,11 @@ function builtin.include(comp_context, cmd, file, ...)
    end
    
    -- Okay, we parsed, so build the runtime
-   return {
+   local rule = {
       fn = _do_include,
-      args = { ruleset, conds }
    }
+   rule.args = { rule, ruleset, conds }
+   return rule
 end
 
 return {
diff --git a/lib/lace/compiler.lua b/lib/lace/compiler.lua
index 7b3c504..1c3aa01 100644
--- a/lib/lace/compiler.lua
+++ b/lib/lace/compiler.lua
@@ -62,7 +62,7 @@ local function _setposition(context, ruleset, linenr)
 end
 
 local function transfer_args(compcontext, content, rules)
-   local args, err = {}
+   local args = {}
    for i = 1, #content do
       if content[i].sub then
 	 local sub = content[i].sub
@@ -75,13 +75,32 @@ local function transfer_args(compcontext, content, rules)
 	 if type(rules) ~= "table" then
 	    return rules, subargs
 	 end
-	 local definerule, err = definefn(compcontext, "define", definename,
+	 local definerule, msg = definefn(compcontext, "define", definename,
 					  unpack(subargs))
 	 if type(definerule) ~= "table" then
 	    -- for now, we lock the error to the whole sublex
-	    err.words = {i}
-	    return definerule, err
+	    msg.words = {i}
+	    return definerule, msg
 	 end
+
+         -- Fix up error location offset
+         -- The error words are offset by 2 because the "define" token and name,
+         -- but we're abusing the define command for something without those,
+         -- we are responsible for offsetting the errors back.
+         local bindname = definerule.fn
+         function definerule.fn(exec_context, rule, name, defn)
+            local fn = defn.fn
+            function defn.fn(...)
+               local res, msg = fn(...)
+               if res == nil then
+                  msg = err.offset(msg, -2)
+                  return nil, msg
+               end
+               return res, msg
+            end
+            return bindname(exec_context, rule, name, defn)
+         end
+
 	 args[#args+1] = definename
 	 rules[#rules+1] = definerule
       else
diff --git a/lib/lace/error.lua b/lib/lace/error.lua
index 6277233..abbfa2f 100644
--- a/lib/lace/error.lua
+++ b/lib/lace/error.lua
@@ -71,11 +71,20 @@ end
 -- @treturn table The error table (mutated with the source information).
 -- @function augment
 local function _augment(err, source, linenr)
-   err.source = source
-   err.linenr = linenr
+   err.words.source = source
+   err.words.linenr = linenr
    return err
 end
 
+local function _subwords(err, word)
+   local subwords = err.words
+   if subwords and #subwords > 0 then
+      return {nr = word, sub = subwords}
+   else
+      return word
+   end
+end
+
 --- Render an error down to a string.
 --
 -- Error tables carry a message, an optional set of words which caused the
@@ -92,28 +101,40 @@ local function _render(err)
    -- A rendered error has four lines
    -- The first line is the error message
    local ret = { err.msg }
-   -- The second is the source filename and line
-   ret[2] = err.source.source .. " :: " .. tostring(err.linenr)
-   -- The third line is the line of the input
-   local srcline = err.source.lines[err.linenr] or {
-      original = "???", content = { {spos = 1, epos = 3, str = "???"} }
-   }
-   ret[3] = srcline.original
-   -- The fourth line is the highlight for each word in question
+
    local wordset = {}
-   local function build_wordset(words, wordset)
+   local function build_wordset(words, wordset, parent_source, parent_linenr)
+      wordset.source = words.source or source
+      wordset.linenr = words.linenr or linenr
       for _, word in ipairs(words) do
          if type(word) ~= "table" then
             wordset[word] = true
          else
             local subwordset = {}
-            build_wordset(word.sub, subwordset)
+            build_wordset(word.sub, subwordset, wordset.source, wordset.linenr)
             wordset[word.nr] = subwordset
          end
       end
    end
    build_wordset(err.words, wordset)
 
+   local linelist = {}
+   local function build_linelist(wordset, parent_source, parent_linenr)
+      if parent_source ~= wordset.source or parent_linenr ~= wordset.linenr then
+         linelist[#linelist+1] = wordset
+      end
+      local srcline = wordset.source.lines[wordset.linenr] or {
+         original = "???", content = { {spos = 1, epos = 3, str = "???"} }
+      }
+      for w, info in ipairs(srcline.content) do
+         -- TODO: Sometimes wordset is table, but token has no subwords.
+         if type(wordset[w]) == "table" and info.sub then
+            build_linelist(wordset[w], wordset.source, wordset.linenr)
+         end
+      end
+   end
+   build_linelist(wordset)
+
    local function mark_my_words(line, wordset)
       local hlstr, cpos = "", 1
       for w, info in ipairs(line) do
@@ -122,12 +143,12 @@ local function _render(err)
             hlstr = hlstr .. " "
             cpos = cpos + 1
          end
-         -- TODO: The subword can be defined in a different line entirely,
-         --       at which point it's not a subword of word in this line.
-         --       This is the norm for explicit definitions.
-         --       Eventually we should trace back to the define's
-         --       definition and highlight where in that line the problem is.
-         if type(wordset[w]) == "table" and info.sub then
+         --  The subword can be defined in a different line entirely,
+         --  at which point it's not a subword of word in this line.
+         --  This is the norm for explicit definitions.
+         if info.sub and type(wordset[w]) == "table"
+         and wordset[w].source == wordset.source
+         and wordset[w].linenr == wordset.linenr then
             -- space for [
             hlstr, cpos = hlstr .. " ", cpos + 1
 
@@ -148,10 +169,18 @@ local function _render(err)
       end
       return hlstr, cpos
    end
-   local hlstr, _ = mark_my_words(srcline.content, wordset)
 
-   ret[4] = hlstr
-   -- The rendered error is those four strings joined by newlines
+   for _, wordset in ipairs(linelist) do
+      ret[#ret+1] = wordset.source.source .. " :: " .. tostring(wordset.linenr)
+      local srcline = wordset.source.lines[wordset.linenr] or {
+         original = "???", content = { {spos = 1, epos = 3, str = "???"} }
+      }
+      ret[#ret+1] = srcline.original
+      local hlstr, _ = mark_my_words(srcline.content, wordset)
+      ret[#ret+1] = hlstr
+   end
+
+   -- The rendered error is those strings joined by newlines
    return table.concat(ret, "\n")
 end
 
@@ -159,5 +188,6 @@ return {
    error = _error,
    offset = _offset,
    augment = _augment,
+   subwords = _subwords,
    render = _render,
 }
diff --git a/test/test-lace.builtin.lua b/test/test-lace.builtin.lua
index b2eafde..84499a3 100644
--- a/test/test-lace.builtin.lua
+++ b/test/test-lace.builtin.lua
@@ -55,10 +55,10 @@ function suite.compile_builtin_allow_deny_novariables()
    assert(type(cmdtab) == "table", "Result should be a table")
    assert(type(cmdtab.fn) == "function", "Result should contain a function")
    assert(type(cmdtab.args) == "table", "Result table should contain an args table")
-   assert(cmdtab.args[1] == "allow", "Result args table should contain the given result 'allow'")
-   assert(cmdtab.args[2] == "because", "Result args table should contain te given reason 'because'")
-   assert(type(cmdtab.args[3]) == "table", "The third argument should be a table")
-   assert(#cmdtab.args[3] == 0, "There should be no conditions")
+   assert(cmdtab.args[2] == "allow", "Result args table should contain the given result 'allow'")
+   assert(cmdtab.args[3] == "because", "Result args table should contain te given reason 'because'")
+   assert(type(cmdtab.args[4]) == "table", "The third argument should be a table")
+   assert(#cmdtab.args[4] == 0, "There should be no conditions")
 end
 
 function suite.run_builtin_allow_deny_novariables()
@@ -203,7 +203,7 @@ function suite.compile_builtin_define_badctype()
    assert(cmdtab == false, "Internal errors should return false")
    assert(type(msg) == "table", "Internal errors should return tables")
    assert(type(msg.msg) == "string", "Internal errors should have string messages")
-   assert(msg.msg:match("Unknown control"), "Expected error should mention unknown control type")
+   assert(msg.msg:match("must be a control type"), "Expected error should mention unknown control type")
 end
 
 function suite.compile_builtin_define_ctype_errors()
@@ -536,17 +536,17 @@ function suite.compile_anyof_two_args()
    assert(type(cmdtab) == "table", "Successful compilations should return tables")
    assert(type(cmdtab.fn) == "function", "With functions")
    assert(type(cmdtab.args) == "table", "And arguments")
-   assert(#cmdtab.args == 2, "There should be two args")
-   assert(type(cmdtab.args[1]) == "string", "The first should be a table")
-   assert(type(cmdtab.args[2]) == "table", "The second should be a bool")
-   local ctrltab = cmdtab.args[2]
+   assert(#cmdtab.args == 3, "There should be two args")
+   assert(type(cmdtab.args[2]) == "string", "The first should be a table")
+   assert(type(cmdtab.args[3]) == "table", "The second should be a bool")
+   local ctrltab = cmdtab.args[3]
    assert(type(ctrltab) == "table", "Successfully compiled control functions should return tables")
    assert(type(ctrltab.fn) == "function", "With functions")
    assert(type(ctrltab.args) == "table", "And arguments")
-   assert(#ctrltab.args == 2, "There should be two args")
-   assert(type(ctrltab.args[1]) == "table", "The first should be a table")
-   assert(type(ctrltab.args[2]) == "boolean", "The second should be a bool")
-   assert(ctrltab.args[2] == true, "The anyof indicator should be true")
+   assert(#ctrltab.args == 3, "There should be two args")
+   assert(type(ctrltab.args[2]) == "table", "The first should be a table")
+   assert(type(ctrltab.args[3]) == "boolean", "The second should be a bool")
+   assert(ctrltab.args[3] == true, "The anyof indicator should be true")
 end
 
 function suite.compile_allof_two_args()
@@ -555,17 +555,17 @@ function suite.compile_allof_two_args()
    assert(type(cmdtab) == "table", "Successful compilations should return tables")
    assert(type(cmdtab.fn) == "function", "With functions")
    assert(type(cmdtab.args) == "table", "And arguments")
-   assert(#cmdtab.args == 2, "There should be two args")
-   assert(type(cmdtab.args[1]) == "string", "The first should be a table")
-   assert(type(cmdtab.args[2]) == "table", "The second should be a bool")
-   local ctrltab = cmdtab.args[2]
+   assert(#cmdtab.args == 3, "There should be two args")
+   assert(type(cmdtab.args[2]) == "string", "The first should be a table")
+   assert(type(cmdtab.args[3]) == "table", "The second should be a bool")
+   local ctrltab = cmdtab.args[3]
    assert(type(ctrltab) == "table", "Successfully compiled control functions should return tables")
    assert(type(ctrltab.fn) == "function", "With functions")
    assert(type(ctrltab.args) == "table", "And arguments")
-   assert(#ctrltab.args == 2, "There should be two args")
-   assert(type(ctrltab.args[1]) == "table", "The first should be a table")
-   assert(type(ctrltab.args[2]) == "boolean", "The second should be a bool")
-   assert(ctrltab.args[2] == false, "The anyof indicator should be false")
+   assert(#ctrltab.args == 3, "There should be two args")
+   assert(type(ctrltab.args[2]) == "table", "The first should be a table")
+   assert(type(ctrltab.args[3]) == "boolean", "The second should be a bool")
+   assert(ctrltab.args[3] == false, "The anyof indicator should be false")
 end
 
 function suite.run_anyof_two_args()
diff --git a/test/test-lace.compiler.lua b/test/test-lace.compiler.lua
index d6a26a2..9eb7e99 100644
--- a/test/test-lace.compiler.lua
+++ b/test/test-lace.compiler.lua
@@ -83,9 +83,9 @@ function suite.no_unconditional_action()
    -- rule 2 should be an unconditional allow with 'Default behaviour' as the reason,
    -- let's check
    local r2a = result.rules[2].args
-   assert(r2a[1] == "allow", "Rule 2 should be an allow")
-   assert(r2a[2] == "Default behaviour", "Rule 2's reason should be 'Default behaviour'")
-   assert(#r2a[3] == 0, "Rule 2 should have no conditions")
+   assert(r2a[2] == "allow", "Rule 2 should be an allow")
+   assert(r2a[3] == "Default behaviour", "Rule 2's reason should be 'Default behaviour'")
+   assert(#r2a[4] == 0, "Rule 2 should have no conditions")
 end
 
 function suite.no_unconditional_action_default_deny()
@@ -99,9 +99,9 @@ function suite.no_unconditional_action_default_deny()
    -- rule 3 should be an unconditional deny with 'Default behaviour' as the reason,
    -- let's check
    local r3a = result.rules[3].args
-   assert(r3a[1] == "deny", "Rule 3 should be a deny, despite last rule behind a deny")
-   assert(r3a[2] == "Default behaviour", "Rule 3's reason should be 'Default behaviour'")
-   assert(#r3a[3] == 0, "Rule 2 should have no conditions")
+   assert(r3a[2] == "deny", "Rule 3 should be a deny, despite last rule behind a deny")
+   assert(r3a[3] == "Default behaviour", "Rule 3's reason should be 'Default behaviour'")
+   assert(#r3a[4] == 0, "Rule 3 should have no conditions")
 end
 
 function suite.is_unconditional_action_default_deny()
@@ -115,9 +115,9 @@ function suite.is_unconditional_action_default_deny()
    -- rule 2 should be an unconditional allow with 'stuff' as the reason
    -- let's check
    local r2a = result.rules[2].args
-   assert(r2a[1] == "allow", "Rule 2 should be an allow, despite default being deny")
-   assert(r2a[2] == "stuff", "Rule 2's reason should be 'stuff'")
-   assert(#r2a[3] == 0, "Rule 2 should have no conditions")
+   assert(r2a[2] == "allow", "Rule 2 should be an allow, despite default being deny")
+   assert(r2a[3] == "stuff", "Rule 2's reason should be 'stuff'")
+   assert(#r2a[4] == 0, "Rule 2 should have no conditions")
 end
 
 -- Now we set up a more useful context and use that going forward:
@@ -341,7 +341,7 @@ function suite.error_in_define4()
    assert(type(msg) == "string", "Compilation errors should be strings")
    assert(msg:find("\n"), "Compilation errors are multiline")
    local line1, line2, line3, line4 = msg:match("^([^\n]*)\n([^\n]*)\n([^\n]*)\n([^\n]*)$")
-   assert(line1:find("Unknown control"), "The first line must mention the error")
+   assert(line1:find("must be a control type"), "The first line must mention the error")
    assert(line2 == "real-errorindefine4 :: 3", "The second line is where the error happened")
    assert(line3 == "define fish does_not_exist", "The third line is the original line")
    assert(line4 == "            ^^^^^^^^^^^^^^", "The fourth line highlights relevant words")
diff --git a/test/test-lace.engine-chaindefine-error.rules b/test/test-lace.engine-chaindefine-error.rules
new file mode 100644
index 0000000..1620172
--- /dev/null
+++ b/test/test-lace.engine-chaindefine-error.rules
@@ -0,0 +1,2 @@
+define bogus error
+allow "FAIL" [anyof [equal jeff banana] bogus]
diff --git a/test/test-lace.engine.lua b/test/test-lace.engine.lua
index 936a2ed..7b8a055 100644
--- a/test/test-lace.engine.lua
+++ b/test/test-lace.engine.lua
@@ -289,6 +289,23 @@ function suite.subsubdefine_err_reported()
    assert(line4 == "                                         ^^^^^  ", "The fourth line highlights relevant words")
 end
 
+function suite.subdefine_chained_err_reported()
+   local ruleset, msg = lace.compiler.compile(comp_context, "chaindefine-error")
+   assert(type(ruleset) == "table", "Ruleset did not compile")
+   local ectx = {error = true}
+   local result, msg = lace.engine.run(ruleset, ectx)
+   assert(result == false, "Did not error out")
+   local lines = {}
+   msg:gsub("([^\n]*)\n?", function(c) table.insert(lines, c) end)
+   assert(lines[1] == "woah", "The first line must mention the error")
+   assert(lines[2] == "real-chaindefine-error :: 2", "The second line is where the error happened")
+   assert(lines[3] == 'allow "FAIL" [anyof [equal jeff banana] bogus]', "The third line is the original line")
+   assert(lines[4] == "                                        ^^^^^ ", "The fourth line highlights relevant words")
+   assert(lines[5] == "real-chaindefine-error :: 1", "The fifth line is where the definition that errored comes from")
+   assert(lines[6] == 'define bogus error', "The sixth line is the define")
+   assert(lines[7] == "             ^^^^^", "The seventh line highlights relevant words")
+end
+
 local count_ok = 0
 for _, testname in ipairs(testnames) do
 --   print("Run: " .. testname)
diff --git a/test/test-lace.error.lua b/test/test-lace.error.lua
index 6f7d995..a2e09a7 100644
--- a/test/test-lace.error.lua
+++ b/test/test-lace.error.lua
@@ -52,8 +52,8 @@ function suite.error_augmentation()
    local src = {}
    local aug = error.augment(err, src, 10)
    assert(aug == err, "Augmentation should return the error")
-   assert(err.source == src, "Augmented errors should contain their source data")
-   assert(err.linenr == 10, "Augmented errors should contain their error line")
+   assert(err.words.source == src, "Augmented errors should contain their source data")
+   assert(err.words.linenr == 10, "Augmented errors should contain their error line")
 end
 
 function suite.error_render()