fix line attribution

dune-project

@@ -10,7 +10,7 @@
 (package
  (name libdash)
  (synopsis "Bindings to the dash shell's parser")
- (version 0.5.0)
+ (version 0.5.1)
  (depends
    ("ctypes" (>= "0.21.1"))
    ("ctypes-foreign" (>= "0.21.1"))

libdash/_dash.py

@@ -223,8 +223,8 @@ class strpush (Structure):
 #     struct alias *ap;   /* if push was associated with an alias */
 #     char *string;       /* remember the string since it may change */
 #
-#     /* Remember last two characters for pungetc. */
-#     int lastc[2];
+#     /* Delay freeing so we can stop nested aliases. */
+#     struct strpush *spfree;
 #
 #     /* Number of outstanding calls to pungetc. */
 #     int unget;
@@ -234,7 +234,7 @@ class strpush (Structure):
                     ("prevnleft", c_int),
                     ("ap", c_void_p),
                     ("string", c_char_p),
-                    ("lastc", 2 * c_int),
+                    ("spfree", POINTER (strpush)),
                     ("unget", c_int)];
 
 class parsefile (Structure):
@@ -245,14 +245,18 @@ class parsefile (Structure):
 #     int linno;      /* current line */
 #     int fd;         /* file descriptor (or -1 if string) */
 #     int nleft;      /* number of chars left in this line */
-#     int lleft;      /* number of chars left in this buffer */
+#     int eof;        /* do not read again once we hit EOF */
 #     char *nextc;        /* next char in buffer */
 #     char *buf;      /* input buffer */
 #     struct strpush *strpush; /* for pushing strings at this level */
 #     struct strpush basestrpush; /* so pushing one is fast */
 #
-#     /* Remember last two characters for pungetc. */
-#     int lastc[2];
+#     /* Delay freeing so we can stop nested aliases. */
+#     struct strpush *spfree;
+#
+# #ifndef SMALL
+#     int lleft;      /* number of chars left in this buffer */
+# #endif
 #
 #     /* Number of outstanding calls to pungetc. */
 #     int unget;
@@ -261,12 +265,13 @@ class parsefile (Structure):
                       ("linno",       c_int),
                       ("fd",          c_int),
                       ("nleft",       c_int),
-                      ("lleft",       c_int),
+                      ("eof",         c_int),
                       ("nextc",       POINTER (c_char)), # NOT c_char_p!
                       ("buf",         c_char_p),
                       ("strpush",     POINTER (strpush)),
                       ("basestrpush", strpush),
-                      ("lastc",       2 * c_int),
+                      ("spfree",      POINTER (strpush)),
+                      ("lleft",       c_int),
                       ("unget",       c_int)];
 
 

libdash/parser.py

@@ -16,7 +16,8 @@ def libdash_library_path():
     LIBDASH_LIBRARY_PATH = os.path.join(FILE_PATH, "libdash.so")
     return LIBDASH_LIBRARY_PATH
 
-EOF_NLEFT = -99; # libdash/src/input.c
+# parsefile->eof bit 1: set by preadbuffer at EOF, never cleared by pungetc.
+PARSEFILE_EOF = 2;
 
 class ParsingException(Exception):
     def __init__(self, message='ParseError'):
@@ -64,7 +65,7 @@ def parse(inputPath, init=True):
         n_ptr_C = parsecmd_safe (libdash, False)
 
         linno_after = parsefile_var.contents.linno - 1; # libdash is 1-indexed
-        nleft_after = parsefile_var.contents.nleft
+        eof_after = parsefile_var.contents.eof
 
         if (n_ptr_C == None): # Dash.Null
             pass
@@ -73,19 +74,18 @@ def parse(inputPath, init=True):
         elif (n_ptr_C == NERR): # Dash.Error
             raise ParsingException()
         else:
