String-subst brackets `${` / `}` misaligned

[DavHau]

In the result of this: https://kamadorueda.github.io/alejandra/?before=%27%27%0A++dream2nix.makeOutputs+%7B%0A++++source+%3D+.%2Fdream-lock.json%3B%0A++++%24%7Blib.optionalString+%28dreamLock.sources.%22%24%7BdefaultPackage%7D%22.%22%24%7BdefaultPackageVersion%7D%22.type+%3D%3D+%22unknown%22%29+%27%27%0A++++++sourceOverrides+%3D+oldSources%3A+%7B%0A++++++++++%22%24%7BdefaultPackage%7D%22.%22%24%7BdefaultPackageVersion%7D%22+%3D+.%2F%24%7BsourcePathRelative%7D%3B%0A++++++++%7D%3B%0A++++%27%27%7D%0A++%7D%0A%27%27%0A

... the closing bracket in line 10 is not aligned with the opening bracket in line 4:

''
  dream2nix.makeOutputs {
    source = ./dream-lock.json;
    ${  # <-- open
    lib.optionalString (dreamLock.sources."${defaultPackage}"."${defaultPackageVersion}".type == "unknown") ''
      sourceOverrides = oldSources: {
          "${defaultPackage}"."${defaultPackageVersion}" = ./${sourcePathRelative};
        };
    ''
  }  # < -- close
  }
''

[kamadorueda]

Misalignment can happen, see for instance:

{
  something = {
    # ...
  };
}

Hanging behavior is common in configuration languages, let me explain why:

The alternative is not optimal. Suppose that you rename the variable to something shorter/longer, should you now move all the attr-set to the left/right?

{
  before =  {
              # many elements
            }; # Should we align in multiples of 2? 
               # or perfectly with `{` even if this is an odd number

  after = {
            # many elements
          };
}

Aligning exposes us to:

• More horizontal space taken
• (Align everything) or (inconsistency - having some elements aligned and others not)
• Bigger diffs && weaker git blame, should you wish to change a variable name or something like that in the future

Now, anyway I thought it could be a good idea so I was implementing this in https://github.com/kamadorueda/alejandra/commit/3594dedc027c5a027626bca09ed4a7abe2883d3f and found cases like this in the Nixpkgs diff:

let
in ''
  node ${pinpointDependenciesFromPackageJSON} ${
                                                  if production
                                                  then "production"
                                                  else "development"
                                               }
''

One may argue that the following is better:

let
in ''
  node ${pinpointDependenciesFromPackageJSON} ${
    if production
    then "production"
    else "development"
                                               }
''

But this exposes us to:

• (Align everything) or (inconsistency - having some elements aligned and others not)
• It's surprising that the contents are before the "mental vertical ruler" traced by the opener/closer bracket

I suppose that in your case, the appropriate refactor would be along the lines of:

let
  isUnknown = dreamLock.sources."${defaultPackage}"."${defaultPackageVersion}".type == "unknown";

  sourceOverrides = ''
    sourceOverrides = oldSources: {
      "${defaultPackage}"."${defaultPackageVersion}" = ./${sourcePathRelative};
    };
  '';
in ''
  dream2nix.makeOutputs {
    source = ./dream-lock.json;
    ${lib.optionalString isUnknown sourceOverrides}
  }
''

[kamadorueda]

I suppose it looks miss-aligned in your example because of the context around it (a stringified nix code), but that context is a string, we cannot format it, and all we see in the AST is a chunk of bytes without meaning

Let me replace the context with other string:

''
  x = ${
    expression
  };
''

Suddenly it does not look miss-aligned but just like any other regular attribute set

[tomberek]

Looking this example the issue seems to be related to the expected layout of things inside of a multiline string. If you play with the spacing at the beginnings of line 2,4, and 10 there is a surprising behavior because of this.

In the original example, i don't think there is any relationship intended between the open and close, it is following the rule for multiline string indentation.

[DavHau]

Thanks for the detailed explanations and experiments. I now understand that alejandra cannot reason about the string context and therefore doesn't know which is the right position for the closing string-subst bracket.

Let me summarize:

