Type safe option value getter #14561
Conversation
Yeah, I didn't plan to merge them that way, just have smaller chunks to review |
dcbaker
force-pushed
the
submit/type-safe-option-value-getter
branch
from
0b91798 to
85f0d3b
Compare
This provides a variant of `get_value_for` that does run-time type checking, but in a way that allows static type checkers like mypy to understand. This gives us the two fold advantage of runtime checking of types that can't be statically known and not having to write a lot `assert isinstance(...), 'for mypy'` checks.
This is not possible for the `umask` option, since it is a union type.
This doesn't convert most of this, because we actually want a different helper for this
This renames the `get_option_value` -> `get_option_value_unsafe` and `get_option_value_safe` -> `get_option_value`, as we want to encourage people to use the safe variant rather than the unsafe variant.
dcbaker
force-pushed
the
submit/type-safe-option-value-getter
branch
from
85f0d3b to
6431122
Compare
| @@ -89,7 +89,7 @@ def get_option_std_args(self, target: BuildTarget, env: Environment, subproject: | |||
| if target: | |||
| std = env.coredata.get_option_for_target(target, key) | |||
| else: | |||
| std = env.coredata.optstore.get_value_for(key) | |||
| std = env.coredata.optstore.get_value_for_unsafe(key) | |||
There was a problem hiding this comment.
Why can't you use the type safe version here?
There was a problem hiding this comment.
Because I'm planning to replace that pattern (which is repeated in a lot of places) with a new safe getter. But also, with get_option_for_target being unsafe you still need to do the type check, so it makes sense to do one check instead of two.
| @@ -82,7 +82,7 @@ def get_option_std_args(self, target: BuildTarget, env: Environment, subproject: | |||
| if target: | |||
| std = env.coredata.get_option_for_target(target, key) | |||
| else: | |||
| std = env.coredata.optstore.get_value_for(key) | |||
| std = env.coredata.optstore.get_value_for_unsafe(key) | |||
There was a problem hiding this comment.
Why can't you use the type safe version here?
| @@ -87,7 +87,7 @@ def get_option_std_args(self, target: BuildTarget, env: Environment, subproject: | |||
| if target: | |||
| std = env.coredata.get_option_for_target(target, key) | |||
| else: | |||
| std = env.coredata.optstore.get_value_for(key) | |||
| std = env.coredata.optstore.get_value_for_unsafe(key) | |||
There was a problem hiding this comment.
Why can't you use the type safe version here?
| @@ -234,7 +234,7 @@ def print_options(self, title: str, opts: T.Union[options.MutableKeyedOptionDict | |||
| return | |||
| if title: | |||
| self.add_title(title) | |||
| #auto = T.cast('options.UserFeatureOption', self.coredata.optstore.get_value_for('auto_features')) | |||
| #auto = T.cast('options.UserFeatureOption', self.coredata.optstore.get_value_for_unsafe('auto_features')) | |||
There was a problem hiding this comment.
No idea, and it doesn't seem right since that won't provide a UserOption at all, but search and replace replaced it.
This (ultimately) makes the
OptionStore.get_option_value()type safe, but using a generic with a bound type. This allows us to both have runtime type checking, and allows mypy (and other type checkers) to see that at static inspection time and treat the return as safe. This, in turn, allows us to remove a lot ofassert isinstance(...), 'for mypy'checks, with a properMesonBugExceptionwith a more helpful error message.The addition of a fallback parameter also allows us to remove a bunch of
if key in options and options.get_value_for(key)style checks, through the use of the fallback.This series has been split into a bunch of patches to attempt to make review easier, but I'm perfectly happy to squash any number of them into less patches before merging.