Add roadmap feature to js-dep-visualizer.

The js-dep-visualizer tool now attempts to find a set of edges
to remove from a call graph that would reduce it to having only
trivial mutual dependencies, and it produces a roadmap of the
changes that need to happen.

If the tool can't reduce the graph all the way, it still produces
a DOT file that can be visualized.

This fix also has some significant code cleanup.

tools/js-dep-visualizer.py

@@ -9,143 +9,266 @@ from __future__ import print_function
 import os
 import re
 import sys
+from collections import defaultdict
+
+from typing import Any, DefaultDict, Dict, List, Set, Tuple
+Edge = Tuple[str, str]
+EdgeSet = Set[Edge]
+Method = str
+MethodDict = DefaultDict[Edge, List[Method]]
 
-from typing import Any, Dict, List
 
 TOOLS_DIR = os.path.abspath(os.path.dirname(__file__))
 ROOT_DIR = os.path.dirname(TOOLS_DIR)
 sys.path.insert(0, ROOT_DIR)
-from tools.lib.graph import Graph, make_dot_file
+from tools.lib.graph import (
+    Graph,
+    make_dot_file,
+    best_edge_to_remove,
+)
 
 JS_FILES_DIR = os.path.join(ROOT_DIR, 'static/js')
 OUTPUT_FILE_PATH = os.path.relpath(os.path.join(ROOT_DIR, 'var/zulip-deps.dot'))
 
-names = set()
+def get_js_edges():
-modules = [] # type: List[Dict[str, Any]]
+    # type: () -> Tuple[EdgeSet, MethodDict]
-for js_file in os.listdir(JS_FILES_DIR):
+    names = set()
-    if not js_file.endswith('.js'):
+    modules = [] # type: List[Dict[str, Any]]
-        continue
+    for js_file in os.listdir(JS_FILES_DIR):
-    name = js_file[:-3] # remove .js
+        if not js_file.endswith('.js'):
-    path = os.path.join(JS_FILES_DIR, js_file)
+            continue
-    names.add(name)
+        name = js_file[:-3] # remove .js
-    modules.append(dict(
+        path = os.path.join(JS_FILES_DIR, js_file)
-        name=name,
+        names.add(name)
-        path=path,
+        modules.append(dict(
-        regex=re.compile('[^_]{}\.\w+\('.format(name))
+            name=name,
-    ))
+            path=path,
+            regex=re.compile('[^_]{}\.\w+\('.format(name))
+        ))
 
-COMMENT_REGEX = re.compile('\s+//')
+    comment_regex = re.compile('\s+//')
-REGEX = re.compile('[^_](\w+)\.\w+\(')
+    call_regex = re.compile('[^_](\w+\.\w+)\(')
 
-tuples = set()
+    methods = defaultdict(list) # type: DefaultDict[Edge, List[Method]]
-for module in modules:
+    edges = set()
-    parent = module['name']
+    for module in modules:
+        parent = module['name']
 
-    with open(module['path']) as f:
+        with open(module['path']) as f:
-        for line in f:
+            for line in f:
-            if COMMENT_REGEX.match(line):
+                if comment_regex.match(line):
-                continue
+                    continue
-            if 'subs.forEach' in line:
+                if 'subs.forEach' in line:
-                continue
+                    continue
-            m = REGEX.search(line)
+                m = call_regex.search(line)
-            if not m:
+                if not m:
-                continue
+                    continue
-            for child in m.groups():
+                for g in m.groups():
-                if (child in names) and (child != parent):
+                    child, method = g.split('.')
+                    if (child not in names):
+                        continue
+                    if child == parent:
+                        continue
                     tup = (parent, child)
-                    tuples.add(tup)
+                    edges.add(tup)
+                    methods[tup].append(method)
+    return edges, methods
 
