nil icon indicating copy to clipboard operation
nil copied to clipboard

Wrong unused binding

Open viperML opened this issue 1 year ago • 6 comments

{
  hello,
  symlinkJoin,
  lib,
} @ inputs:
symlinkJoin {
  name = "foo";
  paths = builtins.filter lib.isDerivation (builtins.attrValues inputs);
}

nil reports an unused_binding on hello, while it is not the case.

viperML avatar Aug 19 '24 09:08 viperML

hello is unused. The fact that inputs.hello does not exist without the explicit hello in the argset due to being callPackageed doesn't change that.

NobbZ avatar Aug 19 '24 11:08 NobbZ

You can evaluate this expression without callPackage or builtins.functionArgs with import ./test.nix (with import <nixpkgs> {}; {inherit hello symlinkJoin lib;})

viperML avatar Aug 19 '24 11:08 viperML

Either way hello is not used, and you'd need ... to signify "and others", you then can not use callPackage anymore.

Point remains, hello is unused currently and nil is rightfully complaining.

NobbZ avatar Aug 19 '24 13:08 NobbZ

The problem here is: {a, unused }@arg: a, the formal[^formal] unused might be used later by accessing arg, but the language server cannot precisely track it.

In libnixf we treat { a, unused }@arg: a differently from { a, unused }: a

See the image below:

Case 1: maybeReferenced could be referenced later from c, thus this is a "Hint". It causes the editor rendering faded text. Case 2: b is definitely unused, thus this is a "Warning". Hinting the user to fix that.

Either way hello is not used, and you'd need ... to signify "and others", you then can not use callPackage anymore.

That's the reason why hello is used in this case. Unused things are something we can safely remove without further modifications. If it affects the usage of callPackage, then hello is actually used (but yes, it is not "directly used", by its name).

[^formal]: In Nix language parser (official) it is called "formal", but not very documented.

inclyc avatar Aug 19 '24 14:08 inclyc

I do agree that this might be or not an issue depending on how we define "using"...

viperML avatar Aug 19 '24 16:08 viperML

In {bar}@foo: foo.bar, the binding bar is unused. The value of it is not unused.

I do see, that the use of the unused binding in the pattern slightly improves the error message in case of calling the function wrong, I also see the benefits in using Vipers original example and how it enables the usage of callPackage where it wouldn't be possible otherwise.

Still, I consider the warning "unused binding" correct, as the binding itself remains unused.

I would prefer an explicit "#ignore: unused" magic comment or so, rather than implicit logic in nil (or the supporting library) that would heuristically decide whether or not a bindings value was used by the use of the encapsulating attribute set.


I do agree that this might be or not an issue depending on how we define "using"...

As seen above, its not about how we define "unused", it is about the object it describes.

NobbZ avatar Aug 20 '24 09:08 NobbZ

Is it unused binding? Yes. But if you remove it doesn't eval properly.

I found a couple of cases in nixpkgs in the rocm section.

EDIT: specifically rocfft

lucasew avatar Jan 07 '25 13:01 lucasew

Can you please point out exact sources in nixpks where removal of an unused pattern would break an otherwise working expression?

As said, I see that there might be an improved error reporting in the case of existence of the binding in the pattern if its not being passed in.

Though technically this binding remains unused.

NobbZ avatar Jan 07 '25 19:01 NobbZ

Can you please point out exact sources in nixpks where removal of an unused pattern would break an otherwise working expression?

https://github.com/NixOS/nixpkgs/blob/ab84748075f1ee255688205e7694d123160e1c19/pkgs/development/rocm-modules/5/rocfft/default.nix#L2

https://github.com/NixOS/nixpkgs/blob/ab84748075f1ee255688205e7694d123160e1c19/pkgs/development/rocm-modules/6/rocfft/default.nix#L2

lucasew avatar Jan 07 '25 22:01 lucasew

@lucasew again, unused, after removing it and fixing the callsite, the result is still the same…

$ nix-instantiate -A rocmPackages_5.rocfft
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
/nix/store/3yy6539jywm7cgwzn0ym9ga3bqvly9aa-rocfft-5.7.1.drv
$ nvim .
diff --git a/pkgs/development/rocm-modules/5/default.nix b/pkgs/development/rocm-modules/5/default.nix
index c1ffd01c8061..7f9a9bbe9338 100644
--- a/pkgs/development/rocm-modules/5/default.nix
+++ b/pkgs/development/rocm-modules/5/default.nix
@@ -156,7 +156,7 @@ in rec {
   hiprand = rocrand; # rocrand includes hiprand

   rocfft = callPackage ./rocfft {
-    inherit rocmUpdateScript rocm-cmake rocrand rocfft clr;
+    inherit rocmUpdateScript rocm-cmake rocrand clr;
     inherit (llvm) openmp;
     stdenv = llvm.rocmClangStdenv;
   };
diff --git a/pkgs/development/rocm-modules/5/rocfft/default.nix b/pkgs/development/rocm-modules/5/rocfft/default.nix
index d7a0d0e81ba8..43183da77baa 100644
--- a/pkgs/development/rocm-modules/5/rocfft/default.nix
+++ b/pkgs/development/rocm-modules/5/rocfft/default.nix
@@ -1,5 +1,4 @@
 {
-  rocfft,
   lib,
   stdenv,
   fetchFromGitHub,
$ nix-instantiate -A rocmPackages_5.rocfft
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
/nix/store/3yy6539jywm7cgwzn0ym9ga3bqvly9aa-rocfft-5.7.1.drv

So the binding is indeed unused and does not have any effect on the returned derivation.

NobbZ avatar Jan 08 '25 19:01 NobbZ

Try to run that parallel eval workflow that CI uses.

lucasew avatar Jan 08 '25 20:01 lucasew

Which are you talking about?

Again, the bindings are obviously unused, and as we do not have an @-pattern, they are not even helpers as in OPs example.

And even when evaluated in parallel, I don't see any reason why the drv-path should change, as the drv is unchanged.

NobbZ avatar Jan 08 '25 21:01 NobbZ

Which are you talking about?

Again, the bindings are obviously unused, and as we do not have an @-pattern, they are not even helpers as in OPs example.

And even when evaluated in parallel, I don't see any reason why the drv-path should change, as the drv is unchanged.

https://github.com/NixOS/nixpkgs/actions/runs/12679250046/job/35338568046?pr=372224

lucasew avatar Jan 08 '25 21:01 lucasew

Please review my diff carefully and fix your "proof of concept", I even wrote "after fixing the call site".

NobbZ avatar Jan 08 '25 21:01 NobbZ