Fix various problems with --import-graph filename parsing (#4259)

Avoids backwards-incompatible changes.
 - Raise error if graphvis is not installed (instead of reporting success)
 - Fix tempfile creation bug when outputfile includes directories
 - Fix bug when using file extension that isn't 3 characters long
 - Fix confusing help text
 - Rename deprecated .dot extension to .gv
 - Default to .png if no extension is specified

* Add typing to modified functions (and ignore mypy thinking codecs.open()
returns an int)

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

5 files changed, 65 insertions, 34 deletions

diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index eb309acda..1924b31e1 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -462,3 +462,5 @@ contributors:
 * Mark Byrne: contributor
 
 * Konstantina Saketou: contributor
+
+* Andrew Howe: contributor
diff --git a/ChangeLog b/ChangeLog
index c85071544..6b2593686 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -80,6 +80,8 @@ Release date: TBA
 
 * Improve check if class is subscriptable PEP585
 
+* Fix documentation and filename handling of --import-graph
+
 * Fix false-positive for ``unused-import`` on class keyword arguments
 
   Closes #3202
diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py
index 9d4e60e6f..d3a5ca2f4 100644
--- a/pylint/checkers/imports.py
+++ b/pylint/checkers/imports.py
@@ -44,6 +44,7 @@ import copy
 import os
 import sys
 from distutils import sysconfig
+from typing import Dict, List
 
 import astroid
 
@@ -56,7 +57,7 @@ from pylint.checkers.utils import (
 from pylint.exceptions import EmptyReportError
 from pylint.graph import DotBackend, get_cycles
 from pylint.interfaces import IAstroidChecker
-from pylint.reporters.ureports.nodes import Paragraph, VerbatimText
+from pylint.reporters.ureports.nodes import Paragraph, VerbatimText, VNode
 from pylint.utils import IsortDriver, get_global_option
 
 
@@ -172,10 +173,10 @@ def _repr_tree_defs(data, indent_str=None):
     return "\n".join(lines)
 
 
-def _dependencies_graph(filename, dep_info):
+def _dependencies_graph(filename: str, dep_info: Dict[str, List[str]]) -> str:
     """write dependencies as a dot (graphviz) file"""
     done = {}
-    printer = DotBackend(filename[:-4], rankdir="LR")
+    printer = DotBackend(os.path.splitext(os.path.basename(filename))[0], rankdir="LR")
     printer.emit('URL="." node[shape="box"]')
     for modname, dependencies in sorted(dep_info.items()):
         done[modname] = 1
@@ -187,15 +188,15 @@ def _dependencies_graph(filename, dep_info):
     for depmodname, dependencies in sorted(dep_info.items()):
         for modname in dependencies:
             printer.emit_edge(modname, depmodname)
-    printer.generate(filename)
+    return printer.generate(filename)
 
 
-def _make_graph(filename, dep_info, sect, gtype):
+def _make_graph(filename: str, dep_info: Dict[str, List[str]], sect: VNode, gtype: str):
     """generate a dependencies graph and add some information about it in the
     report's section
     """
-    _dependencies_graph(filename, dep_info)
-    sect.append(Paragraph(f"{gtype}imports graph has been written to {filename}"))
+    outputfile = _dependencies_graph(filename, dep_info)
+    sect.append(Paragraph(f"{gtype}imports graph has been written to {outputfile}"))
 
 
 # the import checker itself ###################################################
@@ -332,9 +333,9 @@ class ImportsChecker(DeprecatedMixin, BaseChecker):
             {
                 "default": "",
                 "type": "string",
-                "metavar": "<file.dot>",
-                "help": "Create a graph of every (i.e. internal and"
-                " external) dependencies in the given file"
+                "metavar": "<file.gv>",
+                "help": "Output a graph (.gv or any supported image format) of"
+                " all (i.e. internal and external) dependencies to the given file"
                 " (report RP0402 must not be disabled).",
             },
         ),
@@ -343,9 +344,10 @@ class ImportsChecker(DeprecatedMixin, BaseChecker):
             {
                 "default": "",
                 "type": "string",
-                "metavar": "<file.dot>",
-                "help": "Create a graph of external dependencies in the"
-                " given file (report RP0402 must not be disabled).",
+                "metavar": "<file.gv>",
+                "help": "Output a graph (.gv or any supported image format)"
+                " of external dependencies to the given file"
+                " (report RP0402 must not be disabled).",
             },
         ),
         (
@@ -353,9 +355,10 @@ class ImportsChecker(DeprecatedMixin, BaseChecker):
             {
                 "default": "",
                 "type": "string",
-                "metavar": "<file.dot>",
-                "help": "Create a graph of internal dependencies in the"
-                " given file (report RP0402 must not be disabled).",
+                "metavar": "<file.gv>",
+                "help": "Output a graph (.gv or any supported image format)"
+                " of internal dependencies to the given file"
+                " (report RP0402 must not be disabled).",
             },
         ),
         (
diff --git a/pylint/graph.py b/pylint/graph.py
index 5078ddefa..e71822b09 100644
--- a/pylint/graph.py
+++ b/pylint/graph.py
@@ -18,6 +18,7 @@
 """
 import codecs
 import os
+import shutil
 import subprocess
 import sys
 import tempfile
@@ -28,7 +29,7 @@ def target_info_from_filename(filename):
     """Transforms /some/path/foo.png into ('/some/path', 'foo.png', 'png')."""
     basename = osp.basename(filename)
     storedir = osp.dirname(osp.abspath(filename))
-    target = filename.split(".")[-1]
+    target = osp.splitext(filename)[-1][1:]
     return storedir, basename, target
 
 
@@ -76,7 +77,7 @@ class DotBackend:
 
     source = property(get_source)
 
-    def generate(self, outputfile=None, mapfile=None):
+    def generate(self, outputfile: str = None, mapfile: str = None) -> str:
         """Generates a graph file.
 
         :param str outputfile: filename and path [defaults to graphname.png]
@@ -84,25 +85,36 @@ class DotBackend:
 
         :rtype: str
         :return: a path to the generated file
+        :raises RuntimeError: if the executable for rendering was not found
         """
+        graphviz_extensions = ("dot", "gv")
         name = self.graphname
-        if outputfile is not None:
-            _, _, target = target_info_from_filename(outputfile)
-            if target != "dot":
-                pdot, dot_sourcepath = tempfile.mkstemp(".dot", name)
-                os.close(pdot)
-            else:
-                dot_sourcepath = outputfile
-        else:
+        if outputfile is None:
             target = "png"
-            pdot, dot_sourcepath = tempfile.mkstemp(".dot", name)
+            pdot, dot_sourcepath = tempfile.mkstemp(".gv", name)
             ppng, outputfile = tempfile.mkstemp(".png", name)
             os.close(pdot)
             os.close(ppng)
-        pdot = codecs.open(dot_sourcepath, "w", encoding="utf8")
-        pdot.write(self.source)
-        pdot.close()
-        if target != "dot":
+        else:
+            _, _, target = target_info_from_filename(outputfile)
+            if not target:
+                target = "png"
+                outputfile = outputfile + "." + target
+            if target not in graphviz_extensions:
+                pdot, dot_sourcepath = tempfile.mkstemp(".gv", name)
+                os.close(pdot)
+            else:
+                dot_sourcepath = outputfile
+        pdot = codecs.open(dot_sourcepath, "w", encoding="utf8")  # type: ignore
+        pdot.write(self.source)  # type: ignore
+        pdot.close()  # type: ignore
+        if target not in graphviz_extensions:
+            if shutil.which(self.renderer) is None:
+                raise RuntimeError(
+                    f"Cannot generate `{outputfile}` because '{self.renderer}' "
+                    "executable not found. Install graphviz, or specify a `.gv` "
+                    "outputfile to produce the DOT source code."
+                )
             use_shell = sys.platform == "win32"
             if mapfile:
                 subprocess.call(
diff --git a/tests/test_import_graph.py b/tests/test_import_graph.py
index 14e5f387a..b918d3b29 100644
--- a/tests/test_import_graph.py
+++ b/tests/test_import_graph.py
@@ -26,8 +26,8 @@ from pylint.lint import PyLinter
 
 
 @pytest.fixture
-def dest():
-    dest = "dependencies_graph.dot"
+def dest(request):
+    dest = request.param
     yield dest
     try:
         os.remove(dest)
@@ -36,13 +36,18 @@ def dest():
         pass
 
 
+POSSIBLE_DOT_FILENAMES = ["foo.dot", "foo.gv", "tests/regrtest_data/foo.dot"]
+
+
+@pytest.mark.parametrize("dest", POSSIBLE_DOT_FILENAMES, indirect=True)
 def test_dependencies_graph(dest):
+    """DOC files are correctly generated, and the graphname is the basename"""
     imports._dependencies_graph(dest, {"labas": ["hoho", "yep"], "hoho": ["yep"]})
     with open(dest) as stream:
         assert (
             stream.read().strip()
             == """
-digraph "dependencies_graph" {
+digraph "foo" {
 rankdir=LR
 charset="utf-8"
 URL="." node[shape="box"]
@@ -57,6 +62,13 @@ URL="." node[shape="box"]
         )
 
 
+@pytest.mark.parametrize("filename", ["graph.png", "graph"])
+def test_missing_graphviz(filename):
+    """Raises if graphviz is not installed, and defaults to png if no extension given"""
+    with pytest.raises(RuntimeError, match=r"Cannot generate `graph\.png`.*"):
+        imports._dependencies_graph(filename, {"a": ["b", "c"], "b": ["c"]})
+
+
 @pytest.fixture
 def linter():
     pylinter = PyLinter(reporter=testutils.GenericTestReporter())