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

Mypy checking for build.py #11740

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

Conversation

Volker-Weissmann
Copy link
Contributor

@Volker-Weissmann Volker-Weissmann commented Apr 26, 2023

run_mypy.py currently does not check mesonbuild/build.py, much to my distaste. Currently, mypy would show a lot of warnings in build.py if it did.

I fixed all but one mypy warning in build.py.
Some of these mypy warnings hinted at real bugs, I fixed those.
The 3 test cases I added were inspired by the mypy warnings and my experiences fixing them.

There is still one mypy warning left:

mesonbuild/build.py:2125:40: error: Argument 2 to "join" has incompatible type "Union[str, bool]"; expected "str"  [arg-type]

That one hints at a real bug that I can use to create a crash. See #11748.

What do you think? Should I do ninjabackend.py next? I suspect that #1633 is hiding in the mypy warnings of ninjabackend.py

And no, I'm not gaming any LOC metric here. :-)

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Only took a quick look, here are my initial thoughts.

@@ -2741,6 +2741,9 @@ def _add_output(self, f: File) -> None:
self.outputs.append(o)
self.sources_map[f] = o

def is_linkable_output(self, output: str) -> bool:
return False
Copy link
Member

Choose a reason for hiding this comment

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

This is annoying and happens because for the sake of actually implementing it we base this off of BuildTarget, but for proper semantics / type safety we really would be better off if it was a CustomTarget.

Copy link
Contributor Author

@Volker-Weissmann Volker-Weissmann Apr 30, 2023

Choose a reason for hiding this comment

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

Should I do something about that before you merge, or are you just informing me on the state of meson.

Comment on lines -65 to +63
str, build.BuildTarget, build.CustomTarget, build.CustomTargetIndex,
build.ExtractedObjects, build.GeneratedList, ExternalProgram,
mesonlib.File]]
str, mesonlib.File, build.CustomTarget, build.CustomTargetIndex, build.GeneratedList]]
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you do pass a BuildTarget or extracted object or some such in, here? Why is it more correct to change the allowance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing ExternalProgram or ExtractedObjects results in an unhandled exception before this PR.

As to why I also removed build.BuildTarget:
The argument will be passed to msgfmt or itstool eventually.