-IGNORE_TUPLES = [
+def find_edges_to_remove(graph, methods):
-    # We ignore the following tuples to de-clutter the graph, since there is a
+    # type: (Graph, MethodDict) -> Tuple[Graph, List[Edge]]
-    # pretty clear roadmap on how to break the dependencies.  You can comment
+    EXEMPT_EDGES = [
-    # these out to see what the "real" situation looks like now, and if you do
+        # These are sensible dependencies, so don't cut them.
-    # the work of breaking the dependency, you can remove it.
+        ('rows', 'message_store'),
+        ('filter', 'stream_data'),
+        ('server_events', 'user_events'),
+        ('compose_fade', 'stream_data'),
+        ('narrow', 'message_list'),
+        ('stream_list', 'topic_list',),
+        ('subs', 'stream_muting'),
+        ('hashchange', 'settings'),
+        ('tutorial', 'narrow'),
+        ('activity', 'resize'),
+        ('hashchange', 'drafts'),
+        ('compose', 'echo'),
+        ('compose', 'resize'),
+        ('compose', 'unread_ops'),
+        ('compose', 'drafts'),
+        ('echo', 'message_edit'),
+        ('echo', 'stream_list'),
+        ('hashchange', 'narrow'),
+        ('hashchange', 'subs'),
+        ('message_edit', 'echo'),
+        ('popovers', 'message_edit'),
+        ('unread_ui', 'activity'),
+        ('message_fetch', 'message_util'),
+        ('message_fetch', 'resize'),
+        ('message_util', 'resize'),
+        ('notifications', 'tutorial'),
+        ('message_util', 'unread_ui'),
+        ('muting_ui', 'stream_list'),
+        ('muting_ui', 'unread_ui'),
+        ('stream_popover', 'subs'),
+        ('stream_popover', 'muting_ui'),
+        ('narrow', 'message_fetch'),
+        ('narrow', 'message_util'),
+        ('narrow', 'navigate'),
+        ('unread_ops', 'unread_ui'),
+        ('narrow', 'unread_ops'),
+        ('navigate', 'unread_ops'),
+        ('pm_list', 'unread_ui'),
+        ('stream_list', 'unread_ui'),
+        ('popovers', 'compose'),
+        ('popovers', 'muting_ui'),
+        ('popovers', 'narrow'),
+        ('popovers', 'resize'),
+        ('pm_list', 'resize'),
+        ('notifications', 'navigate'),
+        ('compose', 'socket'),
+        ('referral', 'resize'),
+        ('stream_muting', 'message_util'),
+        ('subs', 'stream_list'),
+        ('ui', 'message_fetch'),
+        ('ui', 'unread_ops'),
+        ('condense', 'message_viewport'),
+    ] # type: List[Edge]
 
-    ('echo', 'message_events'), # do something slimy here
+    def is_exempt(edge):
-    ('echo', 'ui'),
+        # type: (Tuple[str, str]) -> bool
+        parent, child = edge
+        if edge == ('server_events', 'reload'):
+            return False
+        if parent in ['server_events', 'user_events', 'stream_events',
+                      'message_events', 'reload']:
+            return True
+        if child == 'rows':
+            return True
+        return edge in EXEMPT_EDGES
 
-    ('stream_data', 'narrow'), # split out narrow.by_foo functions
+    APPROVED_CUTS = [
-    ('activity', 'narrow'),
+        ('echo', 'message_events'),
+        ('resize', 'navigate'),
+        ('narrow', 'search'),
+        ('subs', 'stream_events'),
+        ('stream_color', 'tab_bar'),
+        ('stream_color', 'subs'),
+        ('stream_data', 'narrow'),
+        ('unread', 'narrow'),
+        ('composebox_typeahead', 'compose'),
+        ('message_list', 'message_edit'),
+        ('message_edit', 'compose'),
+        ('message_store', 'compose'),
+        ('settings', 'muting_ui'),
+        ('message_fetch', 'tutorial'),
+        ('settings', 'subs'),
+        ('activity', 'narrow'),
+        ('compose', 'subs'),
+        ('drafts', 'compose'),
+        ('drafts', 'echo'),
+        ('echo', 'compose'),
+        ('echo', 'narrow'),
+        ('echo', 'pm_list'),
+        ('echo', 'ui'),
+        ('message_fetch', 'activity'),
+        ('message_fetch', 'narrow'),
+        ('message_fetch', 'pm_list'),
+        ('message_fetch', 'stream_list'),
+        ('message_fetch', 'ui'),
+        ('narrow', 'ui'),
+        ('message_util', 'compose'),
+        ('subs', 'compose'),
+        ('narrow', 'hashchange'),
+        ('subs', 'hashchange'),
+        ('navigate', 'narrow'),
+        ('navigate', 'stream_list'),
+        ('pm_list', 'narrow'),
+        ('pm_list', 'stream_popover'),
+        ('popovers', 'stream_popover'),
+        ('topic_list', 'stream_popover'),
+        ('topic_list', 'narrow'),
+        ('stream_list', 'narrow'),
+        ('stream_list', 'pm_list'),
+        ('stream_list', 'unread_ops'),
+        ('notifications', 'ui'),
+        ('notifications', 'narrow'),
+        ('notifications', 'unread_ops'),
+        ('typing', 'narrow'),
+        ('message_events', 'compose'),
+        ('stream_muting', 'stream_list'),
+        ('subs', 'narrow'),
+        ('unread_ui', 'pm_list'),
+        ('unread_ui', 'stream_list'),
+    ]
 
-    ('subs', 'narrow'), # data functions
+    def cut_is_legal(edge):
-    ('subs', 'compose'), # data functions
+        # type: (Edge) -> bool
+        parent, child = edge
+        if child in ['reload', 'popovers', 'modals', 'notifications', 'server_events']:
+            return True
+        return edge in APPROVED_CUTS
 
-    ('narrow', 'ui'), # just three functions
+    graph.remove_exterior_nodes()
+    removed_edges = list()
+    print()
+    while graph.num_edges() > 0:
+        edge = best_edge_to_remove(graph, is_exempt)
+        if edge is None:
+            print('we may not be allowing edge cuts!!!')
+            break
+        if cut_is_legal(edge):
+            graph = graph.minus_edge(edge)
+            graph.remove_exterior_nodes()
+            removed_edges.append(edge)
+        else:
+            for removed_edge in removed_edges:
+                print(removed_edge)
+            print()
+            edge_str = str(edge) + ','
+            print(edge_str)
+            for method in methods[edge]:
+                print('    ' + method)
+            break
 
-    ('stream_data', 'stream_color'), # split out stream_color data/UI
+    return graph, removed_edges
-    ('stream_color', 'tab_bar'), # only one call
-    ('stream_color', 'subs'), # only one call
 
-    ('subs', 'stream_events'), # see TODOs related to mark_{un,}subscribed
+def report_roadmap(edges, methods):
+    # type: (List[Edge], MethodDict) -> None
+    child_modules = {child for parent, child in edges}
+    module_methods = defaultdict(set) # type: DefaultDict[str, Set[str]]
+    callers = defaultdict(set) # type: DefaultDict[Tuple[str, str], Set[str]]
+    for parent, child in edges:
+        for method in methods[(parent, child)]:
+            module_methods[child].add(method)
+            callers[(child, method)].add(parent)
 
-    ('subs', 'hashchange'), # modal stuff
+    for child in sorted(child_modules):
+        print(child + '.js')
+        for method in module_methods[child]:
+            print('    ' + child + '.' + method)
+            for caller in sorted(callers[(child, method)]):
+                print('        ' + caller + '.js')
+            print()
+        print()
 
-    ('message_store', 'compose'), # split out compose_data
+def produce_partial_output(graph):
+    # type: (Graph) -> None
+    print(graph.num_edges())
+    buffer = make_dot_file(graph)
 
-    ('search', 'search_suggestion'), # move handler into search_suggestion
+    graph.report()
+    with open(OUTPUT_FILE_PATH, 'w') as f:
+        f.write(buffer)
+    print()
+    print('see dot file here: {}'.format(OUTPUT_FILE_PATH))
 
-    ('unread', 'narrow'), # create narrow_state.js
+def run():
+    # type: () -> None
+    edges, methods = get_js_edges()
+    graph = Graph(edges)
+    graph, removed_edges = find_edges_to_remove(graph, methods)
+    if graph.num_edges() == 0:
+        report_roadmap(removed_edges, methods)
+    else:
+        produce_partial_output(graph)
 
-    ('navigate', 'stream_list'), # move cycle_stream into stream_list.js
+if __name__ == '__main__':
-
+    run()
-    # This one is kind of tricky, but basically we want to split all the basic
-    # code out of both of these modules that's essentially just string manipulation.
-    ('narrow', 'hashchange'),
-
-    ('composebox_typeahead', 'compose'), # introduce compose_state.js
-
-    # This one might require some work, but the idea is to split out something
-    # like narrow_state.js.
-    ('pm_list', 'narrow'),
-
-    ('settings', 'subs'), # not much to fix, can call stream_data directly, maybe
-
-    ('modals', 'subs'), # add some kind of onClose mechanism in new modals.open
-
-    ('channel', 'reload'), # just one call to fix somehow
-    ('compose', 'reload'), # use channel stuff more directly?
-
-    ('settings', 'muting_ui'), # inline call or split out muting_settings.js
-
-    ('resize', 'navigate'), # split out scroll.js
-    ('resize', 'popovers'), # only three interactions
-
-]
-
-for tup in IGNORE_TUPLES:
-    try:
-        tuples.remove(tup)
-    except KeyError:
-        print('''
-            {} no longer needs to be ignored.  Help us celebrate
-            by removing it from IGNORE_TUPLES!
-        '''.format(tup))
-        sys.exit(1)
-
-
-# print(tuples)
-graph = Graph(*tuples)
-ignore_modules = [
-    'blueslip',
-    'message_edit',
-    'message_util',
-    'modals',
-    'notifications',
-    'popovers',
-    'server_events',
-    'stream_popover',
-    'topic_list',
-    'tutorial',
-    'unread_ops',
-    'rows', # message_store
-]
-for node in ignore_modules:
-    graph.remove(node)
-graph.remove_exterior_nodes()
-graph.report()
-buffer = make_dot_file(graph)
-
-with open(OUTPUT_FILE_PATH, 'w') as f:
-    f.write(buffer)
-print()
-print('see dot file here: {}'.format(OUTPUT_FILE_PATH))

