Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/modules: Use options apply function even if no values are defined #66407

Merged
merged 1 commit into from Aug 27, 2019

Conversation

infinisil
Copy link
Member

Motivation for this change

This allows apply functions to return a valid value if they completely
ignore their argument, which is the case for the option renaming
functions like mkAliasOptionModule. Therefore this fixes #63693, meaning the more intrusive fix in #63719 won't be needed

This is a 100% guaranteed backwards compatible change (aka, what succeeded previously will continue to succeed) and doesn't increase the module API surface

Ping @nbp @Profpatsch @j-piecuch @rycee

Things done
  • Ran lib/tests/modules.sh successfully

This allows `apply` functions to return a valid value if they completely
ignore their argument, which is the case for the option renaming
functions like `mkAliasOptionModule`. Therefore this solves issue NixOS#63693
@infinisil
Copy link
Member Author

infinisil commented Aug 9, 2019

Nix proof to eliminate any doubt of this breaking anything :)

Proof of backwards-compatibility: If value succeeded evaluation before, it still evaluates after, and to the same value:

Let's look at the old definition:

if !res.isDefined then
  throw "The option `${showOption loc}' is used but not defined."
else if opt ? apply then
  opt.apply res.mergedValue
else
  res.mergedValue

Since we can assume that it succeeded before, the first case can't happen, so res.isDefined == true. Thus the result of this under this assumption is

if opt ? apply then
  opt.apply res.mergedValue
else
  res.mergedValue

Now let's show that with res.isDefined == true, the new code also evaluates to that. The new definition is

let
  valueDefined = if res.isDefined then res.mergedValue else
    throw "The option `${showOption loc}' is used but not defined.";
in
  if opt ? apply then opt.apply valueDefined else valueDefined

Insert the value true of res.isDefined and apply evaluate the if clause:

let
  valueDefined = res.mergedValue;
in
  if opt ? apply then opt.apply valueDefined else valueDefined

Inline:

if opt ? apply then
  opt.apply res.mergedValue
else
  res.mergedValue

Which is the same as before. QED

Copy link
Member

@rycee rycee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple and elegant solution, I like it!

Maybe add a test case that exercises the case from #63693?

Copy link
Member

@nbp nbp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change sounds fine, by I disapprove the proposed solution for #63693 , which from my point of view, should not be fixed. I will comment on this issue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mkAliasOptionModule doesn't work if the newly created option doesn't have any definitions
3 participants