[hawkmoth] Re: [PATCH 2/4] hawkmoth: add support for propagating diagnostics

  • From: Bruno Santos <brunomanuelsantos@xxxxxxxxxxxxxxxxxx>
  • To: Jani Nikula <jani@xxxxxxxxxx>
  • Date: Sun, 28 Apr 2019 22:08:18 +0200

On 22:01:33 2019-04-28, Jani Nikula wrote:

On Sun, 28 Apr 2019, Bruno Santos <brunomanuelsantos@xxxxxxxxxxxxxxxxxx> 
wrote:
At the same time, the first diagnostics are implemented or rather
forwarded from libclang parse. Clang diagnostics are limited to warnings
and higher as these are the levels at which documentation breaking
issues are found.

Original concept: Marius Vlad

At the high level this looks fine, please find some comments on the
details inline below.

BR,
Jani.

---
 hawkmoth/__init__.py  | 26 +++++++++++++++++++++++--
 hawkmoth/__main__.py  |  8 +++++---
 hawkmoth/parser.py    | 44 ++++++++++++++++++++++++++++++++++++-------
 test/test_hawkmoth.py |  4 +++-
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/hawkmoth/__init__.py b/hawkmoth/__init__.py
index 3355922..9141a0d 100644
--- a/hawkmoth/__init__.py
+++ b/hawkmoth/__init__.py
@@ -21,7 +21,7 @@
 from sphinx.util.docutils import switch_source_input
 from sphinx.util import logging
 
-from hawkmoth.parser import parse
+from hawkmoth.parser import parse, ErrorLevel
 
 with open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
                        'VERSION')) as version_file:
@@ -42,13 +42,35 @@ class CAutoDocDirective(Directive):
     }
     has_content = False
 
+    def __display_parser_diagnostics(self, errors):
+        env = self.state.document.settings.env
+
+        for (severity, filename, lineno, msg) in errors:
+            toprint = '{}:{}: {}'.format(filename, lineno, msg)
+
+            if severity is ErrorLevel.INFO:
+                if env.app.verbosity >= 2:
+                    self.logger.info(toprint,
+                                     location=(env.docname, self.lineno))

Shouldn't this report the C source filename and line number? I don't
imagine the location of the directive being interesting.

I think both are important, the error may be in the directive call
itself and you should be pointed to it.

You can see the source filename and line being added to `toprint` right
below the `for` statement.


+            elif severity is ErrorLevel.WARNING:
+                if env.app.verbosity >= 1:
+                    self.logger.warning(toprint,
+                                        location=(env.docname, 
self.lineno))
+            elif severity is ErrorLevel.ERROR:
+                self.logger.error(toprint, location=(env.docname, 
self.lineno))
+            else:
+                self.logger.critical(toprint,
+                                     location=(env.docname, self.lineno))

I thought we wouldn't do any verbosity filtering ourselves. Instead,
we'd map the clang levels to suitable sphinx levels, log
unconditionally, and have sphinx logger do the filtering based on
verbosity.

Hmm, I think I get what you mean. I'll try it out and see how that works
out. I kind of liked using names to qualify the level of verbosity
rather than a number, but it does open up some opportunities.


Further, with this I think we might be able to use logger.log() with the
level passed in instead of using the specific logger.info(),
logger.warning(), etc. methods.

Maybe, don't know of this alternative call method, but I'll look it up.


+
     def __parse(self, viewlist, filename):
         env = self.state.document.settings.env
 
         compat = self.options.get('compat', env.config.cautodoc_compat)
         clang = self.options.get('clang', env.config.cautodoc_clang)
 
-        comments = parse(filename, compat=compat, clang=clang)
+        comments, errors = parse(filename, compat=compat, clang=clang)
+
+        self.__display_parser_diagnostics(errors)
 
         for (comment, meta) in comments:
             lineoffset = meta['line'] - 1
diff --git a/hawkmoth/__main__.py b/hawkmoth/__main__.py
index 8d237d7..6689777 100644
--- a/hawkmoth/__main__.py
+++ b/hawkmoth/__main__.py
@@ -31,8 +31,10 @@ def main():
                         help='Verbose output.')
     args = parser.parse_args()
 
-    comments = parse_to_string(args.file, args.verbose,
-                               compat=args.compat, clang=args.clang)
-    sys.stdout.write(comments)
+    comments, errors = parse_to_string(args.file, args.verbose,
+                                       compat=args.compat, 
clang=args.clang)
+
+    print(comments)

sys.stdout.write() avoided extra newlines I think.

You were adding the lines explicitly before. But since here I haven't
nuked parse_to_string yet, you're probably right. Need to confirm.

