Errors are inconsistent

[infinisil]

See the nixpkgs_problem.rs file. There's some different ways of construct error messages in there:

• Some are styled with a single-line, some are multi-line (since Better pkgs/by-name errors, minor fixes and improvements nixpkgs#290743)
• Some expression paths are relative to the Nixpkgs root (e.g. EmptyArgument), some are relative to the expression file (e.g. WrongCallPackagePath, using create_path_expr). Ideally the latter should be used in all cases for more accurate errors
• The field names are sometimes superfluous, e.g. UndefinedAttr has both relative_package_file and package_name, even though the former can be computed from the latter using structure::relative_file_for_package, which e.g. EmptyArgument does.

These should be more consistent overall.

[nixos-discourse]

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

https://discourse.nixos.org/t/nixpkgs-architecture-team-conclusion-and-prospective/41020/8

[willbush]

Going to do some work on this. If there's already someone working on this, please let me know. :)

[infinisil]

Awesome, feel free to! Nobody else is working on this :D

[willbush]

So we talked about how it would be nice to just have a context type available to use for formatting the error types into strings in impl fmt::Display for NixpkgsProblem.

After pulling all the data out of all the NixpkgsProblem enum types this is the sorted unique list:

actual_path: RelativePathBuf,
attr_name: String,
call_package_path: Option<RelativePathBuf>,
column: usize,
correct_relative_package_dir: RelativePathBuf,
definition: String,
expected_path: RelativePathBuf,
file: RelativePathBuf,
first: OsString,
io_error: String,
line: usize,
package_name: String,
relative_package_dir: RelativePathBuf,
relative_package_file: RelativePathBuf,
relative_shard_path: RelativePathBuf,
second: OsString,
shard_name: String,
subpath: RelativePathBuf,
text: String,

One goal of consistent errors seems to be to reduce this list so that more errors enum types are using similar context.

And then we could do something like this:

#[derive(Clone)]
pub struct NixpkgsProblem {
    package_name: String,
    file: RelativePathBuf,
    line: usize,
    // ...

    kind: NixpkgsProblemKind
}

#[derive(Clone)]
pub enum NixpkgsProblemKind {
    ShardNonDir,
    InvalidShardName,
    PackageNonDir,
    CaseSensitiveDuplicate,
    // ... 
}

I think if we tried to break up the current NixpkgsProblem enum into more than one type of enum that implements SomeCommonTrait, then we would need Box<dyn SomeCommonTrait> everywhere.

What are your thoughts? I probably need to so some research into common approaches to dealing with problems like this. Perhaps there's an anyhow crate way of composing error messages that I'm not aware of.

[infinisil]

Ping @philiptaron :)

[willbush]

Check out the pub enum FormatError example https://docs.rs/anyhow/latest/anyhow/ where it mentions thiserror. Sounds like we could break up NixpkgsProblems into more than one enum based on classes of errors that have similar context and as long as they impl std::error::Error, whether by hand or thiserror derive macro, then we can use anyhow instead of Box<dyn SomeCommonTrait>.

One example is this where all the returned errors have the same context.

[willbush]

Actually, seems easier to just nest the struct into NixpkgsProblems like this and that way we would never need to downcast an anyhow error to get back well-typed context (used for testing etc).

[infinisil]

Some expression paths are relative to the Nixpkgs root (e.g. EmptyArgument), some are relative to the expression file (e.g. WrongCallPackagePath, using create_path_expr). Ideally the latter should be used in all cases for more accurate errors

Regarding this one, the error becomes more obvious by having tests use pkgs/top-level/all-packages.nix instead of all-packages.nix:

- Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like

    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };

  However, in this PR, the first `callPackage` argument is not a path. See the definition in pkgs/top-level/all-packages.nix:2:

    nonDerivation = self.callPackage ({ someDrv }: someDrv) { };

This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

- Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like

    nonDerivation = callPackage ./../by-name/no/nonDerivation/package.nix { /* ... */ };

  However, in this PR, the first `callPackage` argument is the wrong path. See the definition in pkgs/top-level/all-packages.nix:2:

    nonDerivation = callPackage ./../../someDrv.nix { /* ... */ };

This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

Note how the second error uses ./pkgs/by-name/no/nonDerivation/package.nix (relative to the Nixpkgs root), whereas the second one uses ./../by-name/no/nonDerivation/package.nix (relative to the all-packages.nix file). The latter would be better in general.