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.

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

Suggested improvements to NExpr #377

Open
3 of 6 tasks
infinisil opened this issue Nov 17, 2018 · 24 comments
Open
3 of 6 tasks

Suggested improvements to NExpr #377

infinisil opened this issue Nov 17, 2018 · 24 comments

Comments

@infinisil
Copy link
Contributor

infinisil commented Nov 17, 2018

Maybe other improvements are possible too. I really dislike how some things are representable using the current one but never occur in parsing Nix.

@Synthetica9
Copy link
Contributor

Synthetica9 commented Nov 17, 2018

  • Can we also get consistent source positions? Currently some things have their own SourcePos, next to the SrcSpan provided by NExprLocF:

    hnix/src/Nix/Expr/Types.hs

    Lines 179 to 190 in 2f418b2

    data Binding r
    = NamedVar !(NAttrPath r) !r !SourcePos
    -- ^ An explicit naming, such as @x = y@ or @x.y = z@.
    | Inherit !(Maybe r) ![NKeyName r] !SourcePos
    -- ^ Using a name already in scope, such as @inherit x;@ which is shorthand
    -- for @x = x;@ or @inherit (x) y;@ which means @y = x.y;@. The
    -- unsafeGetAttrPos for every name so inherited is the position of the
    -- first name, whether that be the first argument to this constructor, or
    -- the first member of the list in the second argument.
    deriving (Generic, Generic1, Typeable, Data, Ord, Eq, Functor,
    Foldable, Traversable, Show, NFData, Hashable)
    Maybe have the annotations be at least parametrised, so we can set them to () when it's convenient?

@jwiegley
Copy link
Member

Should NAttrPath take a dynamic boolean argument, rather than two constructors?

@jwiegley
Copy link
Member

@infinisil I'm rather open to PRs for these suggestions. Perhaps doing them one at a time would be simplest?

@ryantrinkle
Copy link
Member

@Synthetica9 Nix has a primitive that retrieves source location for those things internal to the language, so they are semantically significant. The other source locations are used just for error reporting and such. It may be possible to unify them to some extent, but it's not quite as easy as one would hope.

@sjakobi
Copy link
Member

sjakobi commented Jun 28, 2020

#666 (comment) is somewhat related.

@Anton-Latukha

This comment has been minimized.

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Feb 9, 2021

Just wanted to mention:

some things are representable ... but never occur in parsing.

To formulate tersely: Expr may ("if we want") represent only literate Nix grammar. Expr by what it represents should allow any complete Nix grammatically correct schizophasia, well, 'cause schizophasia is still grammatically correct expressions.

Any (majority) of smart constructor logic checking, even as simple mas uniqueness of keys in attrset, - is Eval which is of course before any corporeal Exec.

So the threads talks about grammar strictly.

Anton-Latukha added a commit that referenced this issue May 18, 2021
Once started organizing, it is hard to stop.

Somehow forgot to `bytestring 0.11`, thought we updated to it.

During it found & made a couple of small optimizations here and there.

