Skip to content

mypy: further work on filling in missing types #15057

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

mypy: further work on filling in missing types #15057

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0bdf78
vs backend: don't rely on implicit re.Match truthiness
eli-schwartz Sep 25, 2025
3f63a02
fix obviously wrong type annotations
eli-schwartz Sep 25, 2025
2dfe727
reduce exposed type interface from Path to PurePath
eli-schwartz Sep 26, 2025
3c1ca3e
fix another sinful usage of typing.Sequence[str]
eli-schwartz Sep 26, 2025
a05df2c
mypy: further work on filling in missing types
eli-schwartz Sep 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mesonbuild/backend/backends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ def get_unity_source_file(self, target: T.Union[build.BuildTarget, build.CustomT
osrc = f'{target.name}-unity{number}.{suffix}'
return mesonlib.File.from_built_file(self.get_target_private_dir(target), osrc)

def generate_unity_files(self, target: build.BuildTarget, unity_src: str) -> T.List[mesonlib.File]:
def generate_unity_files(self, target: build.BuildTarget, unity_src: T.Union[T.List[FileOrString], T.List[File]]) -> T.List[mesonlib.File]:
abs_files: T.List[str] = []
result: T.List[mesonlib.File] = []
compsrcs = classify_unity_sources(target.compilers.values(), unity_src)
Expand Down
14 changes: 7 additions & 7 deletions mesonbuild/backend/vs2010backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
from ..arglist import CompilerArgs
from ..interpreter import Interpreter

Project = T.Tuple[str, Path, str, MachineChoice]
Project = T.Tuple[str, PurePath, str, MachineChoice]

def autodetect_vs_version(build: T.Optional[build.Build], interpreter: T.Optional[Interpreter]) -> backends.Backend:
vs_version = os.getenv('VisualStudioVersion', None)
Expand Down Expand Up @@ -88,13 +88,13 @@ def split_o_flags_args(args: T.List[str]) -> T.List[str]:
o_flags += ['/O' + f for f in flags]
return o_flags

def generate_guid_from_path(path, path_type) -> str:
def generate_guid_from_path(path: PurePath, path_type) -> str:
return str(uuid.uuid5(uuid.NAMESPACE_URL, 'meson-vs-' + path_type + ':' + str(path))).upper()

def detect_microsoft_gdk(platform: str) -> bool:
return re.match(r'Gaming\.(Desktop|Xbox.XboxOne|Xbox.Scarlett)\.x64', platform, re.IGNORECASE)
return re.match(r'Gaming\.(Desktop|Xbox.XboxOne|Xbox.Scarlett)\.x64', platform, re.IGNORECASE) is not None

def filtered_src_langs_generator(sources: T.List[str]):
def filtered_src_langs_generator(sources: T.List[File]):
for src in sources:
ext = src.split('.')[-1]
if compilers.compilers.is_source_suffix(ext):
Expand All @@ -106,7 +106,7 @@ def filtered_src_langs_generator(sources: T.List[str]):
# simply refer to the project's shared intellisense define and include fields, rather than have to fill out their
# 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]:
Copy link
Member

Choose a reason for hiding this comment

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

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

lang_counts = Counter([compilers.compilers.SUFFIX_TO_LANG[src.suffix] for src in target_sources if compilers.compilers.is_source_suffix(src.suffix)])
lang_counts += Counter(filtered_src_langs_generator(custom_sources))
most_common_lang_list = lang_counts.most_common(1)
Expand Down Expand Up @@ -288,7 +288,7 @@ def generate(self,
Vs2010Backend.touch_regen_timestamp(self.environment.get_build_dir())

@staticmethod
def get_regen_stampfile(build_dir: str) -> None:
def get_regen_stampfile(build_dir: str) -> str:
return os.path.join(os.path.join(build_dir, Environment.private_dir), 'regen.stamp')

@staticmethod
Expand Down Expand Up @@ -395,7 +395,7 @@ def get_target_deps(self, t: T.Dict[T.Any, T.Union[build.Target, build.CustomTar
ret.update(all_deps)
return ret

def generate_solution_dirs(self, ofile: str, parents: T.Sequence[Path]) -> None:
def generate_solution_dirs(self, ofile: T.TextIO, parents: T.Sequence[PurePath]) -> None:
prj_templ = 'Project("{%s}") = "%s", "%s", "{%s}"\n'
iterpaths = reversed(parents)
# Skip first path
Expand Down
2 changes: 1 addition & 1 deletion mesonbuild/utils/universal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ def get_compiler_for_source(compilers: T.Iterable['Compiler'], src: 'FileOrStrin
raise MesonException(f'No specified compiler can handle file {src!s}')


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']]:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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. :(

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@eli-schwartz eli-schwartz Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

compsrclist: T.Dict['Compiler', T.List['FileOrString']] = {}
for src in sources:
comp = get_compiler_for_source(compilers, src)
Expand Down