mypy: further work on filling in missing types #15057

eli-schwartz · 2025-09-28T19:59:05Z

-Found 182 errors in 8 files (checked 9 source files)
+Found 95 errors in 8 files (checked 9 source files)

With some interesting prep work too.

Code change: vs backend: don't rely on implicit re.Match truthiness

This function is annotated as bool, but actaully returns the raw Optional[Match]. Every use of it is in terms of `if func()`. Tweak it to return whether a match *was* found.

"None" typo from commit 23818fc. Nonsense from me in commit d0cbda9 -- how can `ofile: str` have .write() str/File confusion from initial implementation all the way back in commit 36bf53b.

All we need is the latter, and avoiding file I/O types from pathlib is good.

Which led to a fully typed file having, surprise, an annotation of `str` for a function that can only take list[FileOrString], making it "impossible" to then use from correctly in-progress typed code. typing.Sequence is an abomination that must be destroyed. In this case we can sorta bite the bullet and expand out the covariance as a Union[] of all possible lists we want. There are only two of them.

``` -Found 182 errors in 8 files (checked 9 source files) +Found 95 errors in 8 files (checked 9 source files) ```

dcbaker

I haven't fully looked through patch 5 yet, but I think some of my suggestions to earlier patches would make some of it unnecessary.

Patches 1 and 3 are r-b, go ahead and push them if you like.

Patch 2 I'm fine with the bits that are not about switching to File landing, but I think the switch from list[str] to list[File] are wrong.

Patch 4 I think you should replace with the one from my series :)

dcbaker · 2025-10-01T18:47:35Z

mesonbuild/backend/vs2010backend.py

 # own duplicate full set of defines/includes/opts intellisense fields.  All of which helps keep the vcxproj file
 # size down.
-def get_primary_source_lang(target_sources: T.List[File], custom_sources: T.List[str]) -> T.Optional[str]:
+def get_primary_source_lang(target_sources: T.List[File], custom_sources: T.List[File]) -> T.Optional[str]:


In my series I found this was correct as-is, this ultimately comes from generate_genlist_for_target, which does output a list[str]

dcbaker · 2025-10-01T18:48:45Z

mesonbuild/utils/universal.py



-def classify_unity_sources(compilers: T.Iterable['Compiler'], sources: T.Sequence['FileOrString']) -> T.Dict['Compiler', T.List['FileOrString']]:
+def classify_unity_sources(compilers: T.Iterable['Compiler'], sources: T.Union[T.List['FileOrString'], T.List[File]]) -> T.Dict['Compiler', T.List['FileOrString']]:


Can I interest you in something much better: 3ca3991?

This has the advantage of knowing that if you give it a list[str] you get list[str], etc. Using this makes a lot of the Union work in your next patch unnecessary, since mypy can know up front that it's passing a non-union type.

It is funny that you should say that, because I was tearing my hair out about the same thing. And I wrote effectively the same patch you did,.after I'd submitted the PR itself.

Unfortunately this doesn't work for all cases, only for ones with a transformed return values. :( So we still need some unrolled covariances written as unions, I think.

I won't have time to commit more until probably Sunday, but I do agree that a generic works well here.