And left notes for future to introduce breaking changes to provide more optimization. Some `NExprF` arguments need reordering (for performance) & synonymizing (for readability) (it is a close topic to the #377).

Organization in `Parser`. Use of `liftA*` in it.
Big reorganization in `Type.Infer`.

Sprinkled with some docs here and there.

declared `{AnnE, NExprLocF}` patterns as complete, which reduced a bunch of according bottoms & would allow GHC to optimize.

Used `AnnE` pattern across the code, and added docs so it is understandable what it is.
@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Jul 20, 2021

@infinisil

So, 2.5 years in the making. I arrived to it.

The major part of your proposition, several points of it depends on the - that binding key names should differentiate between static & dynamic, and a lot follows from that.
Recently I've made VarName a proper abstraction, so now what asked would be easier.


I looked into the suggestion & so far have not understood the vector of them. Well, I understand the direction, I do not understand how abstractions align in the suggestion, there may be a typo around use of NKeyName (since proposition, iiac, is about splitting it into two separate types for Static & Dynamic.)


Inherit should be Inherit !(Maybe r) ![VarName] !SourcePos

VarName is an abstraction on Text (formerly was alias):

🅸 ~/s/n/pkgs:master◦>nrg 'inherit.*\$'
pkgs/applications/kde/default.nix
    inherit (srcs.${pname}) src version;

pkgs/stdenv/darwin/default.nix
    inherit (pkgs."${finalLlvmPackages}") compiler-rt;

So, inherit accepts dynamic keys also, they can not be put into the VarName (Text) abstraction, or otherwise someone has some secret insight why it should be so. That one seems false.


I would agree with this:

type StaticAttributes r = Map VarName (StaticAttributeValue r)
type DynamicAttributes r = [(NDynamicAttrPath r, r)]

The current is:

hnix/src/Nix/Expr/Types.hs

Lines 289 to 297 in f92a283

data NKeyName r
= DynamicKey !(Antiquoted (NString r) r)
-- ^
-- > DynamicKey (Plain (DoubleQuoted [Plain "x"])) ~ "x"
-- > DynamicKey (Antiquoted x) ~ ${x}
-- > DynamicKey (Plain (DoubleQuoted [Antiquoted x])) ~ "${x}"
| StaticKey !VarName
-- ^
-- > StaticKey "x" ~ x

Separating statics from dynamics is a good idea & is a guide for further improvements.

@infinisil
Copy link
Contributor Author

Inherit should be Inherit !(Maybe r) ![VarName] !SourcePos

VarName is an abstraction on Text (formerly was alias):

🅸 ~/s/n/pkgs:master◦>nrg 'inherit.*\$'
pkgs/applications/kde/default.nix
    inherit (srcs.${pname}) src version;

pkgs/stdenv/darwin/default.nix
    inherit (pkgs."${finalLlvmPackages}") compiler-rt;

So, inherit accepts dynamic keys also, they can not be put into the VarName (Text) abstraction, or otherwise someone has some secret insight why it should be so. That one seems false.

inherit (foo) bar baz;

is

Inherit (Just (NSym "foo")) ["bar", "baz"]

The Maybe r corresponds to the (...) part of inherit, not the VarName's.

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Jul 20, 2021

Thank you for correcting. That is true.

@Anton-Latukha
Copy link
Collaborator

Our discussion on (inherit):
Checked, (almost) completed the migration:

By guys this was done in this low orbit ion cannon way, because the language allows:

> a = 3       
> {inherit "a";}  # with double qotes 
{ a = 3; }

there is a single case of its use inside Nixpkgs:

namespaces = map (type: { inherit type; }) [ "pid" "network" "mount" "ipc" "uts" ];

So there are indeed no dynamic keys inheritance, but there is this allowance to enclose a static key in "", which complicates treating it as a plain Text.

Removing the dynamic key here is a proper action, but now VarName processing need to be expanded, or have a new abstraction simply for this quirk, otherwise, that quirk requires to split the NString (DoubleQuoted | Indented) datatype, but if we go that pass, quirks would require all data types to be split 8)

It is not a feature. This is a quirk, unneeded language complication semantic & complicates the understanding of the type system to user (my exp) it is easier to refactor that line in Nixpkgs & reduce a bug.

We must remember our main audience, it is Haskellers, they would appreciate the well-formed type system in the language.

For HNix it is wiser & easier to "forget" (add the property of forgetful functor on these particular types of cases (quirks that are vacuous, rare & unused)) & just lint them out of existence (otherwise - get a feature report on it). Suggest users a cleaner code & give them the proper type (to guide (also ones who learn) through the language type system, as I know what it was like).


I moved the {inherit "a";} into syntax mistakes, with a note that it was put there deliberately, so if something there is alive traceback path.

Anton-Latukha added a commit that referenced this issue Jul 21, 2021
Thread: #377 (comment)

`inherit x y` in y` position always takes a variable name.

Nix allows `inherit x "y"`, but there is no use (in the wild real life use) for
it, it seems a misfeature and would be considered a quirk of the original type
system/implementation, until the use case of it would be clear (which is hard,
since there is a single use of it in Nixpkgs, which is mentioned in the thread).
Anton-Latukha added a commit that referenced this issue Jul 21, 2021
Thread: #377 (comment)

`inherit x y` in y` position always takes a variable name.

Nix allows `inherit x "y"`, but there is no use (in the wild real life use) for
it, it seems a misfeature and would be considered a quirk of the original type
system/implementation, until the use case of it would be clear (which is hard,
since there is a single use of it in Nixpkgs, which is mentioned in the thread).
@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Jul 21, 2021

regarding

type DynamicVarName   r = (Antiquoted (NString r) r)
NAttrPath -> newtype NStaticAttrPath (NonEmpty (VarName)) | newtype NDynamicAttrPath NonEmpty (DynamicVarName r)
data StaticAttributeValue r
  = NamedVar [NKeyName r] r SourcePos
  | Inherit (Maybe r) SourcePos