In the final version, I'm pretty confident the result stayed the same as
in the original. This is also neater and avoids adding extra line breaks
explicitly, but it still needs fixing here.


+    print(errors, file=sys.stderr)
 
 main()
diff --git a/hawkmoth/parser.py b/hawkmoth/parser.py
index 5f2509b..20ca327 100644
--- a/hawkmoth/parser.py
+++ b/hawkmoth/parser.py
@@ -35,6 +35,7 @@
 
 import itertools
 import sys
+from enum import Enum, auto
 
 from clang.cindex import CursorKind
 from clang.cindex import Index, TranslationUnit
@@ -43,6 +44,12 @@
 
 from hawkmoth.util import docstr, doccompat
 
+class ErrorLevel(Enum):
+    """Enumeration of supported log levels."""
+    INFO = auto()
+    WARNING = auto()
+    ERROR = auto()
+
 def comment_extract(tu):
 
     # FIXME: How to handle top level comments above a cursor that it does 
*not*
@@ -239,10 +246,24 @@ def _recursive_parse(comments, cursor, nest, compat):
 
     return [(doc, meta)]
 
+def clang_diagnostics(errors, diagnostics):
+    sev = {0: ErrorLevel.INFO,
+           1: ErrorLevel.INFO,
+           2: ErrorLevel.WARNING,
+           3: ErrorLevel.ERROR,
+           4: ErrorLevel.ERROR}
+
+    for diag in diagnostics:
+        # Discard any Clang error lower than a warning.
+        if diag.severity >= 2:

I don't mind this, but at the same time I think we could just log them
all, and if the user adds a sufficient amount of -v's to sphinx-build,
they'd get everything. We'd just have to map the levels suitably to
sphinx levels.

Yeah, I did stutter for a minute here. My reasoning was that whatever
Clang spits out below that level is in all likelihood of no consequence
for building documentation.

While I see the point in allowing INFO level of output at high
verbosity, I'd still limit it to things that matter for the
documentation.

On the other hand, we can simply go for more levels:

- '':     ERROR
- '-v':   WARNING
- '-vv':  INFO that is still (likely) relevant to documentation or the
  inner workings of Hwkmoth
- '-vvv': Everything under the sun?


+            errors.extend([(sev[diag.severity], diag.location.file.name,
+                            diag.location.line, diag.spelling)])
+
 # return a list of (comment, metadata) tuples
 # options - dictionary with directive options
 def parse(filename, **options):
 
+    errors = []
     args = options.get('clang')
     if args is not None:
         args = [s.strip() for s in args.split(',') if len(s.strip()) > 0]
@@ -255,6 +276,8 @@ def parse(filename, **options):
                      TranslationUnit.PARSE_DETAILED_PROCESSING_RECORD |
                      TranslationUnit.PARSE_SKIP_FUNCTION_BODIES)
 
+    clang_diagnostics(errors, tu.diagnostics)
+
     top_level_comments, comments = comment_extract(tu)
 
     result = []
@@ -270,13 +293,20 @@ def parse(filename, **options):
     # Sort all elements by order of appearance.
     result.sort(key=lambda r: r[1]['line'])
 
-    return result
+    return result, errors
 
 def parse_to_string(filename, verbose, **options):
-    s = ''
-    comments = parse(filename, **options)
-    for (comment, meta) in comments:
+    docs_str = ''
+    errors_str = ''
+    docs, errors = parse(filename, **options)
+
+    for (doc, meta) in docs:
         if verbose:
-            s += ('# ' + str(meta) + '\n')
-        s += comment + '\n'
-    return s
+            docs_str += ('# ' + str(meta) + '\n')
+        docs_str += doc + '\n'
+
+    for (severity, filename, lineno, msg) in errors:
+        errors_str += '{}: {}:{}: {}\n'.format(severity.name,
+                                               filename, lineno, msg)
+
+    return docs_str, errors_str
diff --git a/test/test_hawkmoth.py b/test/test_hawkmoth.py
index 84101c3..61c0f08 100755
--- a/test/test_hawkmoth.py
+++ b/test/test_hawkmoth.py
@@ -9,7 +9,9 @@
 from hawkmoth.parser import parse_to_string
 
 def _get_output(input_filename, **options):
-    return parse_to_string(input_filename, False, **options)
+    output, errors = parse_to_string(input_filename, False, **options)
+    # FIXME: should handle errors
+    return output
 
 def _get_expected(input_filename, **options):
     return testenv.read_file(input_filename, ext='rst')
-- 
2.21.0

Other related posts: