NPV-169: detect too-broad with expressions; ratchet them down#142
Open
lucasew wants to merge 1 commit intoNixOS:mainfrom
Open
NPV-169: detect too-broad with expressions; ratchet them down#142lucasew wants to merge 1 commit intoNixOS:mainfrom
with expressions; ratchet them down#142lucasew wants to merge 1 commit intoNixOS:mainfrom
Conversation
Member
|
I'm just taking a look myself, I'll try to create a trampoline for the kind of ratchet you'll need |
This was referenced Jan 8, 2025
Merged
Member
Author
|
I was playing on how to implement the validation itself and, after a lot of rust struggle, there it is: https://github.com/lucasew/playground/tree/master/projetos/20250108-with-detector-rust I may start using it more tho. |
lucasew
force-pushed
the
20250108-toplevel-with
branch
from
a2ce768 to
b0edbd7
Compare
Author
|
I rebased from #144 for the primitives |
lucasew
commented
Jan 9, 2025
src/files.rs
Outdated
| ) -> validation::Result<BTreeMap<RelativePathBuf, ratchet::File>> { | ||
| process_nix_files(nixpkgs_path, nix_file_store, |nix_file| { | ||
| if let Some(open_scope_with_lib) = find_invalid_withs(nix_file.syntax_root) { | ||
| // TODO: what do I return |
Member
There was a problem hiding this comment.
Take a closer look at #145, which is pretty much what you need, except that
Lines 19 to 34 in 003a4ad
file_is_str (or a different name in your case) will be a RatchetState, which should be Tight if the code is as desired and it can't be improved further, and Loose if there's an improvement to make.
2 tasks
Author
|
The xrefcheck fail is related to a non-related test to this PR, a broken symlink but it seems on purpose to test a scenario |
infinisil
previously requested changes
Jan 13, 2025
Member
infinisil
left a comment
infinisil
left a comment
There was a problem hiding this comment.
I'm only reviewing the diff to #144 with this comparison. My feedback:
- You should document the new check here, describing exactly how it behaves. Based on that I also feel like the error message should be changed, because it doesn't seem to be just about top-level
with's. - You should add a test that ensures an existing top-level with doesn't give an error if it still exists after a PR, similar to this test from #145
- The code should have more code comments explaining what it does
However, I guess most importantly, I don't think there's yet any consensus among Nixpkgs committers as to whether such a check is wanted or how exactly it should behave. Of course with the code here already written, it's easy to change the behavior, so I'd engage in NixOS/nixpkgs#371862, emphasising that you can easily change the code as desired, and maybe even make a Discourse post to get more input.
Sorry for the delayed reviews, am quite busy, but already thanks a lot for moving forward with improving CI, this is very valuable work!
lucasew
force-pushed
the
20250108-toplevel-with
branch
from
e716f34 to
205d2c8
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/best-practices-with-with/58857/6 |
philiptaron
force-pushed
the
20250108-toplevel-with
branch
from
45493b0 to
2713203
Compare
philiptaron
dismissed
infinisil’s stale review
Reworked this PR, addressing comments
philiptaron
changed the title
with expressions that may shadow scope
philiptaron
previously approved these changes
Feb 16, 2026
Contributor
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
This looks good to me, but I want @mdaniels5757 to have a look.
Comment on lines
-22
to
-23
| /// Processes all Nix files in a Nixpkgs directory according to a given function `f`, collecting the | ||
| /// results into a mapping from each file to a ratchet value. |
Member
There was a problem hiding this comment.
Why were all of these comments removed?
philiptaron
force-pushed
the
20250108-toplevel-with
branch
from
2713203 to
74de0ce
Compare
Member
mdaniels5757
left a comment
mdaniels5757
left a comment
There was a problem hiding this comment.
High-level review:
- I don't take issue with the code in general, and I support instituting these ratchet checks at least for
meta = with lib; - But:
- I don't understand what's "top-level" about the
withintests/nowith-meta-toplevel-lib/main/test.nix. Top-level, in my head, means something like{ lib, stdenv }: with lib; stdenv.mkDerivation { ... }. So I think the error message (and the variable names/comments) should have this modified. - Is there enough of a consensus for this? On one hand, I don't really see one in #292468, on the other, my sense of the vibes is that, although there are dissenters, at a minimum
meta = with lib; { ... }is generally Considered Harmful™. But I am not so confident that everything of the formswith foo; { ... },with foo; with bar; ..., orwith foo; let ...; in ...is also Considered Harmful™.
- I don't understand what's "top-level" about the
philiptaron
force-pushed
the
20250108-toplevel-with
branch
2 times, most recently
from
3900888 to
d31ac5b
Compare
Contributor
|
This is a little too tight of a ratchet. |
philiptaron
force-pushed
the
20250108-toplevel-with
branch
from
d31ac5b to
0643593
Compare
philiptaron
force-pushed
the
20250108-toplevel-with
branch
4 times, most recently
from
b08cd7a to
32decc6
Compare
Add a ratchet check that prevents introducing new top-level `with` expressions whose body contains attr sets, let-in bindings, or nested with expressions. Existing instances are grandfathered via the ratchet. Co-authored-by: Lucas Eduardo Wendt <lucas59356@gmail.com>
philiptaron
force-pushed
the
20250108-toplevel-with
branch
from
32decc6 to
54115a5
Compare
Contributor
|
I think I'm a lot happier with this. Check out the docs on https://github.com/NixOS/nixpkgs-vet/wiki/NPV-169. |
philiptaron
changed the title
with expressions that may shadow scopewith expressions; ratchet them down
Contributor
Docs LGTM |
Member
|
Hmm. I rigged the ratchet to log whenever instances of TopLevelWithMayShadowVariablesAndBreakStaticChecks (old) and OverlyBroadWith (new) are present. I ran once for each of those, with current nixpkgs (623d84aa5a78836aecb316f4818c12706db75b77, Tue Feb 17 21:42:36 2026 +0000) as both the base and new commit. Here is the diff of the files flagged by each (TopLevelWithMayShadowVariablesAndBreakStaticChecks is on the left, OverlyBroadWith is on the right)Some observations:
All of those came from the set of files that were only flagged by OverlyBroadWith. I'm not quite sure what to propose about this (maybe require both the conditions from TopLevelWithMayShadowVariablesAndBreakStaticChecks and OverlyBroadWith be satisfied to issue a Problem)? But the three files linked are really quite reasonable, so I don't think this can be merged as-is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PR adds NPV-169, a ratchet check that flags top-level
withexpressions whose body contains attr sets,let-inbindings, or nestedwithexpressions, since these can shadow variables and break static analysis.Existing instances are grandfathered via the ratchet mechanism — only newly introduced uses are flagged. It extends the
Fileratchet with atop_level_withfield and theDoesNotIntroduceToplevelWithsratchet typeTest cases
nowith-basicwith lib; 2has no shadowing constructs in bodynowith-meta-toplevel-libwith lib; { ... }shadows via attr setnowith-meta-with-maintainerswith lib.maintainers; [ ... ]is fine (no attr set/let/nested with)nowith-nixos-submodule-shadowwith lib.types; attrsOf (...)nowith-nixos-submodule-shadow-changednowith-nixos-submodule-shadow-do-not-touch-if-not-changednowith-nixos-submodule-unshadowlib.types.prefix instead ofwithnowith-nixos-submodule-unshadow-inheritinheritinstead ofwithThis PR is reworked from original contributions from @lucasew.