Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file reloading in dmypy with --export-types #16359

Merged
merged 5 commits into from Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 40 additions & 8 deletions mypy/dmypy_server.py
Expand Up @@ -393,15 +393,21 @@ def cmd_recheck(
t1 = time.time()
manager = self.fine_grained_manager.manager
manager.log(f"fine-grained increment: cmd_recheck: {t1 - t0:.3f}s")
self.options.export_types = export_types
old_export_types = self.options.export_types
self.options.export_types = self.options.export_types or export_types
if not self.following_imports():
messages = self.fine_grained_increment(sources, remove, update)
messages = self.fine_grained_increment(
sources, remove, update, explicit_export_types=export_types
)
else:
assert remove is None and update is None
messages = self.fine_grained_increment_follow_imports(sources)
messages = self.fine_grained_increment_follow_imports(
sources, explicit_export_types=export_types
)
res = self.increment_output(messages, sources, is_tty, terminal_width)
self.flush_caches()
self.update_stats(res)
self.options.export_types = old_export_types
return res

def check(
Expand All @@ -412,17 +418,21 @@ def check(
If is_tty is True format the output nicely with colors and summary line
(unless disabled in self.options). Also pass the terminal_width to formatter.
"""
self.options.export_types = export_types
old_export_types = self.options.export_types
self.options.export_types = self.options.export_types or export_types
if not self.fine_grained_manager:
res = self.initialize_fine_grained(sources, is_tty, terminal_width)
else:
if not self.following_imports():
messages = self.fine_grained_increment(sources)
messages = self.fine_grained_increment(sources, explicit_export_types=export_types)
else:
messages = self.fine_grained_increment_follow_imports(sources)
messages = self.fine_grained_increment_follow_imports(
sources, explicit_export_types=export_types
)
res = self.increment_output(messages, sources, is_tty, terminal_width)
self.flush_caches()
self.update_stats(res)
self.options.export_types = old_export_types
return res

def flush_caches(self) -> None:
Expand Down Expand Up @@ -461,6 +471,7 @@ def initialize_fine_grained(
messages = result.errors
self.fine_grained_manager = FineGrainedBuildManager(result)

original_sources_len = len(sources)
if self.following_imports():
sources = find_all_sources_in_build(self.fine_grained_manager.graph, sources)
self.update_sources(sources)
Expand Down Expand Up @@ -525,14 +536,16 @@ def initialize_fine_grained(

__, n_notes, __ = count_stats(messages)
status = 1 if messages and n_notes < len(messages) else 0
messages = self.pretty_messages(messages, len(sources), is_tty, terminal_width)
# We use explicit sources length to match the logic in non-incremental mode.
messages = self.pretty_messages(messages, original_sources_len, is_tty, terminal_width)
return {"out": "".join(s + "\n" for s in messages), "err": "", "status": status}

def fine_grained_increment(
self,
sources: list[BuildSource],
remove: list[str] | None = None,
update: list[str] | None = None,
explicit_export_types: bool = False,
) -> list[str]:
"""Perform a fine-grained type checking increment.

Expand All @@ -543,6 +556,8 @@ def fine_grained_increment(
sources: sources passed on the command line
remove: paths of files that have been removed
update: paths of files that have been changed or created
explicit_export_types: --export-type was passed in a check command
(as opposite to being set in dmypy start)
"""
assert self.fine_grained_manager is not None
manager = self.fine_grained_manager.manager
Expand All @@ -557,6 +572,14 @@ def fine_grained_increment(
# Use the remove/update lists to update fswatcher.
# This avoids calling stat() for unchanged files.
changed, removed = self.update_changed(sources, remove or [], update or [])
if explicit_export_types:
# If --export-types is given, we need to force full re-checking of all
# explicitly passed files, since we need to visit each expression.
changed += [
(bs.module, bs.path)
for bs in sources
if bs.path and (bs.module, bs.path) not in changed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quadratic, and it could be slow if there are many files and many that are changed. Maybe construct a temporary set from changed and use it in the not in changed check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point, fixed now.

]
changed += self.find_added_suppressed(
self.fine_grained_manager.graph, set(), manager.search_paths
)
Expand All @@ -575,7 +598,9 @@ def fine_grained_increment(
self.previous_sources = sources
return messages

def fine_grained_increment_follow_imports(self, sources: list[BuildSource]) -> list[str]:
def fine_grained_increment_follow_imports(
self, sources: list[BuildSource], explicit_export_types: bool = False
) -> list[str]:
"""Like fine_grained_increment, but follow imports."""
t0 = time.time()

Expand All @@ -601,6 +626,13 @@ def fine_grained_increment_follow_imports(self, sources: list[BuildSource]) -> l
changed, new_files = self.find_reachable_changed_modules(
sources, graph, seen, changed_paths
)
if explicit_export_types:
# Same as in fine_grained_increment().
changed += [
(bs.module, bs.path)
for bs in sources
if bs.path and (bs.module, bs.path) not in changed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above. Maybe move this to a helper function to avoid code duplication?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to a helper function.

]
sources.extend(new_files)

# Process changes directly reachable from roots.
Expand Down
3 changes: 2 additions & 1 deletion mypy/test/testfinegrained.py
Expand Up @@ -149,6 +149,7 @@ def get_options(self, source: str, testcase: DataDrivenTestCase, build_cache: bo
options.use_fine_grained_cache = self.use_cache and not build_cache
options.cache_fine_grained = self.use_cache
options.local_partial_types = True
options.export_types = True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting the daemon with this option set to True is cleaner for inspect unit tests, I made this explicit now (so it will not slow down other fine grained tests).

options.enable_incomplete_feature = [TYPE_VAR_TUPLE, UNPACK]
# Treat empty bodies safely for these test cases.
options.allow_empty_bodies = not testcase.name.endswith("_no_empty")
Expand All @@ -164,7 +165,7 @@ def get_options(self, source: str, testcase: DataDrivenTestCase, build_cache: bo
return options

def run_check(self, server: Server, sources: list[BuildSource]) -> list[str]:
response = server.check(sources, export_types=True, is_tty=False, terminal_width=-1)
response = server.check(sources, export_types=False, is_tty=False, terminal_width=-1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm why do we have export_types=False here, but true above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing it explicitly on each run will force reloading files. Putting it in options from the start is IMO cleaner.

out = response["out"] or response["err"]
assert isinstance(out, str)
return out.splitlines()
Expand Down
27 changes: 27 additions & 0 deletions test-data/unit/daemon.test
Expand Up @@ -360,6 +360,33 @@ def bar() -> None:
x = foo('abc') # type: str
foo(arg='xyz')

[case testDaemonInspectCheck]
$ dmypy start
Daemon started
$ dmypy check foo.py
Success: no issues found in 1 source file
$ dmypy check foo.py --export-types
Success: no issues found in 1 source file
$ dmypy inspect foo.py:1:1
"int"
[file foo.py]
x = 1

[case testDaemonInspectRun]
$ dmypy run test1.py
Daemon started
Success: no issues found in 1 source file
$ dmypy run test2.py
Success: no issues found in 1 source file
$ dmypy run test1.py --export-types
Success: no issues found in 1 source file
$ dmypy inspect test1.py:1:1
"int"
[file test1.py]
a: int
[file test2.py]
a: str

[case testDaemonGetType]
$ dmypy start --log-file log.txt -- --follow-imports=error --no-error-summary --python-version 3.8
Daemon started
Expand Down