Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use position in all errors #3505

Closed
domenkozar opened this issue Apr 16, 2020 · 7 comments
Closed

Use position in all errors #3505

domenkozar opened this issue Apr 16, 2020 · 7 comments
Labels
error-messages Confusing messages and better diagnostics good first issue Quick win for first-time contributors stale

Comments

@domenkozar
Copy link
Member

There's a lot of errors that don't include the position, which makes it hard to understand what's going on.

Lots of occurrences are easy to fix, e.g. #3502

This would also mean getting rid of git grep noPos and making sure all evaluation errors thrown include a Pos.
@domenkozar domenkozar added the error-messages Confusing messages and better diagnostics label Apr 16, 2020
@domenkozar
Copy link
Member Author

For example, there are still calls of forceValue that don't include Pos:

$ git grep forceValue | grep -v pos
src/libexpr/attr-path.cc:        state.forceValue(*v);
src/libexpr/eval-inline.hh:    forceValue(v);
src/libexpr/eval-inline.hh:    forceValue(v);
src/libexpr/eval.cc:        state.forceValue(*vAttrs);
src/libexpr/eval.cc:    forceValue(fun);
src/libexpr/eval.cc:            forceValue(*v);
src/libexpr/eval.cc:void EvalState::forceValueDeep(Value & v)
src/libexpr/eval.cc:        forceValue(v);
src/libexpr/eval.cc:    forceValue(*i->value);
src/libexpr/eval.cc:    forceValue(v1);
src/libexpr/eval.cc:    forceValue(v2);
src/libexpr/eval.hh:    void forceValueDeep(Value & v);
src/libexpr/get-drvs.cc:    state->forceValue(v);
src/libexpr/get-drvs.cc:        state.forceValue(v);
src/libexpr/primops.cc:    state.forceValueDeep(*args[0]);
src/libexpr/primops.cc:    forceValue(v);
src/libexpr/primops/fetchGit.cc:    state.forceValue(*args[0]);
src/libexpr/primops/fetchMercurial.cc:    state.forceValue(*args[0]);
src/libexpr/primops/fetchTree.cc:    state.forceValue(*args[0]);
src/libexpr/primops/fetchTree.cc:            state.forceValue(*attr.value);
src/libexpr/primops/fetchTree.cc:    state.forceValue(*args[0]);
src/libexpr/value-to-json.cc:    if (strict) state.forceValue(v);
src/libexpr/value-to-xml.cc:    if (strict) state.forceValue(v);
src/libexpr/value-to-xml.cc:                    if (strict) state.forceValue(*a->value);
src/libexpr/value-to-xml.cc:                    if (strict) state.forceValue(*a->value);
src/nix-build/nix-build.cc:            state->forceValue(v);
src/nix-env/user-env.cc:    state.forceValue(topLevel);
src/nix-instantiate/nix-instantiate.cc:        state.forceValue(v);
src/nix-instantiate/nix-instantiate.cc:                if (strict) state.forceValueDeep(vRes);
src/nix/eval.cc:            state->forceValueDeep(*v);
src/nix/installables.cc:        state.forceValue(*v);
src/nix/repl.cc:    state->forceValue(v);
src/nix/repl.cc:    state->forceValue(v);
src/nix/search.cc:                state->forceValue(*v);
src/nix/search.cc:                    state->forceValue(*v);

@zimbatm
Copy link
Member

zimbatm commented Apr 18, 2020

yes please! /cc @grahamc

@Ma27
Copy link
Member

Ma27 commented Apr 29, 2020

slightly related: #3548

@domenkozar domenkozar mentioned this issue Apr 30, 2020
Open
@garbas garbas added the good first issue Quick win for first-time contributors label Feb 9, 2021
@stale
Copy link

stale bot commented Aug 10, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Aug 10, 2021
kamadorueda added a commit to kamadorueda/nix that referenced this issue Oct 15, 2021
@kamadorueda
add pos to EvalState::forceValue
1bdeef8 
- This way we improve error messages
  on infinite recursion
- Demo:
  ```nix
  let
    x = builtins.fetchMercurial x;
  in
  x
  ```
- Before:
  ```bash
  $ nix-instantiate --show-trace --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  nix-instantiate --show-trace --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/test/default.nix:2:7:

            1| let
            2|   x = builtins.fetchMercurial x;
             |       ^
            3| in
  ```

Mentions: NixOS#3505
@kamadorueda kamadorueda mentioned this issue Oct 15, 2021
Merged
@kamadorueda
Copy link
Member

I'll be working on this guys, if you don't mind