These tools accept *.mo, *.xml and *.desktop files. (They don't check the filename, but just care about the content.) I don't see a way how a BuildTarget could also be a valid and useful *.mo or *.xml or *.desktop file.

If you pass an ELF file to msgfmt, it will print warnings, and output an empty file.

if num_outdirs not in {1, num_out}:
m = 'Target {!r} has {} outputs: {!r}, but only {} "install_dir"s were found.\n' \
"Pass 'false' for outputs that should not be installed and 'true' for\n" \
'using the default installation directory for an output.'
raise MesonException(m.format(t.name, num_out, t.get_outputs(), num_outdirs))
assert len(t.install_tag) == num_out
for x in outdirs_unchecked:
assert not x is True
outdirs = T.cast(T.List[T.Union[str, T.Literal[False]]], outdirs_unchecked)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
outdirs = T.cast(T.List[T.Union[str, T.Literal[False]]], outdirs_unchecked)
outdirs = T.cast('T.List[T.Union[str, T.Literal[False]]]', outdirs_unchecked)

T.cast always needs to quote the type annotation being used -- otherwise it gets evaluated at runtime. It is not affected by from __future__ import annotations.

Copy link
Contributor Author

@Volker-Weissmann Volker-Weissmann Apr 28, 2023

Choose a reason for hiding this comment

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

Yes, that's what made the tests fail.

@@ -1233,14 +1233,14 @@ def create_test_serialisation(self, tests: T.List['Test']) -> T.List[TestSeriali
def write_test_serialisation(self, tests: T.List['Test'], datafile: T.BinaryIO) -> None:
pickle.dump(self.create_test_serialisation(tests), datafile)

def construct_target_rel_paths(self, t: T.Union[build.Target, build.CustomTargetIndex], workdir: T.Optional[str]) -> T.List[str]:
def construct_target_rel_paths(self, t: T.Union[build.Target, build.CustomTargetIndex], workdir: T.Optional[str]) -> T.Sequence[str]:
Copy link
Member

Choose a reason for hiding this comment

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

Sequences of str are bad. Like, really bad. T.List[str] will accept ['foo', 'bar'] but T.Sequence[str] will accept that and also 'foo', because a string is a sequence of single-character strings.

What exactly is wrong with it as it is right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

T.Sequence[str] will accept that and also 'foo'

I did not know that.

What exactly is wrong with it as it is right now?

I think we are facing a dilemma:

  • You cannot implicitly convert T.List[str] to T.List[T.Union[str, other]] or mypy will complain
  • You can implicitly convert T.Sequence[str] to T.Sequence[T.Union[str, other]], but as you said, sequences of str's are bad
  • You cannot implicitly convert ImmutableListProtocol[str] to ImmutableListProtocol[T.Union[str, other]]
  • You could copy the list, but that means a performance hit
  • You could insert a T.cast call, but that is ugly and disables many mypy checks.

I think I will be going with the last one.

@@ -662,7 +662,7 @@ def get_option(self, key: 'OptionKey') -> T.Union[str, int, bool, 'WrapMode']:
# cast, we know what's in coredata anyway.
# TODO: if it's possible to annotate get_option or validate_option_value
# in the future we might be able to remove the cast here
return T.cast('T.Union[str, int, bool, WrapMode]', self.options[key].value)
return T.cast(T.Union[str, int, bool, 'WrapMode'], self.options[key].value)
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove these quotes.

(Actually, it is not strictly necessary to remove any quotes. Usually it doesn't particularly hurt, it just takes a bit to look at each change.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -671,7 +671,7 @@ def parse_overrides(kwargs: T.Dict[str, T.Any]) -> T.Dict[OptionKey, str]:
# In this case we have an already parsed and ready to go dictionary
# provided by typed_kwargs
if isinstance(opts, dict):
return T.cast('T.Dict[OptionKey, str]', opts)
return T.cast(T.Dict[OptionKey, str], opts)
Copy link
Member

Choose a reason for hiding this comment

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

same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -706,22 +707,22 @@ def __init__(
for_machine: MachineChoice,
sources: T.List['SourceOutputs'],
structured_sources: T.Optional[StructuredSources],
objects: T.List[ObjectTypes],
objects: T.Sequence[T.Union[str, File, 'ExtractedObjects']],
Copy link
Member

Choose a reason for hiding this comment

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

I would think we need a new type alias for this. Also, list > sequence.

@Volker-Weissmann Volker-Weissmann marked this pull request as draft April 28, 2023 15:01
@Volker-Weissmann Volker-Weissmann force-pushed the typehints_1 branch 3 times, most recently from 8022b0b to 439ffe8 Compare April 30, 2023 13:11
Feed a custom_target output to cc.preprocess,
then feed that output to a generator.
@Volker-Weissmann Volker-Weissmann force-pushed the typehints_1 branch 5 times, most recently from 191d324 to 8d65d71 Compare April 30, 2023 16:26
@Volker-Weissmann Volker-Weissmann marked this pull request as ready for review April 30, 2023 19:27
@Volker-Weissmann
Copy link
Contributor Author

I fixed everything you told me to and made the test pass.

@tristan957
Copy link
Contributor

@dcbaker does your PR make mypy happy for build.py?

@dcbaker
Copy link
Member

dcbaker commented May 3, 2023

@tristan957 no, but it does fix a lot of mypy issues that aren't apparent because it's not 100% clear what's coming in from the interpreter. There's a lot of stuff in there that's pretty broken.

I'm also solving the Sequence thing, but just enforcing that all string paths are converted to files in the Interpreter, so that the backend never has to deal with them. As an added bonus that makes sequences safe to use because we don't have strings to deal with

Copy link
Member

@bruchar1 bruchar1 left a comment

Choose a reason for hiding this comment

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

With annotations from __future__, you shouldn't need to quote types. I know many types are quoted for historical reasons, but if we touch them, I think it's better to correct them.

@@ -808,7 +808,7 @@ def process_objectlist(self, objects):
else:
raise InvalidArguments(f'Bad object of type {type(s).__name__!r} in target {self.name!r}.')

def process_sourcelist(self, sources: T.List['SourceOutputs']) -> None:
def process_sourcelist(self, sources: ImmutableListProtocol['SourceOutputs']) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def process_sourcelist(self, sources: ImmutableListProtocol['SourceOutputs']) -> None:
def process_sourcelist(self, sources: ImmutableListProtocol[SourceOutputs]) -> None:

@@ -1445,7 +1451,7 @@ def link_whole(self, target):
self.link_whole_targets.append(t)

def extract_all_objects_recurse(self) -> T.List[T.Union[str, 'ExtractedObjects']]:
objs = [self.extract_all_objects()]
objs: T.List[T.Union[str, 'ExtractedObjects']] = [self.extract_all_objects()]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
objs: T.List[T.Union[str, 'ExtractedObjects']] = [self.extract_all_objects()]
objs: T.List[T.Union[str, ExtractedObjects]] = [self.extract_all_objects()]

@@ -2662,21 +2677,21 @@ def is_internal(self) -> bool:
return CustomTargetIndex(self, self.outputs[0]).is_internal()

def extract_all_objects_recurse(self) -> T.List[T.Union[str, 'ExtractedObjects']]:
return self.get_outputs()
return T.cast('T.List[T.Union[str, ExtractedObjects]]', self.get_outputs())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return T.cast('T.List[T.Union[str, ExtractedObjects]]', self.get_outputs())
return T.cast(T.List[T.Union[str, ExtractedObjects]], self.get_outputs())

return "@cus"

def __getitem__(self, index: int) -> 'CustomTargetIndex':
return CustomTargetIndex(self, self.outputs[index])

def __setitem__(self, index, value):
def __setitem__(self, index: int, value: 'CustomTargetIndex') -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def __setitem__(self, index: int, value: 'CustomTargetIndex') -> None:
def __setitem__(self, index: int, value: CustomTargetIndex) -> None:

@@ -109,7 +109,7 @@ def __init__(self, type_name: DependencyTypeName, kwargs: T.Dict[str, T.Any]) ->
# Raw -L and -l arguments without manual library searching
# If None, self.link_args will be used
self.raw_link_args: T.Optional[T.List[str]] = None
self.sources: T.List[T.Union['FileOrString', 'CustomTarget', 'StructuredSources']] = []
self.sources: T.List[T.Union['File', 'CustomTarget', 'StructuredSources']] = []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.sources: T.List[T.Union['File', 'CustomTarget', 'StructuredSources']] = []
self.sources: T.List[T.Union[File, CustomTarget, StructuredSources]] = []

@@ -167,7 +167,7 @@ def get_all_link_args(self) -> T.List[str]:
def found(self) -> bool:
return self.is_found

def get_sources(self) -> T.List[T.Union['FileOrString', 'CustomTarget', 'StructuredSources']]:
def get_sources(self) -> ImmutableListProtocol[T.Union['File', 'CustomTarget', 'StructuredSources']]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_sources(self) -> ImmutableListProtocol[T.Union['File', 'CustomTarget', 'StructuredSources']]:
def get_sources(self) -> ImmutableListProtocol[T.Union[File, CustomTarget, StructuredSources]]:

Comment on lines +45 to +46
environment: 'Environment'
subdir: 'str'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
environment: 'Environment'
subdir: 'str'
environment: Environment
subdir: str

@@ -246,7 +249,7 @@ def gettext(self, state: 'ModuleState', args: T.Tuple[str], kwargs: 'Gettext') -

extra_args = kwargs['args']
targets: T.List['Target'] = []
gmotargets: T.List['build.CustomTarget'] = []
gmotargets: T.List['build.Target'] = []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gmotargets: T.List['build.Target'] = []
gmotargets: T.List[build.Target] = []

@@ -1406,7 +1407,7 @@ def extract_as_list(dict_object: T.Dict[_T, _U], key: _T, pop: bool = False) ->


def typeslistify(item: 'T.Union[_T, T.Sequence[_T]]',
types: 'T.Union[T.Type[_T], T.Tuple[T.Type[_T]]]') -> T.List[_T]:
types: 'T.Union[T.Type, T.Tuple[T.Type, ...]]') -> T.List[_T]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
types: 'T.Union[T.Type, T.Tuple[T.Type, ...]]') -> T.List[_T]:
types: T.Union[T.Type, T.Tuple[T.Type, ...]]) -> T.List[_T]:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants