[Discussion] Should "unused code" that are "useful" in a file be removed (or commented out)?

[Aleksanaa]

Case 1. unused with in maintainer list

Removing with lib.maintainers; [ ] because nixf-tidy complaints about with being unused. #330664

Opposed view: #330664 (comment) A package should have maintainers. A package that does not have a maintainer is a special case. Keeping this with can help reduce diff.

Supporting view: #330232 (comment) An exception should not be made here, it would make the rule unnecessarily complex.

Case 2. Unused helper functions in files for enumeration

In an enumerated file, a function that may be used needs to be removed or commented out because there is no use case for the time being, in order to pass nixf-tidy's check. https://github.com/NixOS/nixpkgs/pull/331017/files

Opposed view: Not being used is temporary, being used is normal, and the placed function has a documentation functionality.

Case 3. Unused argument in commonly used builder function

Arguments such as pname, version, src are not used directly, but are passed in as { ... }@args and args is further passed to underlying builder function. nixf-tidy suggests these arguments should be deleted. #330589

Opposed view: They can capture missing arguments as early as possible, and have certain documentation functionality, because this information can be obtained using functionArgs.

Supporting view: #330589 (comment) They can cause problems, such as making arguments with a default value to be actually undefined (example: in doInstallCheck = attrs.doCheck or true;, doInstallCheck is true if doCheck is not passed as an argument, even if doCheck defaults to false); there are better ways to write them; functionArgs is not good documentation and should be replaced by doc-comments in the future.

CC @inclyc @SuperSandro2000 @AndersonTorres @piegamesde @Mic92 @mweinelt @jian-lin

[Aleksanaa]

Note: nixf-tidy currently lacks ignoring flags (in comments), but this is a planned feature.

[ShamrockLee]

Thank you for bringing this up!

Cc: @fricklerhandwerk @TomaSajt

[Sigmanificient]

Hi, I'm trying to manually get rid of most unused arguments (#313981). So far removed about 1700 of them (2000 if you count the opened pr).

I use deadnix to find them, and it also have the same issue with @args. Currently sorting them by hand, but I think a bit of parsing could avoid 99% of the bad scenarios. Files that are not default.nix are the second source of false positives

[pbsds]

My 0.5 NOK:

1. I read maintainers = with lib.maintainers; [ ]; as a friendly open invitation, while maintainers = [ ]; as a sad state of reality. I want people to join the project hence I very much prefer the former.
2. If such a function is commented out i am much more likely to not find it if needed. I prefer dead code whose affordance is obvious, over dead comments which requires people who remember the history to stick around and make the decision of reviving or removing the dead code comment.
3. The moment { ... }@args is a part of the picture, static analysis tend to fail. This is at least my experience with deadnix. nixf-tidy must detect this and silence false warnings, at least when run in nixpkgs CI.

[SuperSandro2000]

Having to comment the code is IMO the solution we definitely do not want. We have git to recover the code if it was removed. Then the question remains if we want to globally drop unused code.

[piegamesde]

An exception should not be made here, it would make the rule unnecessarily complex.

On a slightly philosophical note: Tools should be there to serve and accommodate people. Therefore as long as it can be implemented within reasonable effort, questions should always be answered from the perspective of "what do the humans want", not "what has a simple implementation". We live in a messy world, navigating it necessarily imbues a lot of complexity.

[AndersonTorres]

1. I read maintainers = with lib.maintainers; [ ]; as a friendly open invitation, while maintainers = [ ]; as a sad state of reality.

We have many instances of packages maintained by a single silent-retired being - see #290642.
This is a sadder state of reality, imho.

Further, I could argue that the biggest barrier to being a Nixpkgs maintainer is having a GitHub account.

To not derail this conversation, I refer to this link: #327779

[philiptaron]

Main point

Tidying is a needed and necessary job. Following on from @piegamesde's philosophical point, humans do better work when non-essential details are removed from view, and inactive code is as non-essential as it gets. Yet the work of tidying often turns into toil. If a tool helps with this goal, it's helping us bridge between two good aims which otherwise would be in tension.

Case 1️⃣

