Full type annotations for the XCode backend #14230
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This gives us a simpler way to annotate things returning an option value, and gives us an easy single place to change that will affect everywhere if the option value types are changed.
Which can return any of `ElementaryOptionValues`, but is currently typed as if it only returns `str | int | bool`.
Which should be `Target | CustomTargetIndex`, not just `Target`
So that it can meet it's interface requirements.
Which it needs in the vs2010 backend
Guess what the vs2010 backend can hit?
There are a couple of issues with the current implementation: 1. The API doesn't match the BuildTarget on 2. The CustomTargetIndex version returns all of the CustomTarget's objects, not just the one for that index To fix that the extract_all_objects method is moved to the CustomTargetBase class, and extended to implement the same API as the BuildTarget implementaiton. This also finds a bunch of previously incorrectly typed (and thus unhandled) cases in the Backend base class around ExtractedObjects from CustomTargets Co-Authored-by: Paolo Bonzini <[email protected]>
This means it can be either `str`, `File`, or `File | str`, which gives us the value of knowing that if the input is T, the output is Dict[Compiler, List[T]]`
Which may be passed a BuildTarget, CustomTarget, CustomTargetIndex, or GeneratedList This requires using `get_subdir()` because `CustomTargetIndex` doesn't have a `subdir` attribute.
Which can actually be a `PathLike[str] | str`, not just `str`
…le_serialisation Which should take `Iterable[BuildTarget | CustomTarget | CustomTargetIndex]`, not just `List[BuildTarget]`.
If this gets to the backend it will cause an uncaught exception, so let's catch it here and just give a real error.
At least in the vs backend this will get called, and it will fail. So not only do we need to update the type annotations, but we also need to fix the implementaiton.
It's really only `BuildTarget | CustomTarget | CustomTargetIndex` that can be used here, so let's do that correctly. Also, use only `target.import_filename` since mypy doesn't know about the getter function.
This is actually an Iterable, since we pass both lists and OrderedSets in, but also it's not `Target`, it's `BuildTargetTypes`
which needs to take `Iterable[File | str]` not `str`
This is mostly easy, simple stuff to get out of the way before tackling the harder issues
This is a little more invasive since we're currently using lists where we should be using tuples. I've added a TypeAlias to make this code clearer and cleaner.
This gets the vs2010 backend completely mypy compliant
It's almost never what you want
The latter of which creates a new list with references to all of the objects in the inputs. In our case that's wasted as we just iterate anyway.
Store the buildphasemap in the backend
This points out that we have lots of potential issues with adding objects to targets in the XCode backend. Since the backend is experimental, I haven't attempted to fix this, just raise a nicer exception.
Since mypy doesn't understand the string check.
Which requires casting to make things work correctly
We don't have code to handle CustomTarget, CustomTargetIndex, or GeneratedList. I don't have the ability to write the code, but we should at least give a useful error message
This function isn't supposed to be used for build targets, so just open code this instead. This also gets xcode fully type safe.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Opened as a draft because this is completely untested on my end and we have no CI for this.
Built on #14219