(or maybe it was other uncommitted cases of the same issue? Not sure, brain fried and can't check. :D)

Now that I look again, your patch reiterates the exact issue that I was trying to fix. If you had correctly typed the function using any method, whether expanding covariance out via Union or via generics, you would know you got it correct -- because it would reveal an existing error in backend.py

It feels to me like you didn't actually read my commit message. :(

Uhhh, I don't understand at all. With generics when you pass a list of one thing, say list[File], you get back a list[File], as such:

x: list[File] = [...] classified: T.Dict[Compiler, T.List[file]] = classify_unity_sources(x) for lang, files in classified: for f in files: if f.is_built: ... else: ...

But with the union it's forced to be List[File | str], even though you know, up front, that you never will get a str. So now, you have to write extra code to deal with the fact that mypy thinks you will have a type you do not have. That's straight up worse:

x = list[File] classified: T.Dict[Compiler, list[File | str]] = classify_unity_sources(x) for lang, files in classified: if isinstance(f, str): f = File.from_string(f) # whatever if f.is_built: ... else: ...

that is a net worse outcome. We don't want to have the types expanded, we want to keep them narrow, and with the Generic approach you can still widen them if you want by a cast, which is much cheaper than having to have isinstance checks that you know will never be true.

So, then the entire problem with my patch was to use list? that seems like a small and normal ask in a review.

This whole situation is super frustrating, because when we require the concrete list it means you can't pass an iterator or a generator expression, even though we have no need to force it. I blame the python devs for this, since it really needs language support of some kind, but it doesn't end the frustration

Yes, I agree it is a problem. One can hope that is fixed in python/mypy#11001.

This is a long-standing disagreement between the two of us. You value the ability to pass an iterator as a principled stance -- duck typing -- and I say we never once actually want to do that for reasons that don't relate to type theory and also I don't believe in genericity of container types. I don't mind immutability but I'm staunchly against pretending there's a practical use for iterators and generator expressions in scenarios where there isn't, and it's fine to annotate things as taking a concrete list if they do in fact idiomatically do exactly that and the resulting type annotations actually detect real runtime bugs that produce broken user builds (as has happened in the past to absolutely glorious effect, when code was refactored and switched to T.Sequence over my objections).

Runtime results matter. They are the whole point of type annotations, in fact -- having programs that are more confidently correct. Bit of a shame to dive into type-theoretical purity in a typing system with soundness holes, at the expense of the runtime.

Anyways

So, then the entire problem with my patch was to use list? that seems like a small and normal ask in a review.

Why bother when the entire contention here is that I already agreed before you opened your PR that using a generic is a good idea, said I had locally been playing with it, said I will push an update to my PR to do as you suggested, and then TWO people come by and want to submit independent competing PRs that are incorrect because Sunday is not fast enough for me to push code I have already written locally when I'm dealing with the constraints of a major religious holiday.

Nobody cared about this topic other than me until I wasn't fast enough to ignore the holidays in order to contribute to FOSS.

Please. I said I'll do it, can someone kindly take me at my word? This is nontrivially demotivating to my commitment to spending time working on FOSS. If there's something worse than saying you have already written code to handle a boring niche topic only you care about but someone else wanting to write the same code now that you pointed out the need, out of... impatience to wait a couple of days... it has got to be people getting the results wrong, and worse, than the code you already wrote and said you'd push after the weekend.

Tearing out my hair here when I was supposed to be ritually celebrating the Word of G-d.

Yes, I agree it is a problem. One can hope that is fixed in python/mypy#11001.

It has been open for approximately half a decade and the python typing community still refuses to agree it's a problem. The most they'll offer is that it's "unfortunate" that different people want different things that disagree with type-theoretical purity.

The distinction is subtle but it boils down to "there is no indication that it will ever be fixed".

The mypy maintainers are even offering questionable workarounds that don't reliably work (see the "useful_types" issue tracker) as a third-party dependency, which imo is a very significant acknowledgement of a lack of consensus that it's a problem worth solving. No effort to actually fix it in the type system or in mypy, and the actual author of that ticket now claims the option would be a bad idea.

I'll leave things alone until you return, so please leave this alone until then and we'll discuss this further at that point. Accept my apologies for stepping on your toes here, that wasn't my intention. When you have finished with your holy days, we can discuss this further.

eli-schwartz · 2025-10-01T20:31:37Z

mesonbuild/backend/vs2010backend.py

-        generator_output_files = []
-        custom_target_include_dirs = []
-        custom_target_output_files = []
+    def generate_custom_generator_commands(self, target, parent_node: ET.Element) -> T.Tuple[T.List[str], T.List[str], T.List[str]]:


This returns List[str], according to my own analysis. And it is the intermediate route for generate_genlist_for_target -> get_primary_source_lang. So indeed why did I say File?

eli-schwartz added 5 commits September 28, 2025 03:14

vs backend: don't rely on implicit re.Match truthiness

a0bdf78

This function is annotated as bool, but actaully returns the raw Optional[Match]. Every use of it is in terms of `if func()`. Tweak it to return whether a match *was* found.

fix obviously wrong type annotations

3f63a02

"None" typo from commit 23818fc. Nonsense from me in commit d0cbda9 -- how can `ofile: str` have .write() str/File confusion from initial implementation all the way back in commit 36bf53b.

reduce exposed type interface from Path to PurePath

2dfe727

All we need is the latter, and avoiding file I/O types from pathlib is good.

mypy: further work on filling in missing types

a05df2c

``` -Found 182 errors in 8 files (checked 9 source files) +Found 95 errors in 8 files (checked 9 source files) ```

eli-schwartz requested a review from jpakkane as a code owner September 28, 2025 19:59

bruchar1 added the typing label Sep 29, 2025

dcbaker requested changes Oct 1, 2025

View reviewed changes

eli-schwartz commented Oct 1, 2025

View reviewed changes

eli-schwartz mentioned this pull request Oct 3, 2025

utils/universal: make classify_unity_sources generic #15073

Closed



		def classify_unity_sources(compilers: T.Iterable['Compiler'], sources: T.Sequence['FileOrString']) -> T.Dict['Compiler', T.List['FileOrString']]:
		def classify_unity_sources(compilers: T.Iterable['Compiler'], sources: T.Union[T.List['FileOrString'], T.List[File]]) -> T.Dict['Compiler', T.List['FileOrString']]:

Uh oh!

mypy: further work on filling in missing types #15057

Are you sure you want to change the base?

mypy: further work on filling in missing types #15057

Conversation

eli-schwartz commented Sep 28, 2025

Uh oh!

dcbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-schwartz Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eli-schwartz Oct 3, 2025 •

edited

Loading