Combining nested attribute shorthand and `rec` causes weird scoping behavior #329

New issue

Closed

opened 2024-05-19 00:57:58 +00:00 by gaelan · 5 comments

gaelan commented

2024-05-19 00:57:58 +00:00

Describe the bug

When an attrset is declared with one element which is itself a rec attrset, and another element nested within that attrset using the a.b=c syntax, the value ("c") is evaluated in the scope of the recursive attrset.

Steps To Reproduce

{ a = rec { b = 5; }; a.c = b; }

This results in {a = {b = 5; c = 5;};}, despite b looking like it shouldn't be in scope.

Expected behavior

error: undefined variable 'b', because b isn't in scope at the declaration of a.c

If, god forbid, someone depends on the current behavior, deprecate it and print a warning.

`nix --version` output

nix (Lix, like Nix) 2.90.0-beta.1-lixpre20240506-b6799ab

Additional context

Nix bug: https://github.com/NixOS/nix/issues/6251

## Describe the bug When an attrset is declared with one element which is itself a `rec` attrset, and another element nested within that attrset using the `a.b=c` syntax, the value ("c") is evaluated in the scope of the recursive attrset. ## Steps To Reproduce ``` { a = rec { b = 5; }; a.c = b; } ``` This results in `{a = {b = 5; c = 5;};}`, despite `b` looking like it shouldn't be in scope. ## Expected behavior `error: undefined variable 'b'`, because `b` isn't in scope at the declaration of `a.c` If, god forbid, someone depends on the current behavior, deprecate it and print a warning. ## `nix --version` output `nix (Lix, like Nix) 2.90.0-beta.1-lixpre20240506-b6799ab` ## Additional context Nix bug: https://github.com/NixOS/nix/issues/6251

gaelan added the

bug

label 2024-05-19 00:57:58 +00:00

pennae commented

2024-05-19 12:42:43 +00:00

Owner

consider { a = rec { b = c; }; a.c = 5; }. desugaring makes this work, but again c looks to not be in scope. should we apply the outer scope to this c too? if we do we make the language extremely sensitive to code motion. in fact, swapping the two breaks this snippet due to a parser bug that we can't really fix anymore without risking breakages.

consider let d = 1; in { a = rec { b = rec { c = d; }; d = 2; }; a.b.d = 3; }. what should this evaluate to?

rec sets are a bit of an anti-pattern in themselves, making them non-mergable would be the least surprising and corner-case-y way forward. it's so much easier to explain "attrset merging does not apply to rec" than adding more scoping rules that can be extremely surprising when thing silently evaluate to something else just by moving some code around that.

there's actually no reasonable way to implement your expected behavior in the current version of the language because this is merely syntactic sugar. rec sets in general are very confusing (and even buggy, in some cases); we::horrors'd much rather deprecate the desugaring for rec sets specifically instead of changing how scoping works. consider `{ a = rec { b = c; }; a.c = 5; }`. desugaring makes this work, but again `c` looks to not be in scope. should we apply the outer scope to this `c` too? if we do we make the language extremely sensitive to code motion. in fact, swapping the two *breaks this snippet* due to a parser bug that we can't really fix anymore without risking breakages. consider `let d = 1; in { a = rec { b = rec { c = d; }; d = 2; }; a.b.d = 3; }`. what should this evaluate to? `rec` sets are a bit of an anti-pattern in themselves, making them non-mergable would be the least surprising and corner-case-y way forward. it's so much easier to explain "attrset merging does not apply to `rec`" than adding more scoping rules that can be extremely surprising when thing silently evaluate to something else just by moving some code around that.

👍 1

gaelan commented

2024-05-20 22:29:14 +00:00

Author

consider { a = rec { b = c; }; a.c = 5; }

Honestly this example seems less cursed to me; you can sensibly define rec as "with the attrset I'm making right now, including any attributes that get merged in from elsewhere". In other words, in this example, d's scoping is affected by the fact that it's in a rec block; in my example, something's scope being affected by the presence of a block, despite being outside that block.

That being said:

rec sets are a bit of an anti-pattern in themselves, making them non-mergable would be the least surprising and corner-case-y way forward

Yes, this seems like by far the best move.

let d = 1; in { a = rec { b = rec { c = d; }; d = 2; }; a.b.d = 3; }

😱

> consider `{ a = rec { b = c; }; a.c = 5; }` Honestly this example seems less cursed to me; you can sensibly define rec as "`with` the attrset I'm making right now, including any attributes that get merged in from elsewhere". In other words, in this example, `d`'s scoping is affected by the fact that it's in a rec block; in my example, something's scope being affected by the presence of a block, despite being outside that block. That being said: > `rec` sets are a bit of an anti-pattern in themselves, making them non-mergable would be the least surprising and corner-case-y way forward Yes, this seems like by far the best move. > `let d = 1; in { a = rec { b = rec { c = d; }; d = 2; }; a.b.d = 3; }` 😱

pennae commented

2024-05-21 00:27:44 +00:00

Owner

you can sensibly define rec as "with the attrset I'm making right now, including any attributes that get merged in from elsewhere"

as in, rec { ... } being rewritten to let __hidden = with __hidden; { ... }; in __hidden? with scoping doesn't work like that (with scopes are searched after all other scopes, not in syntactic order) and cannot be changed without breaking things. it'd need an entirely new kind of scope for this to work at all, and that would be quite a lot of complexity for a feature that doesn't even seem to be used very often. deleting the feature does seem to be the only sound option 🫠

> you can sensibly define rec as "`with` the attrset I'm making right now, including any attributes that get merged in from elsewhere" as in, `rec { ... }` being rewritten to `let __hidden = with __hidden; { ... }; in __hidden`? with scoping doesn't work like that (`with` scopes are searched *after* all other scopes, not in syntactic order) and cannot be changed without breaking things. it'd need an entirely new kind of scope for this to work at all, and that would be quite a lot of complexity for a feature that doesn't even seem to be used very often. deleting the feature does seem to be the only sound option 🫠

gaelan commented

2024-05-21 09:35:58 +00:00

Author

Ah yes, very good points.