Make coredata and environment fully type safe #13737
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
dcbaker
force-pushed
the
submit/type-safe-coredata
branch
3 times, most recently
from
e350d38 to
bd136fd
Compare
dcbaker
force-pushed
the
submit/type-safe-coredata
branch
from
bd136fd to
4ae1ed4
Compare
dcbaker
force-pushed
the
submit/type-safe-coredata
branch
3 times, most recently
from
fe33d21 to
876e4ec
Compare
21 tasks
These include things like not narrowing unions and boolean error handling
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.
We don't actually allow anything except elementry types (`str`, `bool`, `int`, `array[str]`) here, so we can tighten the typing. This in turn helps us to simplify the typing in environment.py
This as much as anything is to stop lying to envconfig about the potential types it will be given.
There were two issues, one was that we were calling from an interpreter method to an interpreter method, and second that we needed to narrow some option values.
The closure could not be made type safe because of Python's scoping rules. Plus, it didn't add give us anything the loop doesn't.
Since they need to be strings, but repr() currently gives us what we want.
BaseOption needs more work to be fully type safe, and that work is not trivial or obvious to me ATM on how to do, so we'll cast. This will require more work later.
It must implement an `__iter__` method to meet it's ABC contract. So, lets just teach `OptionStore` to do the thing it's supposed to
dcbaker
force-pushed
the
submit/type-safe-coredata
branch
from
876e4ec to
0b098bd
Compare
This final bit requires more code than anything before because we need to deal with getting option values, which we don't have tight contraints for. To address this I've added a `get_value_safe` method, which takes a type as well as a key, and does the necessary type checking to ensure that we do in fact have what we expect
Safe is just better
This allows us to get rid of some `if key in optstore optstore.get_value` and some wrapper functions around the optstore
…e_unsafe Because using the safe variant should be the default option, and the unsafe should be a "I really know what I'm doing"
Which can return any of `ElementaryOptionValues`, but is currently typed as if it only returns `str | int | bool`.
dcbaker
force-pushed
the
submit/type-safe-coredata
branch
from
0b098bd to
1147816
Compare
bruchar1
reviewed
Feb 6, 2025
| @@ -940,25 +952,25 @@ def get_static_lib_dir(self) -> str: | |||
| return self.get_libdir() | |||
|
|
|||
| def get_prefix(self) -> str: | |||
| return self.coredata.get_option(OptionKey('prefix')) | |||
| return _as_str(self.coredata.get_option(OptionKey('prefix'))) | |||
There was a problem hiding this comment.
Why can't you simply use cast here?
There was a problem hiding this comment.
We generally use runtime type checks with options because we want to catch cases where a type changes. We know what this is supposed to be, not what it actually is going to be. Really, I'd like do more of the option validation up front so that we do, in fact, know what the type is. But then annotating them gets tricky
| @@ -123,10 +123,13 @@ def _args_to_info(self, tgt: str) -> T.Dict[str, str]: | |||
| assert all(x in res for x in ['inc', 'src', 'dep', 'tgt', 'func']) | |||
| return res | |||
|
|
|||
| def _get_variable(self, name: str, fallback: T.Optional[str] = None) -> T.Union[TYPE_var, InterpreterObject]: | |||
There was a problem hiding this comment.
I think you should call it from dependency() function as well
mesonbuild/coredata.py
Outdated
| if isinstance(key, str): | ||
| key = OptionKey(key) | ||
| return self[key].value | ||
|
|
||
| def get_value_safe(self, key: T.Union[OptionKey, str], type_: T.Type[ElementaryOptionTypes]) -> ElementaryOptionTypes: |
There was a problem hiding this comment.
Isn't it possible to tell that the returned type is given by type_ ?
There was a problem hiding this comment.
That is exactly what's happening here, however the unsafe get inside could potentially return anything, so we need to do the assert to assure the type checker that it's getting what we told it it would get.
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.
I give you a mypy compliant environment and coredata. There isn't actually that much here once we get the options module safe. With this in place the next big roadblock to full type safety is the build module.
This is built on #13620, and thus this is a draftmerged, ready for review.