Repository: python/mypy
Pull request: Prevent dmypy from restarting if the configuration contains globs.
If a configuration contains globs, then dmypy incorrectly detected
that the configration changed even though it did not.
This caused the daemon to always restart if a configuration contained globs.

Fixes #7576.

Problem statement:
dmypy run keeps rerunning when using certain glob styles
With a configfile like this:

```
...
[mypy-abc.*]
...
```

dmypy run works fine, and only reruns when the configfile changes.

However, when I change the module configuration to:
```
...
[mypy-abc.*.abc]
...
```

dmypy keeps rerunning, saying:
```
Restarting: configuration changed
Daemon stopped
Daemon started
```

even without further changes to the config file.

I tried to track the error down, and changed the error message in`dmypy_server.py` to:
```
    def cmd_run(self, version: str, args: Sequence[str],
                is_tty: bool, terminal_width: int) -> Dict[str, object]:
        """Check a list of files, triggering a restart if needed."""
            ...
            # Signal that we need to restart if the options have changed
            if self.options_snapshot != options.snapshot():
                return {'restart': 'configuration changed' + str(self.options_snapshot) + '===' + str(options.snapshot())}
            ...
```

Comparing `self.options_snapshot` with `options.snapshot()`, I can see that with the second config file, `self.options_snapshot` contains this:
```
'glob_options':
[('abc.*.abc',
re.compile('abc(\\..*)?\\.abc\\Z'))],
```
while `options.snapshot()` contains this:
```
'glob_options':
[],
```

So I believe something is wrong in this case...

Hints from issue discussion:
Thanks for report and the detailed analysis!

Commit message: Make Options.per_module_cache/glob_options "private" and don't include private attributes in snapshot.

Patch to review:
diff --git a/mypy/options.py b/mypy/options.py
index beaedbc4277d..38072b821d15 100644
--- a/mypy/options.py
+++ b/mypy/options.py
@@ -62,7 +62,7 @@ class Options:
 
     def __init__(self) -> None:
         # Cache for clone_for_module()
-        self.per_module_cache = None  # type: Optional[Dict[str, Options]]
+        self._per_module_cache = None  # type: Optional[Dict[str, Options]]
 
         # -- build options --
         self.build_type = BuildType.STANDARD
@@ -222,7 +222,7 @@ def __init__(self) -> None:
 
         # Per-module options (raw)
         self.per_module_options = OrderedDict()  # type: OrderedDict[str, Dict[str, object]]
-        self.glob_options = []  # type: List[Tuple[str, Pattern[str]]]
+        self._glob_options = []  # type: List[Tuple[str, Pattern[str]]]
         self.unused_configs = set()  # type: Set[str]
 
         # -- development options --
@@ -280,7 +280,8 @@ def snapshot(self) -> object:
         for k in get_class_descriptors(Options):
             if hasattr(self, k) and k != "new_semantic_analyzer":
                 d[k] = getattr(self, k)
-        del d['per_module_cache']
+        # Remove private attributes from snapshot
+        d = {k: v for k, v in d.items() if not k.startswith('_')}
         return d
 
     def __repr__(self) -> str:
@@ -295,7 +296,7 @@ def apply_changes(self, changes: Dict[str, object]) -> 'Options':
         return new_options
 
     def build_per_module_cache(self) -> None:
-        self.per_module_cache = {}
+        self._per_module_cache = {}
 
         # Config precedence is as follows:
         #  1. Concrete section names: foo.bar.baz
@@ -320,7 +321,7 @@ def build_per_module_cache(self) -> None:
         concrete = [k for k in structured_keys if not k.endswith('.*')]
 
         for glob in unstructured_glob_keys:
-            self.glob_options.append((glob, self.compile_glob(glob)))
+            self._glob_options.append((glob, self.compile_glob(glob)))
 
         # We (for ease of implementation) treat unstructured glob
         # sections as used if any real modules use them or if any
@@ -333,7 +334,7 @@ def build_per_module_cache(self) -> None:
             # on inheriting from parent configs.
             options = self.clone_for_module(key)
             # And then update it with its per-module options.
-            self.per_module_cache[key] = options.apply_changes(self.per_module_options[key])
+            self._per_module_cache[key] = options.apply_changes(self.per_module_options[key])
 
         # Add the more structured sections into unused configs, since
         # they only count as used if actually used by a real module.
