Make the new options module fully type safe #13620

dcbaker · 2024-08-30T19:44:52Z

This fixes all of the issues in the options module, as well as fixing type safety issues introduced by it. This really falls into four parts:

fix the pre-existing issues with the UserOption classes
clean up some uses of Any that would hide real issues
Correct annotations for the OptionKey type that were broken
add missing annotations to the OptionStore class.

This fully supersedes #13616

mesonbuild/options.py

dnicolodi · 2024-09-01T21:16:44Z

mesonbuild/options.py

+        self.__choices: str = ', '.join(choices)
+
+    def printable_choices(self) -> T.Optional[T.List[str]]:
+        return [self.__choices]


Overloading printable_choices() to return a description of the valid range is a bit weird. The use of self.__choices makes it even less understandable. Would it make sense to move the computation of the string returned by printable_choices() into the method itself? I don't think that this code path is performance sensitive and requires caching the computation, but if it does, decorating the method with @functools.cache may be a better solution.

This is super important actually, because choices needs to be a Container[T], matching the type of the UserOption[T], but printable choices needs to be List[str] | None, regardless of what T is. The entire point of printable_choices is to fix the fact that we both need a string definable set of choices (like those used in Combo and Array options that are essentially a set of valid values, and the choices of integer options, where the constraints are a range of valid values. The status quo is that IntegerOption shoves '>=0, < 100' into choices and acts like it's cool, but then BooleanOption puts [True, False] in there.

Specifically, printable_choices gets used for printing the configuration and in the introspection code.

mesonbuild/coredata.py

mesonbuild/options.py

dcbaker · 2024-09-03T16:55:18Z

@dnicolodi: I've added an explicit method for checking the choices, so all of that is hidden inside the options module, which I think is a bit more elegant than having coredata poke at the internals of the UserOption classes. I've also added a Range[T] implementation. I'm not sure on it, it does make some things nicer, but its more code and makes other things more complex, so for now I haven't folded into the series itself, but left it separate so it can be evaluated separately.

dnicolodi · 2024-09-03T18:27:45Z

The suggestion to add the Range type was done in the optic of allowing to compare the choice member of enumerated options types and int option types transparently, thus it is somehow alternative to your choices_are_different() function. It does not add much value otherwise.

dnicolodi · 2024-09-03T18:38:58Z

mesonbuild/options.py

+    """
+    if isinstance(a, EnumeratedUserOption):
+        # If this isn't hte case the above failed somehow
+        assert isinstance(b, EnumeratedUserOption), 'for mypy'


Is mypy really this dumb? Isn't isinstance() recognized as a type narrowing check? I don't know what else is then...

It is a type narrowing check, but only for a, b is still left as _U, and even when I tried adding things like:

assert type(a) is type(b)

mypy wasn't smart enough to figure out that if a is EnumeratedUserOption that b would also be EnumeratedUserOption.

Oh, sorry, I misunderstood how type vars work when they reference unions. I though that if the same type vars appears in a function signature it is forced to resolve to the same concrete type. Evidently it does not work like this.

Maybe it should? It would in rust and C++ wouldn't it? Maybe I should open a typing issue about this.

mesonbuild/options.py

dcbaker · 2024-09-03T21:22:55Z

@dnicolodi fixed the typo, and dropped the range patch. I played around with getting better error messages, and just felt like it wasn't worth the amount of code involved.

bruchar1

This isn't directly related to this PR, but there is a lot of repetition for defining options, especially for the winlibs option. I wonder if a global dict or function should be used to prevent having a different description depending where the option is defined?

mesonbuild/compilers/cpp.py

bruchar1

Overall, looks good!

bruchar1 · 2024-09-06T19:11:12Z

mesonbuild/compilers/cpp.py

+        key = self.form_compileropt_key('eh')
+        opts[key] = options.UserComboOption(
+            self.make_option_name(key),
+            'C++ exception handling type.',


nitpick: this one ends with a ., while other options don't...

That has apparently always existed. The main branch right now has that peroid

bruchar1 · 2024-09-06T19:19:10Z

mesonbuild/options.py


    def init_option(self, name: 'OptionKey', value: T.Optional[T.Any], prefix: str) -> UserOption[_T]:
        """Create an instance of opt_type and return it."""
        if value is None:
            value = self.prefixed_default(name, prefix)
-        keywords = {'yielding': self.yielding, 'value': value}
+        keywords = {'yielding': self.yielding, 'value_': value}


Where does that value_ comes from?

Because to use dataclasses I had to rename the argument from value to value_, otherwise you can't convert the type. See python/mypy#15423, where I argued this was a bug but upstream argued it was the intended behavior.

Ah, but python doesn't need convertors for dataclasses, do we...

I'd laugh if the whole thing didn't make me so mad

mesonbuild/mintro.py

eli-schwartz · 2024-09-06T20:05:26Z

I cherry-picked a few (more) of these to master. Haven't taken a close look at the rest and it's getting late on Friday.

This provides a method to get choices for options in a printable form. The goal is to make refactoring options simpler.

This saves a *tiny* bit of typing, but at the cost of requiring either the current solution of throwing up our hands and saying "typing is too hard, better to have bugs!" or an extensive amount of `TypedDict`s, `overloads`, and a very new version of mypy. Let's get our type safety back, even if it means writing a little bit more code.

This reduces code, makes this clearer, and will be a nice step toward the goal of getting everything typesafe

This will allow us to take choices out of the UserOption class, which doesn't actually use this attribute.

Which wants a string, but then passes that string to a function that wants an OptionKey, which means that we'll always miss the lookup in BULITIN_DIR_NOPREFIX_OPTIONS, and return the default. The only case this gets used we cast an OptionKey to str, and then pass that. So instead, do the cast inside the function when necessary and pass the OptionKey

They are very similar, but they are not exactly the same. By splitting them we can get full type safety, and run mypy over the options.py file!

This allows us to hide type differences inside the options module, but still get accurate change information.

…on types The fact that UserOption is generic is really an implementation detail, not something to be used publicly. So by having an `AnyOptionType` alias, we can get better type checking, as can be seen by the patch as a whole. One of the big fixes it replace open-coded equivlalents of `MutableKeydOptionDictType` with that type alias.

eli-schwartz · 2024-11-01T04:12:29Z

mesonbuild/compilers/compilers.py

-    _T = T.TypeVar('_T')
    UserOptionType = T.TypeVar('UserOptionType', bound=options.UserOption)

+_T = T.TypeVar('_T')
+


This diff hunk confused me because it seems to be in the wrong commit.

eli-schwartz · 2024-11-01T04:19:49Z

mesonbuild/compilers/mixins/emscripten.py

@@ -63,7 +63,8 @@ def get_options(self) -> coredata.MutableKeyedOptionDictType:
        opts[key] = options.UserIntegerOption(
            self.make_option_name(key),
            'Number of threads to use in web assembly, set to 0 to disable',
-            (0, None, 4))  # Default was picked at random
+            4,  # Default was picked at random
+            min_value=0)


I think this may be a good case where it's worth explaining the change in the commit message. After thinking about this, I think that this works because the dataclass also fits a historic mess where the "value" member isn't consistently used by all option types, when we do stuff like overloading it with minimum/maximum values instead?

dcbaker requested review from mensinda and jpakkane as code owners August 30, 2024 19:44

dcbaker force-pushed the submit/fully-type-safe-options branch 2 times, most recently from 2fbff61 to 8881a19 Compare August 30, 2024 22:05

dnicolodi reviewed Sep 1, 2024

View reviewed changes

mesonbuild/options.py Show resolved Hide resolved

dnicolodi reviewed Sep 1, 2024

View reviewed changes

mesonbuild/coredata.py Outdated Show resolved Hide resolved

dnicolodi reviewed Sep 2, 2024

View reviewed changes

mesonbuild/options.py Show resolved Hide resolved

dcbaker force-pushed the submit/fully-type-safe-options branch from 8881a19 to 00ae5b5 Compare September 3, 2024 16:52

dcbaker force-pushed the submit/fully-type-safe-options branch from 00ae5b5 to 58437fe Compare September 3, 2024 18:07

dnicolodi reviewed Sep 3, 2024

View reviewed changes

mesonbuild/options.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/fully-type-safe-options branch from 58437fe to 6b288f8 Compare September 3, 2024 21:20

dcbaker force-pushed the submit/fully-type-safe-options branch from 6b288f8 to 60c9113 Compare September 4, 2024 16:34

bruchar1 added the typing label Sep 6, 2024

bruchar1 reviewed Sep 6, 2024

View reviewed changes

mesonbuild/compilers/cpp.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/fully-type-safe-options branch from 60c9113 to 0539929 Compare September 6, 2024 19:05

bruchar1 reviewed Sep 6, 2024

View reviewed changes

dcbaker force-pushed the submit/fully-type-safe-options branch from 0539929 to 9f0e17d Compare September 6, 2024 19:57

dcbaker force-pushed the submit/fully-type-safe-options branch from 9f0e17d to 8c97002 Compare September 9, 2024 16:39

thesamesam mentioned this pull request Sep 10, 2024

compilers: port stragglers to form_compileropt_key #13658

Closed

dcbaker added 5 commits October 1, 2024 08:56

options: fix the annotations of _to_tuple

f42f773

options: Add a printable_choices method to UserOption

13a4b19

This provides a method to get choices for options in a printable form. The goal is to make refactoring options simpler.

options: Get rid of the invalid _U type, and use UserOption[_T]

c89c599

options: use dataclasses for UserOption

aee5c19

This reduces code, makes this clearer, and will be a nice step toward the goal of getting everything typesafe

dcbaker added 5 commits October 1, 2024 08:56

options: Add an EnumeratedUserOption class

9c48b1c

This will allow us to take choices out of the UserOption class, which doesn't actually use this attribute.

options: split UserIntegerOption and UserUmaskOption

b6425df

They are very similar, but they are not exactly the same. By splitting them we can get full type safety, and run mypy over the options.py file!

options: Add a function to compare different option choices

58c4327

This allows us to hide type differences inside the options module, but still get accurate change information.

dcbaker force-pushed the submit/fully-type-safe-options branch from 8c97002 to 4a69ef0 Compare October 1, 2024 15:57

dcbaker mentioned this pull request Oct 1, 2024

Make coredata and environment fully type safe #13737

Draft

eli-schwartz reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the new options module fully type safe #13620

Make the new options module fully type safe #13620

dcbaker commented Aug 30, 2024

dnicolodi Sep 1, 2024

dcbaker Sep 3, 2024 •

edited

Loading

dcbaker commented Sep 3, 2024

dnicolodi commented Sep 3, 2024

dnicolodi Sep 3, 2024

dcbaker Sep 3, 2024

dnicolodi Sep 5, 2024

dcbaker Sep 5, 2024

dcbaker commented Sep 3, 2024

bruchar1 left a comment

bruchar1 left a comment

bruchar1 Sep 6, 2024

dcbaker Sep 6, 2024

bruchar1 Sep 6, 2024

dcbaker Sep 6, 2024

eli-schwartz Sep 6, 2024

dcbaker Sep 6, 2024

eli-schwartz commented Sep 6, 2024

eli-schwartz Nov 1, 2024

eli-schwartz Nov 1, 2024

Make the new options module fully type safe #13620

Are you sure you want to change the base?

Make the new options module fully type safe #13620

Conversation

dcbaker commented Aug 30, 2024

Choose a reason for hiding this comment

dcbaker Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

dcbaker commented Sep 3, 2024

dnicolodi commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker commented Sep 3, 2024

bruchar1 left a comment

Choose a reason for hiding this comment

bruchar1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-schwartz commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Sep 3, 2024 •

edited

Loading