Skip to content

Commit

Permalink
libnixf: remove sema-escaping-with because it is too pedantic (#582)
Browse files Browse the repository at this point in the history
This diagnostic highlighted too much.
In some cases, people may use "let" expression to override something
inside `with pkgs;`

If "escaping-with" is diagnosed, there is no good fix to workaround,
without completely disable that diagnostic in configuration.

CC @Aleksanaa who previously worked on this.
  • Loading branch information
inclyc authored Aug 11, 2024
1 parent 84bedfb commit c9d8970
Show file tree
Hide file tree
Showing 5 changed files with 3 additions and 56 deletions.
4 changes: 1 addition & 3 deletions libnixf/include/nixf/Basic/NoteKinds.inc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,5 @@ DIAG_NOTE("merge-diff-rec-consider", RecConsider,
"will be considered as {}recursive")
DIAG_NOTE("note-bcomment-begin", BCommentBegin, "/* comment begins at here")
DIAG_NOTE("to-match-this", ToMachThis, "to match this {}")
DIAG_NOTE("var-bind-to-this", VarBindToThis,
"variable will bind to this definition")
DIAG_NOTE("escaping-this-with", EscapingWith, "escaping this with expression")

#endif // DIAG_NOTE
6 changes: 0 additions & 6 deletions libnixf/src/Basic/diagnostic.py
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,4 @@ class Diagnostic(TypedDict):
"severity": "Warning",
"message": "unused `with` expression",
},
{
"sname": "sema-escaping-with",
"cname": "EscapingWith",
"severity": "Hint",
"message": "this variable comes from the scope outside of the `with` expression",
},
]
16 changes: 0 additions & 16 deletions libnixf/src/Sema/VariableLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,22 +97,6 @@ void VariableLookupAnalysis::lookupVar(const ExprVar &Var,
if (Def) {
Def->usedBy(Var);
Results.insert({&Var, LookupResult{LookupResultKind::Defined, Def}});

if (EnclosedWith && !Def->isBuiltin()) {
// Escaping from "with" to outer scope.
// https://github.com/NixOS/nix/issues/490

assert(WithEnv && "EnclosedWith -> WithEnv");
// Make a diagnostic.
Diagnostic &D =
Diags.emplace_back(Diagnostic::DK_EscapingWith, Var.range());
if (Def->syntax()) {
D.note(Note::NK_VarBindToThis, Def->syntax()->range());
}
const auto &KwWith =
static_cast<const nixf::ExprWith *>(WithEnv->syntax())->kwWith();
D.note(Note::NK_EscapingWith, KwWith.range());
}
return;
}
if (EnclosedWith) {
Expand Down
29 changes: 0 additions & 29 deletions libnixf/test/Sema/VariableLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -273,35 +273,6 @@ TEST_F(VLATest, FormalDef) {
ASSERT_EQ(Diags.size(), 0);
}

TEST_F(VLATest, EscapingWith) {
std::shared_ptr<Node> AST = parse("a: with { a = 1; b = 2; }; a + b", Diags);
VariableLookupAnalysis VLA(Diags);
VLA.runOnAST(*AST);

ASSERT_EQ(Diags.size(), 1);

const Diagnostic &D = Diags[0];

ASSERT_EQ(D.notes().size(), 2);

ASSERT_EQ(D.notes()[0].kind(), Note::NK_VarBindToThis);
ASSERT_EQ(D.notes()[0].range().lCur().offset(), 0);
ASSERT_EQ(D.notes()[0].range().rCur().offset(), 1);

ASSERT_EQ(D.notes()[1].kind(), Note::NK_EscapingWith);
ASSERT_EQ(D.notes()[1].range().lCur().offset(), 3);
ASSERT_EQ(D.notes()[1].range().rCur().offset(), 7);
}

TEST_F(VLATest, EscapingWithButBuiltin) {
std::shared_ptr<Node> AST =
parse("with { a = 1; }; [ a true false null ]", Diags);
VariableLookupAnalysis VLA(Diags);
VLA.runOnAST(*AST);

ASSERT_EQ(Diags.size(), 0);
}

TEST_F(VLATest, InheritRec) {
// Make sure inheirt (expr), the expression is binded to "NewEnv".
std::shared_ptr<Node> AST =
Expand Down
4 changes: 2 additions & 2 deletions nixd/docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ nvim_lsp.nixd.setup({
// Control the diagnostic system
"diagnostic": {
"suppress": [
"sema-escaping-with"
"sema-extra-with"
]
}
}
Expand All @@ -167,7 +167,7 @@ prefer to suppress diagnostics altogether. This can be achieved by utilizing the
"diagnostic": {
// A list of diagnostic short names
"suppress": [
"sema-escaping-with"
"sema-extra-with"
]
}
}
Expand Down

0 comments on commit c9d8970

Please sign in to comment.