metadata and doc generator optimization and fixes

* Fix ansible-doc traceback when a plugin doesn't parse correctly
* Change extract_metadata ivocation to take either an ast or source
  code.  When given source code, it can find file offsets for the start
  and end of dict.  When given the ast, it is quicker as it doesn't have
  to reparse the source.  Requires changing the call to the function to
  use a keyword arg.
* Fix reading of metadata to find the last occurrence of
  ANSIBLE_METADATA instead of the first.
* Add some more unittests to get closer to complete coverage

hacking/metadata-tool.py

@@ -205,7 +205,8 @@ def write_metadata(filename, new_metadata, version=None, overwrite=False):
         module_data = f.read()
 
     try:
-        current_metadata, start_line, start_col, end_line, end_col, targets = extract_metadata(module_data)
+        current_metadata, start_line, start_col, end_line, end_col, targets = \
+            extract_metadata(module_data=module_data, offsets=True)
     except SyntaxError:
         if filename.endswith('.py'):
             raise
@@ -257,7 +258,7 @@ def return_metadata(plugins):
         if name not in metadata or metadata[name] is not None:
             with open(filename, 'rb') as f:
                 module_data = f.read()
-            metadata[name] = extract_metadata(module_data)[0]
+            metadata[name] = extract_metadata(module_data=module_data, offsets=True)[0]
     return metadata
 
 
@@ -408,7 +409,7 @@ def upgrade_metadata(version=None):
         # For each plugin, read the existing metadata
         with open(filename, 'rb') as f:
             module_data = f.read()
-        metadata = extract_metadata(module_data)[0]
+        metadata = extract_metadata(module_data=module_data, offsets=True)[0]
 
         # If the metadata isn't the requested version, convert it to the new
         # version

lib/ansible/cli/doc.py

@@ -224,6 +224,7 @@ class DocCLI(CLI):
             if os.path.isdir(filename):
                 continue
 
+            doc = None
             try:
                 doc, plainexamples, returndocs, metadata = plugin_docs.get_docstring(filename)
             except:

lib/ansible/parsing/metadata.py

@@ -20,6 +20,7 @@ from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
 import ast
+import sys
 
 import yaml
 
@@ -146,32 +147,53 @@ def _seek_end_of_string(module_data, start_line, start_col, next_node_line, next
     raise NotImplementedError('Finding end of string not yet implemented')
 
 
-def extract_metadata(module_data):
+def extract_metadata(module_ast=None, module_data=None, offsets=False):
     """Extract the metadata from a module
 
-    :arg module_data: Byte string containing a module's code
+    :kwarg module_ast: ast representation of the module.  At least one of this
+        or ``module_data`` must be given.  If the code calling
+        :func:`extract_metadata` has already parsed the module_data into an ast,
+        giving the ast here will save reparsing it.
+    :kwarg module_data: Byte string containing a module's code.  At least one
+        of this or ``module_ast`` must be given.
+    :kwarg offsets: If set to True, offests into the source code will be
+        returned.  This requires that ``module_data`` be set.
     :returns: a tuple of metadata (a dict), line the metadata starts on,
         column the metadata starts on, line the metadata ends on, column the
         metadata ends on, and the names the metadata is assigned to.  One of
-        the names the metadata is assigned to will be ANSIBLE_METADATA If no
-        metadata is found, the tuple will be (None, -1, -1, -1, -1, None)
+        the names the metadata is assigned to will be ANSIBLE_METADATA.  If no
+        metadata is found, the tuple will be (None, -1, -1, -1, -1, None).
+        If ``offsets`` is False then the tuple will consist of
+        (metadata, -1, -1, -1, -1, None).
+    :raises ansible.parsing.metadata.ParseError: if ``module_data`` does not parse
+    :raises SyntaxError: if ``module_data`` is needed but does not parse correctly
     """
+    if offsets and module_data is None:
+        raise TypeError('If offsets is True then module_data must also be given')
+
+    if module_ast is None and module_data is None:
+        raise TypeError('One of module_ast or module_data must be given')
+
     metadata = None
     start_line = -1
     start_col = -1
     end_line = -1
     end_col = -1
     targets = None
-    mod_ast_tree = ast.parse(module_data)
-    for root_idx, child in enumerate(mod_ast_tree.body):
+    if module_ast is None:
+        module_ast = ast.parse(module_data)
+
+    for root_idx, child in reversed(list(enumerate(module_ast.body))):
         if isinstance(child, ast.Assign):
             for target in child.targets:
                 if target.id == 'ANSIBLE_METADATA':
                     metadata = ast.literal_eval(child.value)
+                    if not offsets:
+                        continue
 
                     try:
                         # Determine where the next node starts
-                        next_node = mod_ast_tree.body[root_idx + 1]
+                        next_node = module_ast.body[root_idx + 1]
                         next_lineno = next_node.lineno
                         next_col_offset = next_node.col_offset
                     except IndexError:
@@ -202,10 +224,7 @@ def extract_metadata(module_data):
                                                                 next_lineno,
                                                                 next_col_offset)
                     else:
