Refactor and fix typing in coredata #11492

dcbaker · 2023-03-03T19:05:49Z

The typing (and some of the inheritance model) of the UserOptions was just wrong (I didn't handle generics right the first time). I've simplified both the UserOption and BuiltinOption classes, converted them to dataclasses (yay dataclasses!) and then cleaned up the remaining typing issues. There is some churn because I normalized the constructors for the UserOptions, which changed the order of ComboOptions to match that of the other Option types.

mesonbuild/coredata.py

-        keywords = {'yielding': self.yielding, 'value': value}
+
+        if T.TYPE_CHECKING:
+            class KeywordDict(T.TypedDict, total=False):


mesonbuild/coredata.py

        return self.default

    def add_to_argparse(self, name: str, parser: argparse.ArgumentParser, help_suffix: str) -> None:
-        kwargs = OrderedDict()
+        if T.TYPE_CHECKING:
+            class ArgumentKW(T.TypedDict, total=False):


codecov · 2023-03-03T19:14:31Z

Codecov Report

Merging #11492 (e4dd8d1) into master (95b03f7) will decrease coverage by 2.41%.
The diff coverage is n/a.

❗ Current head e4dd8d1 differs from pull request most recent head cb48934. Consider uploading reports for the commit cb48934 to get more accurate results

@@            Coverage Diff             @@
##           master   #11492      +/-   ##
==========================================
- Coverage   71.24%   68.83%   -2.41%     
==========================================
  Files         212      209       -3     
  Lines       46390    45583     -807     
  Branches    11021     9439    -1582     
==========================================
- Hits        33049    31377    -1672     
- Misses      10881    11782     +901     
+ Partials     2460     2424      -36

see 421 files with indirect coverage changes

tristan957 · 2023-03-03T19:16:07Z

coredata: fix a remaining type issues

Do you mean all instead of a?

tristan957 · 2023-03-03T19:16:48Z

Then if you're really nitpicky, one of the commits is refactor while the other is Refactor, but nice job! :)

eli-schwartz

last commit message: "inisde" should be "inside"

mesonbuild/coredata.py

eli-schwartz · 2023-03-09T01:52:41Z

mesonbuild/coredata.py

+
+    def __post_init__(self) -> None:
+        # This should be possible with an `InitVar`, but it doesn't seem to work
+        # with mypy 0.991.


What's the problem? Is it fixed with mypy 1.0?

if you have:

@dataclass class C(Generic[_T]): value: InitVar[_T] def __post_init__(self, value: _T): self.value = value

It becomes convinced that C doesn't have an attribute value. I figured out I can work around this with value_: InitVar[_T], so I'll do that.

mesonbuild/coredata.py

eli-schwartz · 2023-03-09T02:11:56Z

mesonbuild/coredata.py

-        super().__init__(description, [True, False], yielding, deprecated)
-        self.set_value(value)
+
+    choices: T.List[bool] = field(default_factory=lambda: [True, False])


Do we need a factory field here (and elsewhere)? AFAICT we never modify choices in place, only assign them as = newthing, and never for boolean or umask anyway. Can typing_extensions.Final help here?

Particularly given this is a static choice on the option type, I feel like this should be a ClassVar, but apparently you cannot mix ClassVar and Final: https://bugs.python.org/issue45384

Pity. But personally I'd still go with a ClassVar I guess.

This is all really annoying to solve because with dataclasses you can't forward declare an attribute. I've done my best here by adding an invisible attribute for choices, and then overriding it all of the subclasses. The problem is that without it being defined in UserOption, UserOption can't be used as a generic interface, and mypy says "hey, that dict of [str: UserOption] doesn't have .choices!"

Actually, I realized I'm wrong, you can forward declare them, you just have to do it the old-school way.

@dataclass class C: x: int if typing.TYPE_CHECKING: y: str

But it doesn't help us in this case

eli-schwartz · 2023-03-09T02:31:32Z

mesonbuild/coredata.py

@@ -190,27 +191,34 @@ def printable_value(self) -> str:
    def validate_value(self, value: T.Any) -> T.Union[str, OctalInt]:
        if value is None or value == 'preserve':
            return 'preserve'
-        return OctalInt(super().validate_value(value))
+        if isinstance(value, str):


I haven't investigated for myself, but what's the reason we can't inherit from integer options anymore and have to copy this over?

Because in that case we end up with conflicting types for _T, UserIntegerOption requires that _T is int while UserOption[str | OctalInt] requires _T in [str, OctalInt], so you end up with things like validate_value must simultaneously return an int and str | OctalInt, which is obviously impossible.

Yay... and I guess using a mixin or UserIntegerOption.validate_value(self, value) feel too hacky or something?

I didn't think about using a mixin... that would work

mesonbuild/coredata.py

dcbaker · 2023-03-09T20:52:22Z

I haven't fixed everything, but I've tried to fix the choices. It's a bit of a challenge because of data classes though. I think the most elegant fix would require dropping them

eli-schwartz · 2023-03-09T21:54:43Z

mesonbuild/coredata.py

+    toint: T.Callable[[str], int]
+
+    # This has to be done without super() because the two consumers have
+    # differen types, and mypy gets grumpy in UserUmaskOption that


Suggested change

# differen types, and mypy gets grumpy in UserUmaskOption that

# different types, and mypy gets grumpy in UserUmaskOption that

eli-schwartz · 2023-03-10T00:19:40Z

mesonbuild/coredata.py

+
+    def __post_init__(self, value: str) -> None:
+        if not self.choices or not all(isinstance(c, str) for c in self.choices):
+            raise MesonException('Combo option must have choices, which must be a list, and must not be empty')


a list

The previous code was more meson-style, which is to say it referred to an array.

eli-schwartz · 2023-03-10T00:27:51Z

mesonbuild/coredata.py

+
+    choices: T.List[str] = field(default_factory=list)
+    split_args: bool = False
+    user_input: bool = False


I think we only need user_input here to pass it to validate_value during post_init, which we currently don't do because we rely on the post_init from UserOption.

I think I have fixed this on the next push

eli-schwartz · 2023-03-10T00:30:27Z

mesonbuild/optinterpreter.py

@@ -266,10 +265,12 @@ def integer_parser(self, description: str, args: T.Tuple[bool, _DEPRECATED_ARGS]
    def string_array_parser(self, description: str, args: T.Tuple[bool, _DEPRECATED_ARGS], kwargs: StringArrayArgs) -> coredata.UserOption:
        choices = kwargs['choices']
        value = kwargs['value'] if kwargs['value'] is not None else choices
+        if isinstance(value, str):
+            value = [value]


Should this be a convertor for the KwargInfo?

We can still hit this from -Darray-opt='foo' I think

I tested this code path with meson setup -Darray=foo,bar with an assert on the first line of the function. Was not hit. Not sure how this function is called in practice.

I am an idiot. This code path is no longer hit. -Darray-opt=item is a list by the time we are here.

eli-schwartz · 2023-03-10T00:33:44Z

mesonbuild/coredata.py

-    def printable_value(self) -> str:
-        if self.value == 'preserve':
+    def printable_value(self) -> StringProtocol:
+        if isinstance(self.value, str):
            return self.value
        return format(self.value, '04o')


Is mypy not clever enough here to notice that in all cases of a non-str, it falls through to format()?

yeah, because we haven't actually annotated value to be Literal['presever'] | int. Maybe we can try that now, actually...

I tried this, but ended up breaking mypy somehow. Needs more investigating...

This is kinda challenging, because we expect to be able to pass in string values representing octals like '022' in install_umask. I don't think we can have our cake and eat it too.

eli-schwartz · 2023-03-10T01:13:31Z

mesonbuild/coredata.py

@@ -668,15 +680,16 @@ def get_option(self, key: OptionKey) -> T.Union[T.List[str], str, int, bool, Wra
            if v.yielding:
                if key.name == 'wrap_mode':
                    return WrapMode[v.value]
-                return v.value
+                return T.cast(T.Union[T.List[str], str, int, bool], v.value)


... and here.

eli-schwartz · 2023-03-10T01:15:44Z

mesonbuild/coredata.py

-    def get_nondefault_buildtype_args(self):
-        result = []
+    def get_nondefault_buildtype_args(self) -> T.List[T.Union[T.Tuple[Literal['optimization'], str, str], T.Tuple[Literal['debug'], bool, bool]]]:
+        result: T.List[T.Union[T.Tuple[Literal['optimization'], str, str], T.Tuple[Literal['debug'], bool, bool]]] = []


This is long enough and difficult enough to read, just because of the annotation, that I would like to see it defined as a readable type in T.TYPE_CHECKING and then reused here, even if it was only used once. :D But lucky us, we can use it twice.

Done, but feel free to bikeshed the name

eli-schwartz · 2023-03-10T01:22:59Z

mesonbuild/coredata.py

@@ -783,15 +803,15 @@ def _set_others_from_buildtype(self, value: str) -> None:

    @staticmethod
    def is_per_machine_option(optname: OptionKey) -> bool:
-        if optname.name in BUILTIN_OPTIONS_PER_MACHINE:
+        if optname in BUILTIN_OPTIONS_PER_MACHINE:


Okay, so this is buggy... but what did it do before? The function is used twice, once in mintro (I do not care what that does), and once in environment.py -- in the latter case, it is being used to "Keep only per machine options from the native file" Not sure I understand the ramifications of changing this.

eli-schwartz · 2023-03-10T01:23:28Z

mesonbuild/coredata.py

            return True
        return optname.lang is not None

    def get_external_args(self, for_machine: MachineChoice, lang: str) -> T.List[str]:
-        return self.options[OptionKey('args', machine=for_machine, lang=lang)].value
+        return T.cast(T.List[str], self.options[OptionKey('args', machine=for_machine, lang=lang)].value)


String-quote first argument to T.cast.

eli-schwartz · 2023-03-10T01:24:53Z

mesonbuild/coredata.py

@@ -1193,13 +1230,21 @@ def argparse_name_to_arg(name: str) -> str:
    def prefixed_default(self, name: 'OptionKey', prefix: str = '') -> _T:
        if self.opt_type is UserStringOption:
            try:
-                return BULITIN_DIR_NOPREFIX_OPTIONS[name][prefix]
+                # _T is str in this case, but mypy can't figure that out
+                return T.cast(_T, BULITIN_DIR_NOPREFIX_OPTIONS[name][prefix])


T.cast first argument.

This does a ton of cleanup to the UserOption classes. They are converted into dataclasses, and much of their constructor logic is removed, and what is left is simplified. This also cleans up their typing considerably

This is again, simplified, and the typing has been fixed

For one, they have a BuiltinOption not a UserOption, and for two, they shouldn't be mutable (at least the dicts themselves shouldn't)

There are a lot of them in here. Some of them mypy is spotting legitimate Generic deviations but that is in part because we can't tell mypy what we really mean: listify for example should always return a `List[T where T is scalar]`, so but the strict generics of an array option would be `List[List[T]]`, so we just have to sprinkle in some `type: ignore` comments

dcbaker · 2023-06-05T23:15:16Z

I did a rebase, and @tristan957 helped with some of the review comments. Thanks Tristan!

dcbaker added refactoring No behavior changes typing labels Mar 3, 2023

dcbaker requested a review from jpakkane as a code owner March 3, 2023 19:05

github-advanced-security bot found potential problems Mar 3, 2023

View reviewed changes

dcbaker force-pushed the submit/type-coredata branch from 96d8f14 to 2e721cc Compare March 3, 2023 19:31

eli-schwartz reviewed Mar 9, 2023

View reviewed changes

dcbaker force-pushed the submit/type-coredata branch from 2e721cc to 7506f19 Compare March 9, 2023 20:49

dcbaker force-pushed the submit/type-coredata branch 3 times, most recently from 288b9d8 to ef878a8 Compare March 9, 2023 21:43

eli-schwartz reviewed Mar 10, 2023

View reviewed changes

dcbaker added 6 commits June 5, 2023 16:11

coredata: refactor the UserOption classes

7ffa1c5

This does a ton of cleanup to the UserOption classes. They are converted into dataclasses, and much of their constructor logic is removed, and what is left is simplified. This also cleans up their typing considerably

coredata: refactor the BuiltinOption class

87ad6bd

This is again, simplified, and the typing has been fixed

coredata: fix type annotations for builtin options

f30bb97

For one, they have a BuiltinOption not a UserOption, and for two, they shouldn't be mutable (at least the dicts themselves shouldn't)

run_mypy: add coredata.py

1ec924f

coredata: flatten nested if tree inisde else clause

cb48934

dcbaker force-pushed the submit/type-coredata branch from ef878a8 to cb48934 Compare June 5, 2023 23:14

tristan957 added this to the 1.3.0 milestone Jul 13, 2023

xclaesse modified the milestones: 1.3.0, 1.4.0 Oct 17, 2023

bruchar1 removed this from the 1.4.0 milestone May 2, 2025

bruchar1 mentioned this pull request May 2, 2025

Tracking Issue for type safety PRs #14214

Open

22 tasks

	# differen types, and mypy gets grumpy in UserUmaskOption that
	# different types, and mypy gets grumpy in UserUmaskOption that

Uh oh!

Refactor and fix typing in coredata #11492

Are you sure you want to change the base?

Refactor and fix typing in coredata #11492

Uh oh!

Conversation

dcbaker commented Mar 3, 2023

Uh oh!

Check notice

Check notice

codecov bot commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tristan957 commented Mar 3, 2023

Uh oh!

tristan957 commented Mar 3, 2023

Uh oh!

eli-schwartz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcbaker Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcbaker commented Mar 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 3, 2023 •

edited

Loading

dcbaker Mar 9, 2023 •

edited

Loading