tools/lib/graph.py

@@ -3,11 +3,14 @@ from __future__ import print_function
 
 from collections import defaultdict
 
-from typing import DefaultDict, List, Set, Tuple
+from typing import Callable, DefaultDict, Iterator, List, Set, Tuple
+
+Edge = Tuple[str, str]
+EdgeSet = Set[Edge]
 
 class Graph(object):
-    def __init__(self, *tuples):
+    def __init__(self, tuples):
-        # type: (Tuple[str, str]) -> None
+        # type: (EdgeSet) -> None
         self.children = defaultdict(list) # type: DefaultDict[str, List[str]]
         self.parents = defaultdict(list) # type: DefaultDict[str, List[str]]
         self.nodes = set() # type: Set[str]
@@ -18,6 +21,28 @@ class Graph(object):
             self.nodes.add(parent)
             self.nodes.add(child)
 
+    def copy(self):
+        # type: () -> Graph
+        return Graph(self.edges())
+
+    def num_edges(self):
+        # type: () -> int
+        return len(self.edges())
+
+    def minus_edge(self, edge):
+        # type: (Edge) -> Graph
+        edges = self.edges().copy()
+        edges.remove(edge)
+        return Graph(edges)
+
+    def edges(self):
+        # type: () -> EdgeSet
+        s = set()
+        for parent in self.nodes:
+            for child in self.children[parent]:
+                s.add((parent, child))
+        return s
+
     def remove_exterior_nodes(self):
         # type: () -> None
         still_work_to_do = True
