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

Chained methods with closures that have a body leads to incorrect indentation #900

Open
rrousselGit opened this issue Mar 12, 2020 · 6 comments

Comments

@rrousselGit
Copy link

Similar to #137, but for chained methods

Consider:

final commonParameters = constructrorsNeedsGeneration
    .first.parameters.allParameters
    .where((parameter) {
  return constructrorsNeedsGeneration.every((constructor) {
    return constructor.parameters.allParameters.any((p) {
      return p.name == parameter.name && p.type == parameter.type;
    });
  });
}).toList();

The indentation of the body of the .where clause is unexpected (2 spaces instead of 6)

I would expect:

final commonParameters = constructrorsNeedsGeneration
    .first.parameters.allParameters
    .where((parameter) {
      return constructrorsNeedsGeneration.every((constructor) {
        return constructor.parameters.allParameters.any((p) {
          return p.name == parameter.name && p.type == parameter.type;
        });
      });
    }).toList();

Note that this only happens with closures using {}. Closures using => don't have this issue and are formatted this way:

final commonParameters = constructrorsNeedsGeneration
    .first.parameters.allParameters
    .where((parameter) => constructrorsNeedsGeneration.every(
        (constructor) => constructor.parameters.allParameters.any(
            (p) => p.name == parameter.name && p.type == parameter.type)))
    .toList();
@jolleekin
Copy link

It's a problem with formatting closures with a code block in general.

// ACTUAL

import 'dart:io';

void main() {
  final f = stdin.readLineSync,
      p = int.parse,
      n = p(f()),
      ranges = List.generate(n, (_) {
    final t = f().split(' '), a = p(t[0]), b = a + p(t[1]);
    return [a, b];
  }),
      dp = List<int>(n);
}



// EXPECTED

import 'dart:io';

void main() {
  final f = stdin.readLineSync,
      p = int.parse,
      n = p(f()),
      ranges = List.generate(n, (_) {
        final t = f().split(' '), a = p(t[0]), b = p(t[1]);
        return [a, b];
      }),
      dp = List<int>(n);
}

@munificent
Copy link
Member

@rrousselGit, your example is "deliberate" in that I'm not surprised it's what the formatter does, but I agree it doesn't look great. The challenge is that there are many cases where not indenting lambdas is clearly much better. The test API is the canonical example. It would be a drag if every lambda passed to test() and group() had its entire body indented +4. So the formatter has some heuristics to decide when to give lambdas "block-like" indentation versus normal expression indentation. Those heuristics work well most of the time, but definitely have some failure cases like your example here.

This is one of the more complex parts of the formatter and it interacts with the line splitting algorithm in subtle ways, so it's hard to improve cases like yours without either making more other examples worse or causing performance problems. It's an area I'd love to revisit and try to totally overhaul, but I haven't had the time.

@jolleekin, your example is different. This one is just about not increasing the indentation level in multiple-variable declarations. It's very rare that users declare multiple variables with a single statement in Dart and even rarer for one of the initializers to have a complex body like your example here, so I think I just never ran into this case and decided how to handle it.

A simple workaround, which is more idiomatic in Dart anyway, is to change your code to:

void main() {
  final f = stdin.readLineSync;
  final p = int.parse;
  final n = p(f());
  final ranges = List.generate(n, (_) {
    final t = f().split(' '), a = p(t[0]), b = p(t[1]);
    return [a, b];
  });
  final dp = List<int>(n);
}

munificent added a commit that referenced this issue Mar 20, 2020
This fixes the example in the second comment on issue #900, but not
the main issue.
@jolleekin
Copy link

@munificent Of course I know it will work if I just use single variable declarations. The reason I use the multi-variable declarations is because it's much shorter and faster to type, which I find very convenient in solving programming challenges.

@munificent
Copy link
Member

OK, I've landed a change that fixes your case. It will go out in the next release of dartfmt.

@passsy
Copy link

passsy commented Mar 24, 2020

Nice. Does this also close #755?

@munificent
Copy link
Member

No, it only addresses leading indentation in multiple variable declarations. Indentation in method chains is a much harder problem.

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

4 participants