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 all 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
4 changes: 2 additions & 2 deletions 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 Expand Up @@ -464,7 +464,7 @@ def init_language_file(suffix: str, unity_file_number: int) -> T.TextIO:
return result

@staticmethod
def relpath(todir: str, fromdir: str) -> str:
def relpath(todir: T.Union[str, os.PathLike], fromdir: T.Union[str, os.PathLike]) -> str:
return os.path.relpath(os.path.join('dummyprefixdir', todir),
os.path.join('dummyprefixdir', fromdir))

Expand Down
110 changes: 58 additions & 52 deletions mesonbuild/backend/vs2010backend.py
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions mesonbuild/backend/vs2017backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from ..mesonlib import MesonException

if T.TYPE_CHECKING:
from ..arglist import CompilerArgs
from ..build import Build
from ..interpreter import Interpreter

Expand Down Expand Up @@ -46,11 +47,11 @@ def detect_toolset(self) -> None:
if sdk_version:
self.windows_target_platform_version = sdk_version.rstrip('\\')

def generate_debug_information(self, link):
def generate_debug_information(self, link: ET.Element) -> None:
# valid values for vs2017 is 'false', 'true', 'DebugFastLink', 'DebugFull'
ET.SubElement(link, 'GenerateDebugInformation').text = 'DebugFull'

def generate_lang_standard_info(self, file_args, clconf):
def generate_lang_standard_info(self, file_args: T.Dict[str, CompilerArgs], clconf: ET.Element) -> None:
if 'cpp' in file_args:
optargs = [x for x in file_args['cpp'] if x.startswith('/std:c++')]
if optargs:
Expand Down
5 changes: 3 additions & 2 deletions mesonbuild/backend/vs2019backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from .vs2010backend import Vs2010Backend

if T.TYPE_CHECKING:
from ..arglist import CompilerArgs
from ..build import Build
from ..interpreter import Interpreter

Expand Down Expand Up @@ -41,11 +42,11 @@ def detect_toolset(self) -> None:
if sdk_version:
self.windows_target_platform_version = sdk_version.rstrip('\\')

def generate_debug_information(self, link):
def generate_debug_information(self, link: ET.Element) -> None:
# valid values for vs2019 is 'false', 'true', 'DebugFastLink', 'DebugFull'
ET.SubElement(link, 'GenerateDebugInformation').text = 'DebugFull'

def generate_lang_standard_info(self, file_args, clconf):
def generate_lang_standard_info(self, file_args: T.Dict[str, CompilerArgs], clconf: ET.Element) -> None:
if 'cpp' in file_args:
optargs = [x for x in file_args['cpp'] if x.startswith('/std:c++')]
if optargs:
Expand Down
5 changes: 3 additions & 2 deletions mesonbuild/backend/vs2022backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from .vs2010backend import Vs2010Backend

if T.TYPE_CHECKING:
from ..arglist import CompilerArgs
from ..build import Build
from ..interpreter import Interpreter

Expand Down Expand Up @@ -41,11 +42,11 @@ def detect_toolset(self) -> None:
if sdk_version:
self.windows_target_platform_version = sdk_version.rstrip('\\')

def generate_debug_information(self, link):
def generate_debug_information(self, link: ET.Element) -> None:
# valid values for vs2022 is 'false', 'true', 'DebugFastLink', 'DebugFull'
ET.SubElement(link, 'GenerateDebugInformation').text = 'DebugFull'

def generate_lang_standard_info(self, file_args, clconf):
def generate_lang_standard_info(self, file_args: T.Dict[str, CompilerArgs], clconf: ET.Element) -> None:
if 'cpp' in file_args:
optargs = [x for x in file_args['cpp'] if x.startswith('/std:c++')]
if optargs:
Expand Down
2 changes: 1 addition & 1 deletion mesonbuild/coredata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ def __init__(self, cmd_options: SharedCMDOptions, scratch_dir: str, meson_comman
self.regen_guid = str(uuid.uuid4()).upper()
self.install_guid = str(uuid.uuid4()).upper()
self.meson_command = meson_command
self.target_guids = {}
self.target_guids: T.Dict[str, str] = {}
self.version = version
self.cross_files = self.__load_config_files(cmd_options, scratch_dir, 'cross')
self.compilers: PerMachine[T.Dict[str, Compiler]] = PerMachine(OrderedDict(), OrderedDict())
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
Loading