-                        # Example:
-                        #   ANSIBLE_METADATA = 'junk'
-                        #   ANSIBLE_METADATA = { [..the real metadata..] }
-                        continue
+                        raise ParseError('Ansible plugin metadata must be a dict')
 
                     # Do these after the if-else so we don't pollute them in
                     # case this was a false positive

lib/ansible/utils/plugin_docs.py

@@ -155,7 +155,7 @@ def get_docstring(filename, verbose=False):
                                     data[varkey] = child.value.s
                             display.debug('assigned :%s' % varkey)
 
-        data['metadata'] = extract_metadata(b_module_data)[0]
+        data['metadata'] = extract_metadata(module_ast=M)[0]
         # add fragments to documentation
         if data['doc']:
             add_fragments(data['doc'], filename)
@@ -165,7 +165,7 @@ def get_docstring(filename, verbose=False):
             for x in ('version', 'metadata_version'):
                 if x in data['metadata']:
                     del data['metadata'][x]
-    except:
+    except Exception as e:
         display.error("unable to parse %s" % filename)
         if verbose is True:
             display.display("unable to parse %s" % filename)

test/units/parsing/test_metadata.py

@@ -20,10 +20,13 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
-from ansible.parsing import metadata as md
+import ast
 
 import pytest
 
+from ansible.parsing import metadata as md
+
+
 LICENSE = b"""# some license text boilerplate
 # That we have at the top of files
 """
@@ -43,8 +46,17 @@ ANSIBLE_METADATA = {'metadata_version': '1.0',
                     'supported_by': 'core'}
 """
 
-STRING_STD_METADATA = b"""
-ANSIBLE_METADATA = '''
+TEXT_STD_METADATA = b"""
+ANSIBLE_METADATA = u'''
+metadata_version: '1.0'
+status:
+  - 'stableinterface'
+supported_by: 'core'
+'''
+"""
+
+BYTES_STD_METADATA = b"""
+ANSIBLE_METADATA = b'''
 metadata_version: '1.0'
 status:
   - 'stableinterface'
@@ -161,31 +173,67 @@ METADATA_EXAMPLES = (
      (HASH_SYMBOL_METADATA, 1, 0, 4, 42, ['ANSIBLE_METADATA'])),
 
     # Standard import with a junk ANSIBLE_METADATA as well
