Add `builtins.warn` #10592

roberth · 2024-04-22T23:32:13Z

Motivation

A variation of trace, but

with the correct log level
no ANSI codes in expressions
can be debugged without stopping for actual trace calls (ie intentional "info level" or trace logging).
logs warnings without trace: in front of it
follows the Nix color scheme.

Context

I'd planned to add this a long time ago but forgot to eventually follow up on it.
This Nixpkgs PR reminded me to complete it, and I would like to avoid an unnecessary deprecation.
It proposes to rename the environment variable, which would make the current lib.warn not an (anachronistic) polyfill of the Nix feature anymore. That'd be unfortunate.

lib.trivial: rename NIX_ABORT_ON_WARN to NIXPKGS_ABORT_ON_WARN nixpkgs#306026

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Ericson2314 · 2024-04-23T15:07:03Z

src/libexpr/primops.cc

+    if (args[0]->type() == nString)
+        printMsg(lvlWarn, ANSI_WARNING "warning:" ANSI_NORMAL " %1%", args[0]->string_view());
+    else
+        printMsg(lvlWarn, ANSI_WARNING "warning:" ANSI_NORMAL " %1%", ValuePrinter(state, *args[0]));


Why do we need to redo the error formatting here, vs reused something from the loggers?

It now uses logWarning, which also sets us up to print an actual position, after #10597.

Ericson2314 · 2024-04-24T01:37:43Z

src/libexpr/primops.cc