-            if (nleft_after == EOF_NLEFT):
-                linno_after = linno_after + 1; # The last line wasn't counted
-
-                if (inputPath != "-"):
-                    ## Both of these assertions check "our" assumption with respect to the final parser state
-                    ## and are therefore not necessary if they become an issue.
-                    assert ((linno_after == len (lines)) or (linno_after == len (lines) + 1))
-
-                    # Last line did not have a newline
-                    assert (len (lines [-1]) > 0 and (lines [-1][-1] != '\n'))
-            elif nleft_after != 0:
-                # we formerly asserted that `nleft_after != 0`, but this no longer holds
-                linno_after = linno_after + 1; # The last line wasn't counted
+            ## dash bumps `linno` on newline, so an unterminated last line is
+            ## never counted. Only adjust at EOF; otherwise every node would
+            ## swallow the next one's first line.
+            if (eof_after & PARSEFILE_EOF):
+                if (inputPath == "-"):
+                    ## no lines to check against; trust the flag
+                    linno_after = linno_after + 1
+                elif (lines and (lines [-1][-1:] != '\n')):
+                    linno_after = len (lines)
+
+            if (inputPath != "-"):
+                linno_after = min (linno_after, len (lines))
 
             n_ptr = cast (n_ptr_C, POINTER (union_node))
             new_ast = of_node (n_ptr)

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "libdash"
-version = "0.5.0"
+version = "0.5.1"
 authors = [
   { name="Michael Greenberg", email="michael@greenberg.science" },
   { name="PaSh contributors" },

python/Makefile

@@ -1,9 +1,19 @@
-.PHONY:  test clean
+.PHONY:  test test-structs test-roundtrip test-line-mapping clean
 
-test: rt.py ../libdash/*.py
+test: test-structs test-roundtrip test-line-mapping
+
+# _dash.py hand-copies dash's struct layouts; nothing else checks them.
+test-structs: ../test/check_structs.py ../libdash/_dash.py
+	@../test/check_structs.py
+
+test-roundtrip: rt.py ../libdash/*.py
 	@find ../test/tests ../test/pash_tests -type f | while read f; do ../test/round_trip.sh ./rt.py "$$f"; done | tee python.log
 	@cat python.log | egrep '^[A-Z0-9_]+:' | cut -d ':' -f 1 | sort | uniq -c
 	@grep ':' python.log && echo "FAILED" && exit 1 || exit 0
 
+# Per-node line mapping; round_trip.sh cannot see it (rt.py uses only the AST).
+test-line-mapping: dump.py ../libdash/*.py
+	@../test/line_mapping.sh
+
 clean:
 	rm *.o *.so *.log

python/dump.py

@@ -0,0 +1,210 @@
+#!/usr/bin/env python3
+"""Dump the parser nodes libdash detects in a script.
+
+Per node: attributed line range, source lines, printed form, and the AST (-a).
+
+Also audits attribution across nodes, reporting OVERLAP (range runs into the
+next node), GAP (lines belong to no node), EMPTY, INVERTED, OVERRUN and
+UNCOVERED. Exits nonzero on any, so it doubles as a regression test.
+"""
+
+import argparse
+import os
+import pprint
+import sys
+
+try:
+    import libdash
+except ModuleNotFoundError:
+    # fall back to the working tree, so libdash/*.py edits need no reinstall
+    sys.path.insert (0, os.path.dirname (os.path.dirname (os.path.abspath (__file__))))
+    import libdash
+
+sys.setrecursionlimit (9001)
+
+
+class Problem:
+    def __init__ (self, kind, index, detail):
+        self.kind = kind
+        self.index = index
+        self.detail = detail
+
+    def __str__ (self):
+        return "{}: node {}: {}".format (self.kind, self.index, self.detail)
+
+
+def read_lines (path):
+    with open (path, 'r') as fp:
+        return fp.readlines ()
+
+
+def collect (path, init):
+    """Parse `path`, returning (nodes, error) where nodes is a list of
+    (ast, lines, linno_before, linno_after)."""
+    nodes = []
+
+    try:
+        for node in libdash.parse (path, init):
+            nodes.append (node)
+    except Exception as e:
+        return (nodes, e)
+
+    return (nodes, None)
+
+
+def is_inert (line):
+    """A blank line or a whole-line comment yields no node, so it may legitimately
+    fall between two nodes."""
+    stripped = line.strip ()
+    return stripped == "" or stripped.startswith ("#")
+
+
+def audit (nodes, lines):
+    """Check the line ranges the parser handed back for overlaps and gaps."""
+    problems = []
+    prev_after = 0
+
+    for (i, (_ast, _text, before, after)) in enumerate (nodes):
+        if after < before:
+            problems.append (Problem ("INVERTED", i,
+                                      "range [{}, {}) runs backwards".format (before, after)))
+        elif after == before:
+            problems.append (Problem ("EMPTY", i,
+                                      "range [{}, {}) covers no lines".format (before, after)))
+
+        if before < prev_after:
+            problems.append (Problem ("OVERLAP", i,
+                                      "starts at line {} but the previous node ran through line {}"
+                                      .format (before, prev_after - 1)))
+        elif before > prev_after:
+            # Blank lines and comments produce no node, so only flag a gap that
+            # swallowed something the parser should have accounted for.
+            skipped = lines [prev_after:before]
+
+            if not all (is_inert (line) for line in skipped):
+                problems.append (Problem ("GAP", i,
+                                          "lines {}..{} belong to no node: {}"
+                                          .format (prev_after + 1, before,
+                                                   ", ".join (repr (l) for l in skipped
+                                                              if not is_inert (l)))))
+
+        prev_after = max (prev_after, after)
+
+    if nodes and prev_after > len (lines):
+        problems.append (Problem ("OVERRUN", len (nodes) - 1,
+                                  "last node ends at line {} but the file has {} lines"
+                                  .format (prev_after, len (lines))))
+
+    trailing = lines [prev_after:]
+
+    if nodes and not all (is_inert (line) for line in trailing):
+        problems.append (Problem ("UNCOVERED", len (nodes) - 1,
+                                  "lines {}..{} after the last node belong to no node: {}"
+                                  .format (prev_after + 1, len (lines),
+                                           ", ".join (repr (l) for l in trailing
+                                                      if not is_inert (l)))))
+
+    return problems
+
+
+def show (path, nodes, err, problems, args):
+    lines = read_lines (path)
+
+    print ("=== {} ({} lines) ===".format (path, len (lines)))
+
+    for (i, (ast, text, before, after)) in enumerate (nodes):
+        print ("--- node {}  lines [{}, {})  {}"
+               .format (i, before, after,
+                        "src lines {}..{}".format (before + 1, after) if after > before
+                        else "no source lines"))
+
+        if text is None:
+            print ("    src   | <stdin: not captured>")
+        else:
+            for (n, line) in enumerate (text.splitlines ()):
+                print ("    src   | {:>4} | {}".format (before + n + 1, line))
+
+        for line in libdash.to_string (ast).splitlines ():
+            print ("    print | {}".format (line))
+
+        if args.ast:
+            for line in pprint.pformat (ast, width = 100).splitlines ():
+                print ("    ast   | {}".format (line))
+
+    if err is not None:
+        print ("!!! {}: {}".format (type (err).__name__, err))
+
+    for p in problems:
+        print ("!!! {}".format (p))
+
+    print ("--- {} node(s), {} problem(s)".format (len (nodes), len (problems)))
+    print ()
+
+
+def show_ranges (path, nodes, err):
+    """One line per node, stable enough to diff against a golden file."""
+    for (i, (_ast, text, before, after)) in enumerate (nodes):
+        print ("node {}  [{}, {})  {!r}".format (i, before, after, text))
+
+    if err is not None:
+        print ("error {}: {}".format (type (err).__name__, err))
+
+
+def main ():
+    ap = argparse.ArgumentParser (description = __doc__,
+                                  formatter_class = argparse.RawDescriptionHelpFormatter)
+    ap.add_argument ("files", nargs = "*", help = "shell scripts to dump")
+    ap.add_argument ("-a", "--ast", action = "store_true",
+                     help = "also dump the raw AST for each node")
+    ap.add_argument ("-q", "--quiet", action = "store_true",
+                     help = "only show files with problems")
+    ap.add_argument ("-r", "--ranges", action = "store_true",
+                     help = "terse one-line-per-node output, for golden-file comparison")
+    ap.add_argument ("-w", "--which", action = "store_true",
+                     help = "print which libdash was imported, then exit")
+    args = ap.parse_args ()
+
+    if args.which:
+        print (libdash.__file__)
+        return 0
+
+    if not args.files:
+        ap.error ("no files given")
+
+    init = True
+    bad = 0
+
+    for path in args.files:
+        try:
+            lines = read_lines (path)
+        except OSError as e:
+            print ("=== {}: cannot read: {}".format (path, e))
+            bad += 1
+            continue
+
+        (nodes, err) = collect (path, init)
+        init = False
+
+        if args.ranges:
+            show_ranges (path, nodes, err)
+            continue
+
+        problems = audit (nodes, lines)
+
+        if problems:
+            bad += 1
+
+        if problems or not args.quiet:
+            show (path, nodes, err, problems, args)
+        elif err is not None:
+            print ("=== {}: {}: {}".format (path, type (err).__name__, err))
+            print ()
+
+    if bad:
+        print ("{} file(s) with line-attribution problems".format (bad))
+
+    return 1 if bad else 0
+
+
+if __name__ == "__main__":
+    sys.exit (main ())

setup.py

@@ -61,7 +61,7 @@ def run(self):
 setup(name='libdash',
       packages=['libdash'],
       cmdclass={'build_py': libdash_build_py},
-      version='0.5.0',
+      version='0.5.1',
       long_description=long_description,
       long_description_content_type='text/markdown',
       include_package_data=True,

test/README.md

@@ -1,7 +1,24 @@
-There are three directories of tests:
+There are four directories of tests:
 
   - `tests` are the original libdash tests, mostly handwritten
   - `pash_tests` are shell scripts taken from [`pash`](https://github.com/binpash/pash)
   - `failing` are shell scripts that aren't working right now (which is probably a bug)
+  - `line_mapping` holds fixtures for the per-node source line mapping, each with a
+    `.expected` golden
 
-Both OCaml and Python bindings use the `round_trip.sh` to test round tripping. The `test_ocaml_python.sh` script compares the output from Python and OCaml.
+Both OCaml and Python bindings use the `round_trip.sh` to test round tripping, i.e., that `print . parse` reaches a fixpoint.
+It cannot compare against the original source, since the AST drops comments, whitespace and quoting style.
+The `test_ocaml_python.sh` script ensures that Python and OCaml round-trip to the same output (or fail together).
+
+Round trip tests, however, can hide important bugs.
+Two other tests check parses more concretely.
+
+`check_structs.py` checks the ctypes mirrors in `libdash/_dash.py` against
+dash's real structs, comparing `sizeof` and every field offset via a probe
+compiled from `src/*.h`. 
+This test needs a C compiler and a built tree.
+This test only runs on the Python code, as OCaml's ctypes automatically computes offsets, but `_dash.py` hand-writes them.
+
+`line_mapping.sh` covers source line mapping (`parsedLines`, `linno_before`, `linno_after`).
+It runs `python/dump.py --ranges` over each fixture and diffs against the `.expected` output.
+Run `REGEN=1 ./line_mapping.sh` to automatically update the expected output after an intentional change.