• The problem only occurs whenever there is an indented multi-line string substitution within a multi-line string.
• This is specifically relevant when using nix for templating.
• Whatever fixed rule alejandra uses for the closing bracket position, it will always be good for some cases and break others
• As of current behavior, the only reliable way to circumvent the problem is to re-factor the code to not use a multi-line string substitution anymore
• Indentation of the closing bracket does not influence the drv hash (I just verified this).

Simplified example:

pkgs.writeText "test"
''
  # some nix template
  let
    test = ${lib.optionalString true ''
      content
    ''}
  in
    test
''

In this example, shifting the closing bracket to the left, will look wrong, as well as shifting it to the right to align it with the opening bracket.

I'd argue, that when there is no way for alejandra to know if a change increases or decreases readability, then there is no good reason for alejandra to change something. So, why not leave it up to the user? Of course, whenever alejandra decides to shift the opening bracket, the closing one should be shifted accordingly.

So the rule could be:

• In multi line string substitutions within multi-line strings, never change the horizontal offset between the opening/closing brackets/quotation marks.

[spikespaz]

Not sure if this is the same problem or if it needs a new issue, but right around lib.makeBinPath looks terrible

{
  # maintainers,
  lib,
  stdenv,
  makeWrapper,
  bash,
  coreutils,
  dbus,
  libnotify,
  evtest,
  # expected to be overridden
  disableDevices ? [],
}:
stdenv.mkDerivation {
  pname = "wayland-disable-keyboard";
  version = "0.0.1";

  strictDeps = true;

  src = ./.;

  nativeBuildInputs = [makeWrapper];

  installPhase = ''
    runHook preInstall

    install -Dm755 disable_kb.sh $out/bin/wayland-disable-keyboard
    wrapProgram $out/bin/wayland-disable-keyboard \
      --set PATH \
        '/run/wrappers/bin${lib.makeBinPath [
      bash
      coreutils
      dbus
      libnotify
      evtest
    ]}' \
      --set DISABLE_DEVICES '${lib.concatStringsSep ":" disableDevices}'

    runHook postInstall
  '';

  meta = {
    description = lib.mdDoc ''
      Simple utility to temporarily disable the keyboard
      (and other input devices). Requires `disableDevices` to be
      passed as an argument to the package, usually via override.
    '';
    license = lib.licenses.mit;
    platforms = lib.platforms.linux;
    # maintainers = with maintainers; [spikespaz];
  };
}

[spikespaz]

Still wanting this to look better. Keep being bothered by the issue. Option to turn multiline string formatting off, or perhaps special comment syntax to disable formatting for a block?

    listenerScript = pkgs.writeText "hyprland-event-listener" ''
      socket=/tmp/hypr/$HYPRLAND_INSTANCE_SIGNATURE/.socket2.sock
      echo "INFO: opening socket: $socket"

      ${lib.getExe pkgs.socat} - UNIX-CONNECT:$socket | while read line; do
        ${
        lib.concatStrings (lib.mapAttrsToList (_: info: ''
            if [[ $line =~ ${mkEventRegex info} ]]; then
              ${
              lib.concatStringsSep "\n  " (
                enumerate
                (i: v: "export ${v}=\"$BASH_REMATCH[${toString i}]\"")
                info.vars
              )
            }
              ${info.script}
              continue
            fi
          '')
          handlerInfos)
      }
        echo "WARNING: unhandled event: $line"
      done
    '';

    listenerScript = pkgs.writeText "hyprland-event-listener" ''
      socket=/tmp/hypr/$HYPRLAND_INSTANCE_SIGNATURE/.socket2.sock
      echo "INFO: opening socket: $socket"

      ${lib.getExe pkgs.socat} - UNIX-CONNECT:$socket | while read line; do
        ${lib.concatStrings (lib.mapAttrsToList (_: info: ''
          if [[ $line =~ ${mkEventRegex info} ]]; then
            ${lib.concatStringsSep "\n  " (
            enumerate
            (i: v: "export ${v}=\"$BASH_REMATCH[${toString i}]\"")
            info.vars
          )}
            ${info.script}
            continue
          fi
        '')
        handlerInfos)}
        echo "WARNING: unhandled event: $line"
      done
    '';

Just can't get it to look right. It bothers me.