+    }
+
+    if (evalSettings.builtinsAbortOnWarn) {
+        state.error<Abort>("aborting to reveal stack trace of warning, as abort-on-warn is set").debugThrow();


Optional: we could also just throw our BaseError with this as extra context.

I prefer to treat them separately, because they don't have the same message, and I don't think it should be just a single message.

Totally fine! Though just to be clear, I was thinking the this message would become an addTrace call --- I too don't think it should be a single message.

Ericson2314 · 2024-04-24T16:06:17Z

src/libexpr/primops.cc

+    }
+
+    if (evalSettings.builtinsAbortOnWarn) {
+        state.error<Abort>("aborting to reveal stack trace of warning, as abort-on-warn is set").debugThrow();


Totally fine! Though just to be clear, I was thinking the this message would become an addTrace call --- I too don't think it should be a single message.

Ericson2314 · 2024-04-24T16:06:59Z

src/libexpr/primops.cc

+    if ((evalSettings.builtinsTraceDebugger || evalSettings.builtinsDebuggerOnWarn) && state.debugRepl && !state.debugTraces.empty()) {
+        const DebugTrace & last = state.debugTraces.front();
+        state.runDebugRepl(nullptr, last.env, last.expr);
+    }


A few of the call sites runDebugRepl look similar --- not just builtin.trace's even. Should we abstract over this a bit?

I think yes, by allowing any primop to be marked as a breakpoint.
Until we have that, let's just keep it simple.

Just to be clear, I mean not a new generalized feature, but something very mechanical:

if (state.debugRepl && !state.debugTraces.empty()) { const DebugTrace & last = state.debugTraces.front(); state.runDebugRepl(<some-expression>, last.env, last.expr); }

seems to be part of this EvalErrorBuilder<T>::debugThrow, primop_break, primop_trace, and now prim_warn. So I am wondering if a new function (maybe in header, taking an auto && lambda, to get the laziness right) is warranted.

src/libexpr/primops.cc

lorenzleutgeb · 2024-05-02T18:47:21Z

src/libexpr/primops.cc

+        msg.atPos(state.positions[pos]);
+        auto info = msg.info();
+        info.level = lvlWarn;
+        logWarning(info);


If I understand correctly, users will not be able to easily distinguish output produced by builtins.warn from output produced by other calls of logWarning. However, other callsites will not go ahead and trigger the "abort on warn" behaviour. It's a safe bet that you'll have some users set NIX_ABORT_ON_WARN, then hit one of the other codepaths, and be confused why Nix did not abort.

I don't think that it's a solution to have Nix abort on all these kinds of warnings. There are rather benign warnings too, like:

warning: Git tree '...' is dirty warning: unknown flake output 'homeModules'

The only proposal I have is to use some other logging prefix, like eval-warning: instead of warning: to make the distinction more clear.

Constructing ErrorInfo is a little awkward for now, but this does produce a richer log entry.

... so that we may perhaps later extend the interface. Note that Nixpkgs' lib.warn already requires a string coercible argument, so this is reasonable. Also note that string coercible values aren't all strings, but in practice, for warn, they are.

roberth · 2024-05-06T16:53:27Z

Trivial rebase for CI 🤞

edolstra · 2024-05-06T18:49:16Z

src/libexpr/primops.cc

+    }
+
+    if (evalSettings.builtinsAbortOnWarn) {
+        state.error<Abort>("aborting to reveal stack trace of warning, as abort-on-warn is set").debugThrow();


This shouldn't throw Abort, since that's a subclass of EvalError, which causes the error to be stored in the eval cache. So it should be an exception type that is not a subclass of EvalError.

edolstra · 2024-05-06T19:02:12Z

Can you add a release note for this?

roberth added feature Feature request or proposal error-messages Confusing messages and better diagnostics language The Nix expression language; parser, interpreter, primops, evaluation, etc labels Apr 22, 2024

roberth requested a review from edolstra as a code owner April 22, 2024 23:32

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 22, 2024

roberth mentioned this pull request Apr 22, 2024

lib.trivial: rename NIX_ABORT_ON_WARN to NIXPKGS_ABORT_ON_WARN NixOS/nixpkgs#306026

Open

13 tasks

This comment was marked as outdated.

Sign in to view

roberth force-pushed the builtins-warn branch from be20eb6 to 3e146f4 Compare April 23, 2024 10:25

Ericson2314 reviewed Apr 23, 2024

View reviewed changes

roberth mentioned this pull request Apr 23, 2024

[WIP] Add a non-noPos call position to primops #10597

Draft

1 task

Ericson2314 reviewed Apr 24, 2024

View reviewed changes

Ericson2314 approved these changes Apr 24, 2024

View reviewed changes

roberth mentioned this pull request Apr 24, 2024

lib.warn: Use or behave like builtins.warn NixOS/nixpkgs#306481

Draft

13 tasks

roberth force-pushed the builtins-warn branch from ad9f26d to 582de3b Compare April 24, 2024 09:24

roberth requested a review from Ericson2314 April 24, 2024 09:43

Ericson2314 reviewed Apr 24, 2024

View reviewed changes

roberth requested a review from Ericson2314 May 2, 2024 08:25

Ericson2314 reviewed May 2, 2024

View reviewed changes

src/libexpr/primops.cc Outdated Show resolved Hide resolved

roberth force-pushed the builtins-warn branch from b2f39fe to 91d9008 Compare May 2, 2024 16:56

lorenzleutgeb reviewed May 2, 2024

View reviewed changes

Ericson2314 approved these changes May 3, 2024

View reviewed changes

roberth added 4 commits May 6, 2024 18:52

Add builtins.warn

0f8cfd9

builtins.warn: Use logWarning

d1072d0

Constructing ErrorInfo is a little awkward for now, but this does produce a richer log entry.

builtins.warn: Require string argument

b3c9432

... so that we may perhaps later extend the interface. Note that Nixpkgs' lib.warn already requires a string coercible argument, so this is reasonable. Also note that string coercible values aren't all strings, but in practice, for warn, they are.

refactor: Extract EvalState::{runDebugRepl,canDebug}

15b6cc5

roberth force-pushed the builtins-warn branch from 91d9008 to 15b6cc5 Compare May 6, 2024 16:52

edolstra requested changes May 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `builtins.warn` #10592

Add `builtins.warn` #10592

roberth commented Apr 22, 2024

This comment was marked as outdated.

Ericson2314 Apr 23, 2024 •

edited

roberth Apr 23, 2024

Ericson2314 Apr 24, 2024

roberth Apr 24, 2024

Ericson2314 Apr 24, 2024

Ericson2314 Apr 24, 2024

Ericson2314 Apr 24, 2024

roberth Apr 24, 2024

Ericson2314 Apr 24, 2024 •

edited

lorenzleutgeb May 2, 2024 •

edited

roberth commented May 6, 2024

edolstra May 6, 2024 •

edited

edolstra commented May 6, 2024

Add builtins.warn #10592

Are you sure you want to change the base?

Add builtins.warn #10592

Conversation

roberth commented Apr 22, 2024

Motivation

Context

Priorities and Process

This comment was marked as outdated.

Ericson2314 Apr 23, 2024 • edited

Choose a reason for hiding this comment

roberth Apr 23, 2024

Choose a reason for hiding this comment

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

roberth Apr 24, 2024

Choose a reason for hiding this comment

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

roberth Apr 24, 2024

Choose a reason for hiding this comment

Ericson2314 Apr 24, 2024 • edited

Choose a reason for hiding this comment

lorenzleutgeb May 2, 2024 • edited

Choose a reason for hiding this comment

roberth commented May 6, 2024

edolstra May 6, 2024 • edited

Choose a reason for hiding this comment

edolstra commented May 6, 2024

Add `builtins.warn` #10592

Add `builtins.warn` #10592

Ericson2314 Apr 23, 2024 •

edited

Ericson2314 Apr 24, 2024 •

edited

lorenzleutgeb May 2, 2024 •

edited

edolstra May 6, 2024 •

edited