type safe option value getters - #15678
Draft
dcbaker wants to merge 24 commits into
Draft
Conversation
dcbaker
force-pushed
the
submit/type-safe-option-getters
branch
from
April 6, 2026 19:56
baed5ae to
12bbf92
Compare
dcbaker
force-pushed
the
submit/type-safe-option-getters
branch
5 times, most recently
from
June 11, 2026 16:59
ba13fc2 to
5bae175
Compare
11 tasks
dcbaker
force-pushed
the
submit/type-safe-option-getters
branch
4 times, most recently
from
June 22, 2026 16:07
b8add99 to
3600c7b
Compare
dcbaker
force-pushed
the
submit/type-safe-option-getters
branch
3 times, most recently
from
June 22, 2026 21:32
138f9b6 to
455874d
Compare
And do some cleanups. This one test takes a ton of time to run, and cannot be parallelized. I have split the code so that we run one test per language, but the rest of the subtests remain. This allows us to run up to 11 parallel tests for templates, which can really help with the test runtime.
This leaves a wrapper in the coredata struct, which will allow us to have one big mechanical change in the next commit
This is mostly generated through search and replace, but with the wrapper in CoreData removed manually
Leaves a wrapper in place that will be removed later
These provide unconstrained ElementryOptionValues variants. The next patches will provide typed versions using the normal name, that do automatic type validation.
These use a TypeVar to constrain the type, and then do a runtime check to ensure that the option is of the correct type. One has not been added for `get_option_and_value_for` because we need to further constrain the types via overloads, but I can't figure out how to make the type checker happy with it.
This allows use to replace patterns like: ```python if key in opstore and opstore.get_value_for(...) ``` with ```python if opstore.get_value_for(..., default=...) ```
This replaces most of the uses of the unsafe get_value_for, with a couple of exceptions: - There is code in the Compiler that does some interesting BuildTarget | None checking that should be wrapped - the `install_umask` option cannot use this helper due to it's variant type - There's a wrapper in the Module code that needs to be replaced
There are still a few uses of the unsafe variant, manly around compilers and some wrappers that need to be removed. Additionally, this work allows some cleanups to remove try/except blocks
These are allowed to take None for target. This is the only case where the subproject argument is not `target.subproject`. To simplify this, I have implemented the signature such that a target or a SubProject (which includes None) can be passed, a small helper is used to convert the union into a tuple of BuildTarget | None, SubProject | None. This simplifies the implementation, fixes the typing, and is much cleaner than a previous approach that used type overloading.
There is some added complexity because the langauge of an option was merged back into the name of the key instead of being a separate field. This method was basically doing the same thing as get_option_for_maybe_target, plus building a key. Some of the uses didn't make sense, as we knew we didn't have a target and just wanted a global value, plus we lost our type checking.
It was just another layer of indirection, and doesn't provide type safety
This is just a wrapper around `get_value_for_target` with `default=False`.
Checking that the option is in the command line arguments is less robust than checking that the Option has a non default value.
dcbaker
force-pushed
the
submit/type-safe-option-getters
branch
from
June 22, 2026 21:55
455874d to
733f7c8
Compare
Member
Author
|
There's a bug in "compilers: Fix the signature of get_option_{std,compile,link}_args", but I can't find it immediately by inspection |
bonzini
reviewed
Jun 23, 2026
|
|
||
| try: | ||
| host_compiler_args = self.host_compiler.get_option_compile_args(target, subproject) | ||
| host_compiler_args = self.host_compiler.get_option_compile_args(target) |
Contributor
There was a problem hiding this comment.
I think this needs to be target if target is not None else subproject.
Contributor
|
The failing test logs ( |
This file contains hidden or 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
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 adds a couple of methods to the OptionStore to work with options value in a type safe manner. This works by putting the type in the getter itself, this avoids the need for the pattern of:
While also giving better error messages. Additionally, I've added a
default=parameter, to allow not erroring when an options isn't available (a per-platform or per-compiler option). The end result is the following:get_value_fora type safe getter for a system valueget_value_for_untypeda type unsafe getter for system values, works likeget_value_forin current mainget_value_for_targeta type safe getter for an option that could be overwritten by a targetget_value_for_target_untypeda type unsafe getter for an option that could be overwritten by a target, same asget_value_for_targetin current main branchget_value_for_maybe_targeta type safe getter that allows a target to be optional. This is a bit ugly, but I don't see a better way to handle this without duplicating code in a way that would lead to bugs.this allows the following patterns to be eliminated:
with
(and the same for targets)
and
with
and
with
This has been set to draft as I'm sure there will be some issues with Windows and the VS backend that need to be resolved.