@@ -345,14 +346,14 @@ def clone_for_module(self, module: str) -> 'Options':
         NOTE: Once this method is called all Options objects should be
         considered read-only, else the caching might be incorrect.
         """
-        if self.per_module_cache is None:
+        if self._per_module_cache is None:
             self.build_per_module_cache()
-        assert self.per_module_cache is not None
+        assert self._per_module_cache is not None
 
         # If the module just directly has a config entry, use it.
-        if module in self.per_module_cache:
+        if module in self._per_module_cache:
             self.unused_configs.discard(module)
-            return self.per_module_cache[module]
+            return self._per_module_cache[module]
 
         # If not, search for glob paths at all the parents. So if we are looking for
         # options for foo.bar.baz, we search foo.bar.baz.*, foo.bar.*, foo.*,
@@ -363,15 +364,15 @@ def clone_for_module(self, module: str) -> 'Options':
         path = module.split('.')
         for i in range(len(path), 0, -1):
             key = '.'.join(path[:i] + ['*'])
-            if key in self.per_module_cache:
+            if key in self._per_module_cache:
                 self.unused_configs.discard(key)
-                options = self.per_module_cache[key]
+                options = self._per_module_cache[key]
                 break
 
         # OK and *now* we need to look for unstructured glob matches.
         # We only do this for concrete modules, not structured wildcards.
         if not module.endswith('.*'):
-            for key, pattern in self.glob_options:
+            for key, pattern in self._glob_options:
                 if pattern.match(module):
                     self.unused_configs.discard(key)
                     options = options.apply_changes(self.per_module_options[key])
diff --git a/test-data/unit/daemon.test b/test-data/unit/daemon.test
index 99e1ee632e37..ccb5e9f9381e 100644
--- a/test-data/unit/daemon.test
+++ b/test-data/unit/daemon.test
@@ -99,6 +99,37 @@ from mypy.plugin import Plugin
 class Dummy(Plugin): pass
 def plugin(version): return Dummy
 
+[case testDaemonRunRestartGlobs]
+-- Ensure dmypy is not restarted if the configuration doesn't change and it contains globs
+-- Note: Backslash path separator in output is replaced with forward slash so the same test succeeds on Windows as well
+$ dmypy run -- foo --follow-imports=error --python-version=3.6 | {python} -c 'import sys; sys.stdout.write(sys.stdin.read().replace("\\", "/"))'
+Daemon started
+foo/lol.py:1: error: Name 'fail' is not defined
+Found 1 error in 1 file (checked 3 source files)
+$ dmypy run -- foo --follow-imports=error --python-version=3.6 | {python} -c 'import sys; sys.stdout.write(sys.stdin.read().replace("\\", "/"))'
+foo/lol.py:1: error: Name 'fail' is not defined
+Found 1 error in 1 file (checked 3 source files)
+$ {python} -c "print('[mypy]')" >mypy.ini
+$ {python} -c "print('ignore_errors=True')" >>mypy.ini
+$ dmypy run -- foo --follow-imports=error --python-version=3.6 | {python} -c 'import sys; sys.stdout.write(sys.stdin.read().replace("\\", "/"))'
+Restarting: configuration changed
+Daemon stopped
+Daemon started
+Success: no issues found in 3 source files
+$ dmypy stop
+Daemon stopped
+[file mypy.ini]
+\[mypy]
+ignore_errors = True
+\[mypy-*.lol]
+ignore_errors = False
+
+[file foo/__init__.py]
+[file foo/lol.py]
+fail
+[file foo/ok.py]
+a: int = 1
+
 [case testDaemonStatusKillRestartRecheck]
 $ dmypy status
 No status file found


Code review comments:
diff_hunk: @@ -99,6 +99,37 @@ from mypy.plugin import Plugin
 class Dummy(Plugin): pass
 def plugin(version): return Dummy
 
+[case testDaemonRunRestartGlobs]
+-- Ensure dmypy is not restarted if the configuration doesn't change and it contains globs
+-- Note: Backslash path separator in output is replaced with forward slash so the same test succeeds on Windows as well
+$ dmypy run -- foo --follow-imports=error --python-version=3.6 | {python} -c 'import sys; sys.stdout.write(sys.stdin.read().replace("\\", "/"))'
path: test-data/unit/daemon.test
text: @user1:
`'` isn't a quote character in windows command line parsing, I think.

I would ditch the pipe stuff, though and do the slash normalization in the test case driver. We have a `normalize_error_messages` function that fixes path slashes in test output that we use in most of our test drivers but don't seem to call in `testdaemon`.