A few small type annotation fixes for the build module #13549
Conversation
I have annotated this as we actually use it... however, I'm concerned that this is actually supposed to be `SubProject`, the unique ID of the project rather than the pretty display name.
dcbaker
force-pushed
the
submit/a-few-build-annotations
branch
from
597778a
to
7bb653b
Compare
| for k, v in builddata.subprojects.items(): | ||
| c: T.Dict[str, str] = { | ||
| 'name': k, | ||
| 'version': v, | ||
| 'descriptive_name': builddata.projects.get(k), | ||
| 'descriptive_name': builddata.projects.get(T.cast('SubProject', k)), |
There was a problem hiding this comment.
Looks like this cast shouldn't be necessary except that builddata.subprojects lacks typing?
There was a problem hiding this comment.
It's neceissary because Build.projects is dict[SubProject, str], so indexing with a str is a type error.
| @@ -471,16 +472,16 @@ def list_machines(builddata: build.Build) -> T.Dict[str, T.Dict[str, T.Union[str | |||
|
|
|||
| def list_projinfo(builddata: build.Build) -> T.Dict[str, T.Union[str, T.List[T.Dict[str, str]]]]: | |||
| result: T.Dict[str, T.Union[str, T.List[T.Dict[str, str]]]] = { | |||
| 'version': builddata.project_version, | |||
| 'version': builddata.project_version or 'unknown', | |||
There was a problem hiding this comment.
Isn't this just #13481 (comment) again.
The type annotations say builddata.project_version can be None because the object is created like that, but as soon as func_project is executed it is updated to 'unknown'. In the linked PR, we solved this with an assert instead.
| @@ -255,7 +255,8 @@ def __init__(self, environment: environment.Environment): | |||
| self.data: T.List[Data] = [] | |||
| self.symlinks: T.List[SymlinkData] = [] | |||
| self.static_linker: PerMachine[StaticLinker] = PerMachine(None, None) | |||
| self.subprojects = {} | |||
| # TODO: Shouldn't this be `dict[SubProject, str]`? That would mean some changes to the interpreter | |||
| self.subprojects: T.Dict[str, str] = {} # Maps pretty subproject name to it's version | |||
There was a problem hiding this comment.
The changes to SubProject don't look significant.
| @@ -3072,12 +3072,16 @@ class TestSetup: | |||
| env: EnvironmentVariables | |||
| exclude_suites: T.List[str] | |||
|
|
|||
| def get_sources_string_names(sources, backend): | |||
|
|
|||
| def get_sources_string_names(sources: T.Sequence[T.Union[str, BuildTarget, CustomTarget, CustomTargetIndex, GeneratedList, File, ExtractedObjects]], | |||
There was a problem hiding this comment.
As usual, sequence means it would be legal to do:
get_sources_string_names('foobar', current_backend)But we already know that the only place this function is ever used, must pass a list and nothing but a list.
| names = [] | ||
| names: T.List[str] = [] | ||
| if isinstance(sources, str): | ||
| sources = [sources] |
There was a problem hiding this comment.
... which means this logic is incorrect, it does nothing except work around incorrect type information.
There was a problem hiding this comment.
In fact, CustomTarget already incorrectly says it takes a Sequence as sources, then converts it to a list(sources) which flatly misbehaves if passed the str that it is allowed to be passed. But by the time we hit get_sources_string_names it is guaranteed to be a list anyway, so I'll spare my heartburn over python's typing model w.r.t. CustomTarget and stick to pointing out that this condition right here is impossible.
There was a problem hiding this comment.
I know this really bothers you, but this is the way to have covariant sequences, and we spend a lot of time bending over backwards to not use Sequence when it would save us a ton of pain, just have a look at Interpreter.source_strings_to_files, and then all of the uses that don't fit the already substantial number of overloads we have.
But it's really correct for CustomTarget to use a Sequence. It's madness to do anything else. Otherwise we'd have to write overloads for a combinatorial explosion of arguments.
The better solution, and the one that I'd like to get to, is to never pass string file names past the Interpreter, and to convert them all to File at that point. Because this problem basically exists only because str happens to be a valid Sequence, and Python doesn't have type subtraction to fix this.
There was a problem hiding this comment.
No it is NOT correct to use a Sequence. It is correct to use a CovariantIndexableContainerizedHolderOfDiscreteElements, key terms being "covariant" and "container". We want a container type.
A list is a container. A tuple is a container. Custom objects designed to be containers are containers. A string is typed as a sequence but it is not in fact a "container type". It doesn't hold anything inside.
It is unfortunate that python typing doesn't support container types (no, typing.Container doesn't count). I don't think the solution is to reconvert datatypes back to themselves, especially when it is the WRONG object mode. I would be more comfortable if this were an assert, and use of a 'str' when a Sequence[str] is expected produced an "ERROR: Unhandled python exception" rather than being silently converted to ['s', 't', 'r'].
It's not even the use of Sequence itself that I object to, it's the use of list() on sequences. It is the tail wagging the dog. More importantly, it is a bug magnet. I've tried pointing this out on numerous occasions.
The whys and wherefores of "the problem basically exists only because ..." aren't very important to me. I know this already.
These were part of some other work I was doing, but they could land separately.