Update fine-grained tests to prep for --namespace-packages support

[nipunn1313]

These tests were deleting package files, but not the empty
directory, causing those directories to be interpreted as
namespace packages in #9636

Test Plan

Tried these out on top of #9636 and confirmed they all work. A couple changed error messages - so I needed to add flags for --namespace-packages, but figured that's the future anyway, so this is a good thing.

[hauntsaninja]

Thanks! Got pretty thrown off by Import of "p.a" ignored. I just didn't realise that finegrained tests set follow_imports = error by default. Feels like there's a shortage of follow_imports=normal tests too...

[hauntsaninja]

Actually, maybe it's more in the spirit of the testDeletePackage5 and testDeletePackage6 to change the commands, since they are trying to pass p.a on the command line. E.g., maybe we pass --explicit-package-bases or switch to specifying -m instead of files.

[nipunn1313]

Actually, maybe it's more in the spirit of the testDeletePackage5 and testDeletePackage6 to change the commands, since they are trying to pass p.a on the command line. E.g., maybe we pass --explicit-package-bases or switch to specifying -m instead of files.

We still need to provide --namespace-packages to make sure that these tests continue to work after #9636.

I can change them to use -m p instead of passing all the files individually, though that does seem unrelated to this commit. I can add as a separate commit. It won't really change the output.

[hauntsaninja]

Yes, agreed on passing --namespace-packages. My concern was the error messages that look like:

main:2: error: Import of "p.a" ignored
main:2: note: (Using --follow-imports=error, module not passed on command line)

since when the command is e.g. mypy main p/a.py p/__init__.py we are intending to pass p.a on the command line.

To that end, I was suggesting something like:

 [case testDeletePackage5]
-# cmd1: mypy main p/a.py p/__init__.py
-# cmd2: mypy main p/a.py
-# cmd3: mypy main
+# flags: --namespace-packages
+# cmd1: mypy -m main -m p.a -m p.__init__
+# cmd2: mypy -m main -m p.a
+# cmd3: mypy -m main

 [case testDeletePackage6]
-# cmd1: mypy p/a.py p/b.py p/__init__.py
-# cmd2: mypy p/a.py p/b.py
-# cmd3: mypy p/a.py p/b.py
+# flags: --namespace-packages
+# cmd1: mypy -m p.a -m p.b -m p.__init__
+# cmd2: mypy -m p.a -m p.b
+# cmd3: mypy -m p.a -m p.b

with the hope of changing that middle error message. It looks to me like even with that change we still ignore p.a, which is concerning.

I might be missing something obvious though :-)

[nipunn1313]

I see your point! With namespace packages, p should be a namespace package and p.a should be a valid package within the namespace package. In that case, this seems like a bug.

[hauntsaninja]

Okay, so this is the diff I'm playing around with now. That is, I think at least some of the problem is that the test fixtures were passing in a as the module name for p/a.py after __init__.py was deleted, instead of p.a. These changes make it so that I see the build sources I expect over here https://github.com/python/mypy/blob/0860ca7204f716c18dfa014226a2336b71a9841d/mypy/test/testfinegrained.py#L297.

diff --git a/mypy/test/testfinegrained.py b/mypy/test/testfinegrained.py
index ed99b4aea..182212823 100644
--- a/mypy/test/testfinegrained.py
+++ b/mypy/test/testfinegrained.py
@@ -84,6 +84,7 @@ class FineGrainedSuite(DataSuite):
 
         options = self.get_options(main_src, testcase, build_cache=False)
         build_options = self.get_options(main_src, testcase, build_cache=True)
+        options.mypy_path.append(test_temp_dir)
         server = Server(options, DEFAULT_STATUS_FILE)
 
         num_regular_incremental_steps = self.get_build_steps(main_src)
diff --git a/test-data/unit/fine-grained-modules.test b/test-data/unit/fine-grained-modules.test
index 50da301aa..5f8314b93 100644
--- a/test-data/unit/fine-grained-modules.test
+++ b/test-data/unit/fine-grained-modules.test
@@ -902,7 +902,7 @@ main:2: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missin
 main:2: error: Cannot find implementation or library stub for module named "p"
 
 [case testDeletePackage5]
-# flags: --namespace-packages
+# flags: --namespace-packages --explicit-package-bases
 # cmd1: mypy -m main -m p.a -m p.__init__
 # cmd2: mypy -m main -m p.a
 # cmd3: mypy -m main
@@ -917,8 +917,7 @@ def f(x: str) -> None: pass
 [out]
 main:7: error: Argument 1 to "f" has incompatible type "int"; expected "str"
 ==
-main:6: error: Import of "p.a" ignored
-main:6: note: (Using --follow-imports=error, module not passed on command line)
+main:7: error: Argument 1 to "f" has incompatible type "int"; expected "str"
 ==
 main:6: error: Cannot find implementation or library stub for module named "p.a"
 main:6: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

Unfortunately, mypy now silently passes once we delete p/__init__.py, it silently infers p.a.f as Any and we get no errors, instead of the incompatible arg one we want.

[nipunn1313]

Ok - so I confirmed that there's a bug in mypy daemon when using --namespace-packages

Repro steps:

Initial conditions:

p/__init__.py
p/a.py

1. Run mypy daemon
2. Then we delete p/__init__.py
3. Run mypy daemon

We expect p to convert into a namespace package and for p.a to continue to exist.
However, mypy daemon update logic (in update.py) does not handle this.

Note that if the initial conditions are

p/a.py

Then it is handled! It has to do with the update logic in mypy daemon.

I spent several hours trying to understand update.py and fix this bug and really struggled.
My proposal is that we file a separate issue for this conversion-to-ns-package bug in mypy-daemon, but don't block #9636 on it, since the condition is rare and there is a workaround (reset the state of the daemon). I know that achieving correctness on highly stateful incremental systems like mypy-daemon is incredibly difficult. I don't really view this as a regression, since it only affects those using namespace packages (where the issue preexisted).

[nipunn1313]

I filed #11322 to convert these in the future.

[nipunn1313]

Updated these to pass --no-namespace-packages on the three tests that trip over the bug. Filed #11322 for the new bug. My proposal is to move forward with this diff - and not block #9636 on #11322. We can focus on the more important blockers instead.

[nipunn1313]

Hiya friends (@hauntsaninja)! Wanted to bump this one.
No rush - just wanted to make sure this didn't get lost.

[nipunn1313]

Hiya again! (@hauntsaninja)! Wanted to bump this one.
No rush - just wanted to make sure this didn't get lost.

[hauntsaninja]

Thanks, I think I left this dangling in the hope that I could quickly fix #11322. Clearly that hasn't happened in the last month, so merge it is :-) Thanks for pushing on this!

[hauntsaninja]

Also merged master into #9636