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

Add the tool "nixos-typecheck" that can check an option declaration to: #13607

Closed
wants to merge 1 commit into from

Conversation

ts468
Copy link
Contributor

@ts468 ts468 commented Mar 1, 2016

Add the tool "nixos-typecheck" that can check an option declaration to:

Enforce that an option declaration has a "defaultText" if and only if the
type of the option derives from "package", "packageSet" or "nixpkgsConfig"
and if a "default" attribute is defined.

Enforce that the value of the "example" attribute is wrapped with "literalExample"
if the type of the option derives from "package", "packageSet" or "nixpkgsConfig".

Warn if a "defaultText" is defined in an option declaration if the type of
the option does not derive from "package", "packageSet" or "nixpkgsConfig".

Warn if no "type" is defined in an option declaration.

Remove all errors that were found by "nixos-typecheck".

 - Enforce that an option declaration has a "defaultText" if and only if the
   type of the option derives from "package", "packageSet" or "nixpkgsConfig"
   and if a "default" attribute is defined.

 - Enforce that the value of the "example" attribute is wrapped with "literalExample"
   if the type of the option derives from "package", "packageSet" or "nixpkgsConfig".

 - Warn if a "defaultText" is defined in an option declaration if the type of
   the option does not derive from "package", "packageSet" or "nixpkgsConfig".

 - Warn if no "type" is defined in an option declaration.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @urkud and @nbp to be potential reviewers

ts468 referenced this pull request Mar 1, 2016
 - Enforce that an option declaration has a "defaultText" if and only if the
   type of the option derives from "package", "packageSet" or "nixpkgsConfig"
   and if a "default" attribute is defined.

 - Enforce that the value of the "example" attribute is wrapped with "literalExample"
   if the type of the option derives from "package", "packageSet" or "nixpkgsConfig".

 - Warn if a "defaultText" is defined in an option declaration if the type of
   the option does not derive from "package", "packageSet" or "nixpkgsConfig".

 - Warn if no "type" is defined in an option declaration.
@jagajaga
Copy link
Member

jagajaga commented Mar 1, 2016

cc @oxij

@domenkozar domenkozar added this to the 16.09 milestone Mar 2, 2016
# A module option taking a function can not be introspected and documented properly.
functionTo = resultType:
mkOptionType {
name = "function to ${resultType.name}";
Copy link
Member

Choose a reason for hiding this comment

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

Never again, I shall see this thing coming back to life!
Whoever need this should burn in misery under the flames of my fury!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm absolutely with you on that! The problem is that functions are currently used as options in a few modules. So my plan was to name them first, and then to deprecate the use of functions as options in a next step.

@nbp
Copy link
Member

nbp commented Mar 13, 2016

@ts468 ,

The suggested change is highly intrusive. I can understand the will of adding the typerep attribute, such that we can better way of handling time in a computer friendly manner. If you want to make another pull-request with the typerep attribute, that would be fine.

But I do not understand what is this type inference story or how it influence in plays in the module evaluation.

All values, definitions, declarations are available for introspection without the need to make such invasive changes. This is how nixos-option is made, as an external tool. If we are looking at a sanitizer / linter tool, I do not see why we would need such instrumentation.

then inferredType mode /* Set to 'true' to see every type that is being inferred, not just those types that result in 'unspecified'. */
else if mode != "silent" then builtins.trace noInferDoc types.unspecified else types.unspecified;


Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nbp
I assume you're referring to this function, and all the changes needed to add it, as the quite intrusive one?

Ideally, we would reject any option without an explicit type definition. This function is just there to help us getting towards explicitly defined option types by showing all the options that need to be typed.

So my plan was to add this function temporarily, and to switch the default value from 'silent' to 'printUnspecified' to 'printAll' over time, and then to replace the function with a hard error in case an untyped option is found.

I would prefer to have that enforcing mechanism within the module system directly, instead of in an external tool. And I thought this would be the right place for it. What do you think?

@ts468
Copy link
Contributor Author

ts468 commented Mar 14, 2016

@nbp
Thank you very much for reviewing that PR. It's highly appreciated!

So what I want to achieve is to make sure that all options of the module system are permanently checked to always have a 'type'. And if an option contains a derivation, it should also be checked to use 'defaultText' and 'literalExample' if a default or example is given.
The reason I wanted to tie such checks into the module system is to enforce that the module options are always fully specified. I think having that guarantee on the module options is important for external tools processing them.

So I don't know whether that approach is the best one, and I'm very open for any suggestions on how to ensure the options are fully specified.

@nbp
Copy link
Member

nbp commented Mar 14, 2016

@ts468
I think you should have a look at

(import <nixos> {}).options.boot.consoleLogLevel.type

And expose anything you need this way.

From what I understand, making this as part of the module system adds overhead that every user will have to go through, while having an external tool add such overhead only when we are checking for correctness, such as within hydra, or with nox-review.

@ts468
Copy link
Contributor Author

ts468 commented Mar 15, 2016

@nbp
Thank you very much for your help!

Could you maybe explain a little bit more on what you mean by exposing everything I need similarly like:

(import <nixos> {}).options.boot.consoleLogLevel.type

I'm afraid I don't see it myself.

Concerning the overhead for every user, I actually hoped that the implementation would keep that quite low.
The checking for proper use of 'defaultText' and 'literalExample' is done the same way as extracting option details for the documentation. Doesn't that mean that it's only evaluated when typechecker in nixos/default.nix is evaluated?
For the inference function that checks whether a nixos module option has a type attribute, that function should only be evaluated if _module.typeInference is changed from its default value. At least that is what I hoped the module system would do :)
I haven't done any measurements of the change in evaluation time, but I thought it shouldn't be significant. Do you think I'm wrong with that?

@nbp
Copy link
Member

nbp commented Mar 16, 2016

Yes, basically nixos provide a config attribute and an options attributes, The config attribute is what we are used to see as argument of modules, but the options attributes is the meta aspect of it, and allow us to pull all the definitions / declarations of the options, and give extra information about them.

What I suggesting, is that yoiu modify options under the options attributes, such that they expose all the information which is needed for an external tool, as we do today for nixos-option tool, which mostly read the options attribute to collect the location of the definitions and the declarations.

This change should not be intrusive, as they will only collect information, and should require only minimal changes to modules.nix file (no more that 10 lines of changes).

@ts468
Copy link
Contributor Author

ts468 commented Mar 17, 2016

@nbp
Smart! :) Thank you very much! I think I'm slowly getting there...

@stale
Copy link

stale bot commented Jun 2, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@infinisil infinisil added the 6.topic: module system About NixOS module system internals label Sep 17, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@AndersonTorres
Copy link
Member

Is this still relevant?

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.

None yet

10 participants