newtype StaticAttributes r = StaticAttributes (HashMap VarName (StaticAttributeValue r))
newtype DynamicAttributes r = DynamicAttributes [(NDynamicAttrPath r, r)]
data Bindings r = Bindings !(StaticAttributes r) (DynamicAttributes r)

I started, but would finish a bit later - for obvious reasons. The data type change is massive, while also needs to land properly, types need to be chosen to keep instance inference & whole code needs to be refactored.

If I would receive a hunch on the data types to use - would appreciate it.

@infinisil
Copy link
Contributor Author

infinisil commented Jul 21, 2021

It looks like Nix treats "str" and ${"str"} when possible as just str at the parser level:

$ nix-instantiate --parse -E 'a: { inherit ${"a"}; }'
(a: { inherit a ; })
$ nix-instantiate --parse -E 'a: { inherit "a"; }'
(a: { inherit a ; })
$ nix-instantiate --parse -E 'a: { inherit a; }'
(a: { inherit a ; })

@layus
Copy link
Collaborator

layus commented Jul 21, 2021

Yes, it has some simplification logic in the parser.

Which has an impact on semantics, because it make it tolerate one level of dynamic attributes, but fail on nested ones. 'a: { inherit "a"; }'is weird.

$ nix-instantiate --parse -E 'a: { inherit ${"${"a"}"}; }'
error: dynamic attributes not allowed in inherit at (string):1:14

See https://github.com/NixOS/nix/blob/e6150de90d8db101209fc6363f5f7696ee8192c4/src/libexpr/parser.y#L501-L522 and string_parts_interpolated.

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Jul 22, 2021

We can do it through additional abstraction & the very same coerce, coercing ever on these.

But why?

My current position

My current way is refactoring (clean-up & simplification) of the code. I am interested in how clean & how far HNix code can go implementing the alike preferentially transparent language.

This thread topic is "improvements to NExpr", not "how much of quirks we can duplicate". The quirks that need to be duplicated would show themselves during work.

*these syntax quirks is what created deep puzzlement in me & strongly slowed the learning of the language. Wondering on every corner on these types of strange semantic inflaxions. So inherit takes what? Strings, quasiquoted variables - what for ("I seem to not know something"), & that "I seem to not know it" "I seem to not know why", "I seem to not know something about it" haunted & still haunts me about Nix. "Aha - you did not expected this! - It is you fault." No, it is not my or users fault.

So since inherit in 99% of cases is a key - I would recommend keep it & expect a key, & ask to supply a key, until someone comes around & gives a genuine argument, that is not "it was done under a blanket in this way years ago, so you must follow", but a genuine example why that semantic is needed.

Keeping the language semantics simple & clean is a core piece to have a great productive language & tooling. For example - less code, simpler code, precise type inference. Keeping language semantics in check is especially important when we are having very limited resources.

In other words - I am happy with the decision to put proper clear use & in the cases where the 99% of use is this way - we need to not imagine & give the otherwise check case to the reality.

Not every time I decide to reduce, for example I put a lot of effort in carefully preserving the replaceStringsNix quirks during refactoring. But even there, sometimes I think maybe I needed to bite the bullet & tell people that:

builtins.replaceStrings ["" "e"] [" " "i"] "Hello world"
" H e l l o   w o r l d "

Is definitely not "a feature". If we'd to reduce it - the replaceStrings simplifies drastically & from some "zygohistomorphic prepromorphisms" (with complex corecursion) it turns into a simple catamorphism.

In short - those are obviously not features, those are bugs that are better (in an ideal situation) to not be there. "A feature" that is not used at all is generally a misfeature and can be reduced (not in terms of GNOME degree of feature removal).
We may give users a service improving the language a bit here and there where possible, where language & its use gives way.
So we need to think if replicate cargoing-in a particular bug is worth it, or it is worth looking at it realistically & abstain from replication.

A good software design is to postpone this type of (especially this particular) decision until would be necessary to make a decision (if ever - reality would ask for it to be implemented). Haskell allows that path very much.

@Anton-Latukha

This comment has been minimized.

@sjakobi

This comment has been minimized.

@Anton-Latukha

This comment has been minimized.

@sjakobi

This comment has been minimized.

@sjakobi

This comment has been minimized.

@Anton-Latukha

This comment has been minimized.

@Anton-Latukha

This comment has been minimized.

@jwiegley

This comment has been minimized.

Anton-Latukha added a commit to Anton-Latukha/hnix that referenced this issue Jan 21, 2022
This commit takes-put the NApp out of NBinaryOp & places it as a proper citizen
on NExprF.

Addresses haskell-nix#1041 &
haskell-nix#377.
@Anton-Latukha

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants