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

Feature: Indent multiline antiquotations (v3) #2490

Closed
wants to merge 4 commits into from
Closed

Feature: Indent multiline antiquotations (v3) #2490

wants to merge 4 commits into from

Conversation

layus
Copy link
Member

@layus layus commented Oct 26, 2018 •
edited

After a long discussion in #580, it appears that there are two safe paths to introduce this feature. Either use a custom operator for antiquotations (like ${> ... } for example) or introduce pragma-like statements to change the semantics on a per-file basis, and in a backward compatible manner. Again, see #580 for details.

In this PR, I implemented the pragma idea introduced by @copumpkin [1]. This seems better than introducing a new operator. I am proposing a semantics change, not a new operator. Oh, and also because I agree with @vcunat's [2].

To get this new semantics within a file, one needs to add /*@ LANGUAGE reindent @*/ at the top of the file. That's it. This implementation is not a mass rebuild, and does not break anything.

"Smart antiquotations", tersely renamed to the "reindent" language feature in this PR are antiquotations alone on a line of a multiline nix string (a.k.a. ''-strings). When that occurs, the whole inserted block gets reindented to match the indentation level of the antiquotation itself.

This means that

/*@ LANGUAGE reindent @*/
let
  inside = "a\nb\nc";
in ''
  BEGIN
    ${inside}
    BEGIN
      ${inside}
    END
  END
''

ends up looking like

''
BEGIN
  a
  b
  c
  BEGIN
    a
    b
    c
  END
END
''

instead of the old

''
BEGIN
  a
b
c
  BEGIN
    a
b
c
  END
END
''

and we can get rid of code like https://github.com/NixOS/nixpkgs/blob/8070a6333f3fc41ef93c2b0e07f999459615cc8d/nixos/modules/services/x11/xserver.nix#L76-L86

Things that need discussion

The actual syntax for pragmas can still be changed, but will be introduced once and for all here.

This PR needs rebasing onto smaller changes yet to merge and included here for simplicity.

The code is available for testing on layus.cachix.org as /nix/store/73fi8m9zylxc4g72r4h55wcyr99p2skh-nix-2.2pre0_0000000

layus and others added 4 commits October 27, 2018 14:04
@layus
Parse indented strings lines as separate entities.
242158b
@layus
Indent multiline antiquotations in indented strings.
9e2083e
@layus
Implement the reindent pragma (finally!)
7fccb5e
@layus
Make pragmas slightly more generic
5ee845c 
Singles lines matching "/*@ ... @*/" are recognised as pragmas, and
error out if they are not a reindent pragma.
This helps making pragmas feel more integrated in the language,
and prevents any uses of simmilar syntax for other uses in the meantime.

This does not however further refine the pragma syntax, which remains
open at the moment, except for the fixed "LANGUAGE reindent" string.
@shlevy
Copy link
Member

shlevy commented Oct 27, 2018

@layus I think we should separate this into independent discussions

  1. Should we have conditional evaluation modes of some kind? If so, what should the interface be?
  2. Should we fix indentation somehow? If so, how should we expose that?

IMO for number 1 I think the answer is yes. I think pragmas are a decent approach (they work well enough for haskell), and if/when we have package specifications for nix libraries we can set project-wide settings there as well.

For number 2, I think we should fix it somehow but I'm completely ambivalent between having it as a pragma vs a different quotation syntax.

@NixOS/nix-core can y'all chime in on these questions and we can try to make a decision soon here?

@domenkozar
Copy link
Member

domenkozar commented Oct 30, 2018 •
edited

I'm not in Nix core team, but then we can really forever say goodbye for smooth upgrades. Even today upgrading from NixOS version to another, Nix fails due to some compatibility features. I guess we should fix that before making more of them :)

@domenkozar
Copy link
Member

Thinking about it more, the extensions would make the errors better (eg. NixOS/nixpkgs#36268 (comment)). But this should have an RFC since it's introducing a big change to the language.

@edolstra
Copy link
Member

@domenkozar This feature should not be a problem for NixOS upgrades because it only affects evaluation, and nixos-rebuild already fetches the latest stable Nix for evaluation. The problem is when new build features are required (as is the case for structured derivation attributes).

BTW, instead of using magic comments, which are always an ugly hack, it seems better to use something like

require feature reindent;

since then the file will fail to parse with older versions of Nix. However I really don't fancy adding annotations like that to thousands of Nix expressions. (See also https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md#build-tags for some discussion of this style.) If we're going to implement flakes, then it's probably best to specify a language version in the flake file, similar to Rust's epoch mechanism.

@shlevy
Copy link
Member

shlevy commented Oct 31, 2018

I don't think it's mutually exclusive to do it per-file or per-flake (GHC has both, for example). IMO though it's nicer to specify a set of extensions (possibly with some bundles of common extensions for convenience) than a version.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/would-you-like-to-use-nix-instead-of-consul-template-templating-language/5145/1

@stale
Copy link

stale bot commented Feb 13, 2021

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

@stale stale bot added the stale label Feb 13, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

I closed this issue due to inactivity. → More info

@stale stale bot closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@layus @shlevy @domenkozar @edolstra @nixos-discourse