Type safety for the VisualStudio Backend(s) #14219
Open
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
21 tasks
dcbaker
force-pushed
the
submit/vs2010-backend-typing
branch
3 times, most recently
from
90d76d9 to
a9ab54c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
@bonzini: the extract_all_objects methods get used all over the place, in the Interpreter, the Ninja Backend, and within build itself. I don't think we can drop those. |
|
They aren't dropped, just unified to CustomTargetBase. The CustomTargetIndex case was wrong in that it returned all objects instead of just the required index; and they had the wrong type because they could return |
|
Ah, I see what you did now. Okay, yeah, plus a little work in the ExtractedObjects class and we can make that work. |
dcbaker
force-pushed
the
submit/vs2010-backend-typing
branch
2 times, most recently
from
62c8924 to
2210da5
Compare
bruchar1
reviewed
Feb 5, 2025
mesonbuild/backend/vs2010backend.py
Outdated
| @@ -356,6 +356,7 @@ def get_target_deps(self, t: T.Dict[T.Any, T.Union[build.Target, build.CustomTar | |||
| if isinstance(ldep, build.CustomTargetIndex): | |||
| all_deps[ldep.get_id()] = ldep.target | |||
| elif isinstance(ldep, File): | |||
| # XXX: this this true? What if the file `is_built`? | |||
There was a problem hiding this comment.
Is there an action to take here?
There was a problem hiding this comment.
I mean, probably. But I don't have a windows machine handy to find out what happens if a generated file is used for a link_depends and fix it.
There was a problem hiding this comment.
Ok. I will try to look at this (maybe not in a near future, but someday...)
There was a problem hiding this comment.
I did reduce it from an XXX comment to just a comment. along with fixing the this this :)
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.
dcbaker
force-pushed
the
submit/vs2010-backend-typing
branch
from
2210da5 to
fb95fe7
Compare
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.
dcbaker
force-pushed
the
submit/vs2010-backend-typing
branch
from
fb95fe7 to
c5d0de9
Compare
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.
This gets most of the VS backend type safe, with the exception of some of the object extraction stuff where there seems to be a real potential for things to explode.This gets all of the VS Backends type safe. Thanks @bonzini for for help with the object stuff.
This turned up a lot of issues in the build module and in the base
backendmodule.It should be possible to merge pieces of this series incrementally.
It should be noted that this is untested on the vs2010 backend. I expect to find issues at runtime.