Synthetica9 / nix-linter

Linter for the Nix expression language
BSD 3-Clause "New" or "Revised" License
158 stars 16 forks source link

False positive "Move `let` block..." lint #41

Open lovesegfault opened 3 years ago

lovesegfault commented 3 years ago

The following code is minimized from something I had in a nixpkgs overlay:

pkgs: _:
let
  common = if pkgs.hostPlatform.system == "x86_64-linux" then pkgs.foo else pkgs.bar;
in
{
  foo = import ./foo.nix { inherit common; };
  bar = import ./bar.nix { inherit common; };
  baz = import ./baz.nix { inherit common; };
}

It triggers this warning:

Move `let` block outside function definition at foo.nix:1:7-10:1

Which I believe to be a false-positive as moving the let block outside the function definition will put pkgs out of scope and break the if ... then ... else clause.

seppeljordan commented 3 years ago

What if nix-linter meant the following:

pkgs:
let
  common = if pkgs.hostPlatform.system == "x86_64-linux" then pkgs.foo else pkgs.bar;
in
_: {
  foo = import ./foo.nix { inherit common; };
  bar = import ./bar.nix { inherit common; };
  baz = import ./baz.nix { inherit common; };
}

That would technically be correct but also stupid.

EDIT: This would mean that this is not a false positive. But instead another issue. I agree with the OP though in that the linter should not recommend moving the let block outside of the second function definition.

Synthetica9 commented 3 years ago

What if nix-linter meant the following:

It does mean that, so it is working as intended. I'm not sure how one would make the distinction between this "stupid" case and the normal, more useful case (even though this check is not too important even if it does correctly trigger). Maybe it should just be disabled by default to be honest

lovesegfault commented 3 years ago

Oh, indeed, this is way trickier than I expected at first.

@Synthetica9 perhaps just making the diagnostics/hint clearer would be sufficient?

NobbZ commented 3 years ago

So how to disable this check?

Neither can I find an option that would disable a check at all, nor would I know which of the checks causes this error.