Looks great!

A few quick style comments (haven't looked at the code in detail):

• Please follow the coding style from ~/.dir-locals.el (4 characters indent, etc.)

• format("bla %1%") % args is deprecated, please use fmt("bla %s", args) instead.

• Instead of using std::nullopt; please use std:: everywhere.

• Templates nested 8 levels deep (NixLangWarning) frighten me as a simple programmer...

• Builder patterns like .name("bla").description("bla")... are no longer necessary in C++20 thanks to designated initializers (NixLangWarning { .name = "bla", .description = "bla", ... }).

Is the ColumnRange type intended to replace Pos?

Thanks for the feedback! I'll fix the style stuff.

Pos: true! ColumnRange and nixFile are duplicating functionality from Pos. That's part of the integration problem I hadn't addressed yet. I don't think having both is the way to go, but Pos depends on Symbol, so pulling Pos out into libutil woudn't work. I could perhaps make a builder function in libexpr that would take Pos as an argument - assuming we keep the builders. More on that below. At any rate, ColumnRange isn't meant to replace Pos.

format/fmt; ok - didn't know we were phasing out boost::format. I made the Hintfmt wrapper to ensure that all templated values in hints are automatically colored yellow, for consistency. I think it should be possible to have the automatic yellow feature and the variadic argument interface too. Sound ok?

Re the builder interface. I get it with the templates - reading error.hh is a little hard on the eyes compared to a plain class. My justification for using the builder pattern over designated initializers is that the initializers are optional. For instance with the ErrLine class this works:

  ErrLine el = ErrLine { 
      .lineNumber = 20, 
      .columnRange = ColumnRange { .start = 40, .len = 30 }, 
      .prevLineOfCode = optional("prev"),
      .errLineOfCode = "current",
      .nextLineOfCode = optional("next") 
      };

That's great, and it looks good. But this also works - no helpful designations:

  ErrLine el2 = ErrLine { 
      20, 
      ColumnRange { .start = 40, .len = 30 }, 
      optional("prev"),
      "current",
      optional("next") };

And you can also leave things out, like so:

  ErrLine el3 = ErrLine { 20 };

  ErrLine el4 = ErrLine { .errLineOfCode = "current" };

With the builder interface, you are guaranteed to have the same builder functions 'naming' each argument. Also with the builders, the sequence of functions is always the same.

For adding hints, I was thinking the hint()/nohint() builder would be handy. Initially all errors could be created without hints, as they are now. They'd call function nohint() on create. As hints are added, it would be easy to find the errors still lacking hints by grepping for 'nohint()'. With designated initializers, you're looking for a lack of .hint = <foo>, which is harder to find.

This could be taken a (draconian?) step further by removing the nohint() builder function, and then all errors without hints would be type errors.

With the language errors, on the other hand, virtually all of them are already hints - templated text. We might start with hints for those, and add descriptions afterwards, in a similar fashion.

So that's the case for the builder interface. Its easy to read at the call site at least! I like the ability to micromanage the syntax of error creation. But if its too much we can just go for a consistent calling convention, and keep error.hh simple.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/marketing-team-can-we-present-nix-nixos-better/6249/38

If you post updates to a blog or the crowdfunding page of before/after of these I bet it would increase excitement and funding.

Great work!

Progress is made!

I think I've addressed most of the concerns above. Highlights follow:

Pos:

It seems like a good thing to have all the ErrorInfo code in error.cc and error.hh. To that end, I've opted to use a template function to pull information out of Pos and store it in error info - its just file, line, and column at this point. The template function means there's no dependency on Pos, so all the error code stays in libutil.

An alternative to this would be to make a subclass of ErrorInfo that lives in libexpr, or to just have a separate error class for libexpr.

fmt, hintfmt

I think having automatic yellow for templated values is a good idea, so I made a hintfmt function that does that, with the variadic syntax of fmt. It uses the formatHelper from fmt underneath. Unlike fmt, hintfmt returns a class, not a string. That way the constructor methods can require hintfmt usage.

If you really want monochrome templated values you can get them with ANSI_NORMAL.

Construction style:

At this point, I have 3 variations.

• builder style. This is still the template approach from before. What's different from last time is fewer functions, and one takes Pos as an argument. Builder style usage is here:

https://github.com/bburdette/nix/blob/error-format/src/error-demo/error-demo.cc

An example:

    printErrorInfo(NixLangError()
                   .name("error name")
                   .description("error description")
                   .pos(Pos(problem_symbol, 40, 13))
                   .linesOfCode(std::optional("previous line of code"),
                                "this is the problem line of code",
                                std::optional("next line of code"))
                   .hint(hintfmt("this hint has %1% templated %2%!!", "yellow", "values")));

• designated initializer style. This is in a branch on my repo, not merged into this PR as of yet. Here's the error-demo program in this style:

https://github.com/bburdette/nix/blob/initializer-style/src/error-demo/error-demo.cc

I think this style looks pretty good. You can't put the parameters in out of order, it turns out, but you can omit them, or use a more conventional init style without designations. On this branch I restructured the ErrorInfo class a bit to allow using Pos to init an 'errPos' class.

    printErrorInfo(
        ErrorInfo{
            .level = elError,
            .name = "error name",
            .description = "error description",
            .hint = hintfmt("this hint has %1% templated %2%!!", "yellow", "values"),
            .nixCode = NixCode {
                .errPos = Pos(problem_file, 40, 13),
                .prevLineOfCode = std::optional("previous line of code"),
                .errLineOfCode = "this is the problem line of code",
                .nextLineOfCode = std::optional("next line of code"),
            }});

• named constructor functions. I'm not really that fond of this but I did it this way to see what it looked like. There's a demo of it here:

https://github.com/bburdette/nix/blob/constructor-style/src/error-demo/error-demo.cc

We lose the parameter naming with this approach, but parameters are non-optional, similar to the template builder approach.

printErrorInfo(
        ErrorInfo::NixLangError(
            "error name",
            "error description",
            Pos(problem_symbol, 40, 13),
            std::optional("previous line of code"),
            "this is the problem line of code",
            std::optional("next line of code"),
            hintfmt("this hint has %1% templated %2%!!", "yellow", "values")));

error-demo location?

Currently I have the demo program in src/error-demo/error-demo.cc. I moved it out of test/ since its not really a test, but then I'm not sure where else it should go. For now I just put it with the rest of the C++ programs. We can leave it there, move it back to test, or remove it completely. The next phase of this error project will be to make a PR that applies the format to existing errors and integrates with logging and so forth. That will make this demo program perhaps unnecessary.

@edolstra as @bburdette is going to start using this all around the code base, would really appreciate a review to finalize the api.

I think I would prefer the designated initializer style, mainly because it requires much less boilerplate code.

Templates should be avoided IMHO. For example, the function AddPos::pos() should take a Pos argument, rather than a class P. Or maybe pos should just be a field and we don't need a AddPos template class. BTW maybe those classes could be expressed using inheritance (e.g. ProgramError inherits from AddName, AddDescription, etc.) but it seems easier to just define ProgramError as

struct ProgramError {
  std::string name;
  std::string description;
  ...
}

Ok thanks for the review. I've merged in the designated initializer style.

The error class itself is quite simple, which is appealing:

class ErrorInfo
{
public:
    ErrLevel level;
    string name;
    string description;
    std::optional<hintformat> hint;
    std::optional<NixCode> nixCode;

    static std::optional<string> programName;
};

The nixCode part has an ErrPos field, which you init with assignment like so:

            .nixCode = NixCode {
                .errPos = Pos(problem_file, 40, 13),
                 ...
                 }

ErrPos implements its constructor and operator= as templates though.

I think the inheritance/virtual function approach may end up with more boilerplate than this, but I'll give it some thought and see what I can come up with.

Ok I used inheritance to pull the info from Pos. I have that in a branch right now:

struct PosInterface
{
    virtual int getLine() const = 0;
    virtual int getColumn() const = 0;
    virtual string getFile() const = 0;
};

class ErrPos
{
public:
   int line;
   int column;
   string file;

   ErrPos& operator=(const PosInterface &pos) {
     line = pos.getLine();
     column = pos.getColumn();
     file = pos.getFile();
     return *this;
   }
   ErrPos(const PosInterface &pos) {
     *this = pos; 
   }
};

And then in nixexpr.hh, Pos implements the getters:

struct Pos : public PosInterface 
{
    Symbol file;
    unsigned int line, column;
    Pos() : line(0), column(0) { };
    Pos(const Symbol & file, unsigned int line, unsigned int column)
        : file(file), line(line), column(column) { };
    operator bool() const
    {
        return line != 0;
    }
    bool operator < (const Pos & p2) const
    {
        if (!line) return p2.line;
        if (!p2.line) return false;
        int d = ((string) file).compare((string) p2.file);
        if (d < 0) return true;
        if (d > 0) return false;
        if (line < p2.line) return true;
        if (line > p2.line) return false;
        return column < p2.column;
    }

    int getLine() const override { return line; }
    int getColumn() const override { return column; }
    string getFile() const override { return file; }
};

For reference, the template way was like this:

class ErrPos
{
public:
    int line;
    int column;
    string nixFile;

    template <class P>
    ErrPos& operator=(const P &pos)
    {
        line = pos.line;
        column = pos.column;
        nixFile = pos.file;
        return *this;
    }

    template <class P>
    ErrPos(const P &p)
    {
      *this = p;
    }
};

Either of these options is fine by me. Another way to go would be to subclass ErrorInfo and implement its interface in nixexpr.hh. That would look something like this:

// ErrorInfo interface would be in error.hh
class ErrorInfo
{
public:
    ErrLevel level;
    string name;
    string description;
    std::optional<hintformat> hint;
    virtual std::optional<NixCode> getNixCode() = 0;

    static std::optional<string> programName;
};

// NixCode becomes an interface.
class NixCode 
{
public:
  virtual int getLine() = 0;
  virtual int getColumn() = 0;
  virtual string getFile() = 0;
  ...
}

// this would be in error.hh
class ProgramError : public ErrorInfo
{
public:
    // interface implementation.
    std::optional<NixCode> getNixCode() override { return std::nullopt; }
};

// this would live in nixexpr.hh 
class LanguageError : public ErrorInfo
{
public:
    std::optional<NixCode> nixCode;

    // interface implementation.
    std::optional<NixCode> getNixCode() override { return this->nixCode; }
};

class NixCodeImpl : public NixCode
{
  Pos pos;

  int getLine() override { return pos.line; }
  int getColumn() override { return pos.column; }
  string getFile() override { return pos.file; }
};

Do any of these options appeal? I haven't prototyped the subclass one yet. Another way to go would be to bail on having one unified error class, and do error processing for the language stuff separately.

@edolstra ready for re-review.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-extract-signal-from-almost-useless-error-message/11733/13