meson icon indicating copy to clipboard operation
meson copied to clipboard

Make the new options module fully type safe

Open dcbaker opened this issue 1 year ago • 4 comments

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:

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

This fully supersedes #13616

dcbaker avatar Aug 30 '24 19:08 dcbaker

@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.

dcbaker avatar Sep 03 '24 16:09 dcbaker

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 avatar Sep 03 '24 18:09 dnicolodi

@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.

dcbaker avatar Sep 03 '24 21:09 dcbaker

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.

eli-schwartz avatar Sep 06 '24 20:09 eli-schwartz

This probably conflicts with option refactor quite badly. Which one should be merged first?

jpakkane avatar Jan 30 '25 00:01 jpakkane

I think this one, and then following that: https://github.com/mesonbuild/meson/pull/13737. Which would give us a really solid base of typing guarantees to build the option refactor on top of so that we have a fairly good idea that things work across the changes. I've actually started doing that rebase on top of 13737, and I've found several issues already. I'll try to push that work upstream somewhere we can look at it after a bit, right now I need to go start dinner.

I'm also saying that because I'll put my money where my mouth is. Having mypy typing catches lots of real issues before they go to production.

dcbaker avatar Jan 30 '25 00:01 dcbaker

How ready is this for merging?

jpakkane avatar Feb 03 '25 11:02 jpakkane

From my perspective I've addressed all of the review feedback, with the exception of the question about __post_init__ vs init=False. I have a preference but if the wider consensus is to go the other way I can do that.

dcbaker avatar Feb 03 '25 18:02 dcbaker

I don't really have a preference on that, I just want this in so optionrefactor can proceed.

jpakkane avatar Feb 04 '25 00:02 jpakkane

Lint failure is unrelated.

jpakkane avatar Feb 05 '25 15:02 jpakkane