Removing the with seems straightforwardly good. Why leave odd-looking superstructure in place for the future? Let the future re-add it when needed. This tool is called tidy. Let it do the work of tidying without complexity of extra rules.

Case 2️⃣

On this specific case: why should every aliases file re-implement a similar function? Extract the helper to some more relevant location, then use it when needed.

On the general case, if there's some social expectation when editing a file, having a comment seems like the right place to land that, and having code inside that comment is no bad thing. The most important thing is to communicate the social expectation. Don't just comment out code and leave the social expectation of what to do with that code out, please!

Case 3️⃣

I feel quite complicated about this case. Behavior that "lights up" if you pass an argument that's not in the parameters (as shown by functionArgs) is pretty spooky, especially if it's several layers deep (triggered inside of a // args block, for instance).

In Python, I advocate avoiding **kwargs for similar reasons. It's too hard to document what's needed and what's not. If you have a choice, use simple parameter passing. The extra glue code required to pass through parameters is worth the documentation effort, in my estimation.

On the other hand, we live in a complicated world in nixpkgs, and it's quite certainly the case that there will of necessity be action at a distance as two orthogonal features are combined together. The use of @args and ... may be necessary to combine those features! And the subtlety of the example, where a default was made ineffective through code removal, makes me believe that this may be an example of only "mostly dead" code.

[jian-lin]

Case 3. Unused argument in commonly used builder function

Arguments such as pname, version, src are not used directly, but are passed in as { ... }@args and args is further passed to underlying builder function. nixf-tidy suggests these arguments should be deleted. #330589

Opposed view: ...

Supporting view: #330589 (comment) They can cause problems, such as making arguments with a default value to be actually undefined

This part of the supporting view is weak.

The linked example (#330589 (comment)) and potential problems do not apply to the mentioned unused arguments pname, version and src because presumably these three arguments do not use the pattern of the linked example. Actually, I do not find any usage of such pattern for any of these three arguments in Nixpkgs.

To be clear, the pattern is to provide a default value for an argument ({ pname ? "foo" }@args: ...) and try to use that default value through args. (in bar = args.pname or "foobar", bar is "foobar" if pname is not passed to the function).

[inclyc]

To be clear, the pattern is to provide a default value for an argument ({ pname ? "foo" }@args: ...) and try to use that default value through or (bar = args.pname or "foobar").

So maybe this should be identified in another tidy check.

[Aleksanaa]

So the fix here I guess is:

1. Don't report unused with when suffixed by a empty list [ ] of set { }, as they are obvious and are less likely to cause a problem, and if they cause a problem after some edits, we can still catch that in other checks such as escaping with.
2. Don't report unused argument when using together with @args pattern, but do report unused argument if they have a default value in this case (the default value won't take effect, as noted above)
3. Adding ignoring flags, this needs refactoring the parser so may need to wait a bit longer

I would like to remind everyone that inclyc is currently unable to escape due to some serious matters, so there may not be any progress this week or next week. At present, CI is still working relatively well and can detect more than 10 incorrect writings a day, so the status quo should be maintained for the time being. For the few PRs that have errors due to 2 and 3, if there is nothing inappropriate, please remind them to ignore the errors first.

[TomaSajt]

Was reminded of the Case 3 situation about default values while working on one of my PRs and wanted to write down some thoughts. (Maybe there's a more appropriate place to post this, but I couldn't find it.)

I've realized that there's an alternative that is much simpler to mentally process than the ?-based default values.

Let's say you wanted to write a wrapper for mkDerivation that just applies some default values.

Instead of doing

{ stdenv }:

{
  a ? "a_default",
  b ? "b_default",
}@args:

stdenv.mkDerivation args

which wouldn't work, since the defaults don't get inherited by args,
you would instead do

{ stdenv }:

_args:

let
  args = {
    a = "a_default";
    b = "b_default";
  } // _args;
in

stdenv.mkDerivation args

In my experience it's simply wrong to combine ? with @. Using only one of the two is fine, but together they are just too error-prone.

---

Note: this example didn't have any required attributes.
You could do { c, ... }@args: if c was required.
Or maybe just keep args: and add c = throw "c value needs to be set!" to the defaults, though that might be a bit too weird.