-    (LICENSE + FUTURE_IMPORTS + b"\nANSIBLE_METADAtA = 'junk'\n" + HASH_COMBO_METADATA + REGULAR_IMPORTS,
+    (LICENSE + FUTURE_IMPORTS + b"\nANSIBLE_METADATA = 10\n" + HASH_COMBO_METADATA + REGULAR_IMPORTS,
      (HASH_SYMBOL_METADATA, 7, 0, 10, 42, ['ANSIBLE_METADATA'])),
 )
 
 # FIXME: String/yaml metadata is not implemented yet.  Need more test cases once it is implemented
 STRING_METADATA_EXAMPLES = (
     # Standard import
-    (LICENSE + FUTURE_IMPORTS + STRING_STD_METADATA + REGULAR_IMPORTS,
+    (LICENSE + FUTURE_IMPORTS + TEXT_STD_METADATA + REGULAR_IMPORTS,
      (METADATA, 5, 0, 10, 3, ['ANSIBLE_METADATA'])),
     # Metadata at end of file
-    (LICENSE + FUTURE_IMPORTS + REGULAR_IMPORTS + STRING_STD_METADATA.rstrip(),
+    (LICENSE + FUTURE_IMPORTS + REGULAR_IMPORTS + TEXT_STD_METADATA.rstrip(),
      (METADATA, 8, 0, 13, 3, ['ANSIBLE_METADATA'])),
     # Metadata at beginning of file
-    (STRING_STD_METADATA + LICENSE + REGULAR_IMPORTS,
+    (TEXT_STD_METADATA + LICENSE + REGULAR_IMPORTS,
+     (METADATA, 1, 0, 6, 3, ['ANSIBLE_METADATA'])),
+
+    # Standard import
+    (LICENSE + FUTURE_IMPORTS + BYTES_STD_METADATA + REGULAR_IMPORTS,
+     (METADATA, 5, 0, 10, 3, ['ANSIBLE_METADATA'])),
+    # Metadata at end of file
+    (LICENSE + FUTURE_IMPORTS + REGULAR_IMPORTS + BYTES_STD_METADATA.rstrip(),
+     (METADATA, 8, 0, 13, 3, ['ANSIBLE_METADATA'])),
+    # Metadata at beginning of file
+    (BYTES_STD_METADATA + LICENSE + REGULAR_IMPORTS,
      (METADATA, 1, 0, 6, 3, ['ANSIBLE_METADATA'])),
 )
 
 
 @pytest.mark.parametrize("code, expected", METADATA_EXAMPLES)
-def test_extract_metadata(code, expected):
-    assert md.extract_metadata(code) == expected
+def test_dict_metadata(code, expected):
+    assert md.extract_metadata(module_data=code, offsets=True) == expected
 
 
 @pytest.mark.parametrize("code, expected", STRING_METADATA_EXAMPLES)
-def test_extract_string_metadata(code, expected):
+def test_string_metadata(code, expected):
     # FIXME: String/yaml metadata is not implemented yet.
     with pytest.raises(NotImplementedError):
-        assert md.extract_metadata(code) == expected
+        assert md.extract_metadata(module_data=code, offsets=True) == expected
+
+
+def test_required_params():
+    with pytest.raises(TypeError, message='One of module_ast or module_data must be given'):
+        assert md.extract_metadata()
+
+
+def test_module_data_param_given_with_offset():
+    with pytest.raises(TypeError, message='If offsets is True then module_data must also be given'):
+        assert md.extract_metadata(module_ast='something', offsets=True)
+
+
+def test_invalid_dict_metadata():
+    with pytest.raises(SyntaxError):
+        assert md.extract_metadata(module_data=LICENSE + FUTURE_IMPORTS + b'ANSIBLE_METADATA={"metadata_version": "1.0",\n' + REGULAR_IMPORTS)
+
+    with pytest.raises(md.ParseError, message='Unable to find the end of dictionary'):
+        assert md.extract_metadata(module_ast=ast.parse(LICENSE + FUTURE_IMPORTS + b'ANSIBLE_METADATA={"metadata_version": "1.0"}\n' + REGULAR_IMPORTS),
+                                   module_data=LICENSE + FUTURE_IMPORTS + b'ANSIBLE_METADATA={"metadata_version": "1.0",\n' + REGULAR_IMPORTS,
+                                   offsets=True)
+
+
+def test_multiple_statements_limitation():
+    with pytest.raises(md.ParseError, message='Multiple statements per line confuses the module metadata parser.'):
+        assert md.extract_metadata(module_data=LICENSE + FUTURE_IMPORTS + b'ANSIBLE_METADATA={"metadata_version": "1.0"}; a=b\n' + REGULAR_IMPORTS,
+                                   offsets=True)