This was referenced Oct 15, 2021
Open
Open
kamadorueda added a commit to kamadorueda/nix that referenced this issue Oct 16, 2021
@kamadorueda
fetchTree: add pos to EvalState::forceValue
18e3d63 
- This way we improve error messages
  on infinite recursion
- Demo:
  ```nix
  let x = builtins.fetchTree x;
  in x
  ```
- Before:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/nix/test.nix:1:9:

            1| let x = builtins.fetchTree x;
             |         ^
            2| in x
  ```

Mentions: NixOS#3505
kamadorueda added a commit to kamadorueda/nix that referenced this issue Oct 16, 2021
@kamadorueda
fetchTree: add pos to EvalState::forceValue
e5a27a3 
- This way we improve error messages
  on infinite recursion
- Demo:
  ```nix
  let x = builtins.fetchTree {
    type = "git";
    inherit x;
  };
  in x
  ```
- Before:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/nix/test.nix:3:10:

            2|   type = "git";
            3|   inherit x;
             |          ^
            4| };
  ```

Mentions: NixOS#3505
@kamadorueda kamadorueda mentioned this issue Oct 16, 2021
Merged
kamadorueda added a commit to kamadorueda/nix that referenced this issue Oct 17, 2021
@kamadorueda
fetch: nicer infinite recursion errors
823dce9 
- This change applies to builtins.fetchurl and builtins.fetchTarball
- PoC: `let x = builtins.fetchurl x; in x`
- Before:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/nix/test.nix:1:9:

            1| let x = builtins.fetchurl x; in x
             |         ^
  ```

Mentions: NixOS#3505
@kamadorueda kamadorueda mentioned this issue Oct 17, 2021
Merged
dramforever pushed a commit to dramforever/nix that referenced this issue Nov 27, 2021
@kamadorueda @dramforever
add pos to EvalState::forceValue
069d4f5 
- This way we improve error messages
  on infinite recursion
- Demo:
  ```nix
  let
    x = builtins.fetchMercurial x;
  in
  x
  ```
- Before:
  ```bash
  $ nix-instantiate --show-trace --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  nix-instantiate --show-trace --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/test/default.nix:2:7:

            1| let
            2|   x = builtins.fetchMercurial x;
             |       ^
            3| in
  ```

Mentions: NixOS#3505
dramforever pushed a commit to dramforever/nix that referenced this issue Nov 27, 2021
@kamadorueda @dramforever
fetchTree: add pos to EvalState::forceValue
3488c54 
- This way we improve error messages
  on infinite recursion
- Demo:
  ```nix
  let x = builtins.fetchTree x;
  in x
  ```
- Before:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/nix/test.nix:1:9:

            1| let x = builtins.fetchTree x;
             |         ^
            2| in x
  ```

Mentions: NixOS#3505
dramforever pushed a commit to dramforever/nix that referenced this issue Nov 27, 2021
@kamadorueda @dramforever
fetchTree: add pos to EvalState::forceValue
3daacd7 
- This way we improve error messages
  on infinite recursion
- Demo:
  ```nix
  let x = builtins.fetchTree {
    type = "git";
    inherit x;
  };
  in x
  ```
- Before:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/nix/test.nix:3:10:

            2|   type = "git";
            3|   inherit x;
             |          ^
            4| };
  ```

Mentions: NixOS#3505
dramforever pushed a commit to dramforever/nix that referenced this issue Nov 27, 2021
@kamadorueda @dramforever
fetch: nicer infinite recursion errors
f6ba47c 
- This change applies to builtins.fetchurl and builtins.fetchTarball
- PoC: `let x = builtins.fetchurl x; in x`
- Before:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered
  ```
- After:
  ```bash
  $ nix-instantiate --extra-experimental-features flakes --strict
  error: infinite recursion encountered

       at /data/github/kamadorueda/nix/test.nix:1:9:

            1| let x = builtins.fetchurl x; in x
             |         ^
  ```

Mentions: NixOS#3505
@kamadorueda kamadorueda mentioned this issue Nov 27, 2021
Merged
@kamadorueda
Copy link
Member

A few changes have happened, it's very likely that we can close this now, we added pos to many force* calls, made lazy computing the position, and made mandatory passing a position to the force* calls

The following problem to attack in order to have better error messages would be: #1150

@stale stale bot removed the stale label Feb 4, 2022
@stale stale bot added the stale label Aug 12, 2022
@dark-ether
Copy link

i was looking for first issues to familiarize myself with the code base shouldn't this one be closed? @domenkozar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
error-messages Confusing messages and better diagnostics good first issue Quick win for first-time contributors stale
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@zimbatm @garbas @domenkozar @Ma27 @kamadorueda @dark-ether