@@ -61,6 +86,30 @@ class Graph(object):
         for tup in tups:
             print(tup)
 
+def best_edge_to_remove(orig_graph, is_exempt):
+    # type: (Graph, Callable[[Edge], bool]) -> Edge
+    # expects an already reduced graph as input
+
+    orig_edges = orig_graph.edges()
+
+    def get_choices():
+        # type: () -> Iterator[Tuple[int, Edge]]
+        for edge in orig_edges:
+            if is_exempt(edge):
+                continue
+            graph = orig_graph.minus_edge(edge)
+            graph.remove_exterior_nodes()
+            size = graph.num_edges()
+            yield (size, edge)
+
+    choices = list(get_choices())
+    if not choices:
+        return None
+    min_size, best_edge = min(choices)
+    if min_size >= orig_graph.num_edges():
+        raise Exception('no edges work here')
+    return best_edge
+
 def make_dot_file(graph):
     # type: (Graph) -> str
     buffer = 'digraph G {\n'
@@ -73,7 +122,7 @@ def make_dot_file(graph):
 
 def test():
     # type: () -> None
-    graph = Graph(
+    graph = Graph(set([
         ('x', 'a'),
         ('a', 'b'),
         ('b', 'c'),
@@ -82,7 +131,7 @@ def test():
         ('d', 'e'),
         ('e', 'f'),
         ('e', 'g'),
-    )
+    ]))
     graph.remove_exterior_nodes()
 
     s = make_dot_file(graph)