use_string_buffers shouldn't lint if variable is used in the loop

[a14n]

From Flutter https://github.com/flutter/flutter/blob/e216004b864b3c7374f4613718df2c99646c14b4/packages/flutter/lib/src/widgets/navigator.dart#L757-L765

      final List<String> routeParts = initialRouteName.split('/');
      if (initialRouteName.isNotEmpty) {
        String routeName = '';
        for (String part in routeParts) {
          routeName += '/$part';   // LINT
          plannedInitialRouteNames.add(routeName);
          plannedInitialRoutes.add(_routeNamed(routeName, allowNull: true));
        }
      }

This code shouldn't lint routeName.

[pq]

cc @alexeieleusis

[alexeieleusis]

I am not sure I agree with the requested change. Sounds like what they are trying to achieve there could be implemented in a cleaner way with a utility function like scan, which can perfectly be implemented for iterables.

I understand it all depends on the libraries you use, but they could even get rid of the if statement. Thoughts?

[pq]

cc @bwilkerson

[bwilkerson]

The lint isn't suggesting that the code should be converted to use scan, it's suggesting that the code should be converted to use StringBuffer. If scan were available (and I'm guessing it isn't), then a lint to suggest using it might make sense, but I think that would be a different lint.

So, the question is, which code is better, the original:

String routeName = '';
for (String part in routeParts) {
  routeName += '/$part';
  plannedInitialRouteNames.add(routeName);
  plannedInitialRoutes.add(_routeNamed(routeName, allowNull: true));
}

or the replacement:

StringBuffer routeBuffer = '';
for (String part in routeParts) {
  routeBuffer.write('/$part');
  String routeName = routeBuffer.toString();
  plannedInitialRouteNames.add(routeName);
  plannedInitialRoutes.add(_routeNamed(routeName, allowNull: true));
}

Personally, given the two choices, I would opt for the original. Is there a better replacement that I'm not thinking of, or a reason why the replacement is better?

[a14n]

/cc @Hixie ( re. https://github.com/flutter/flutter/pull/11581/files#r133030122 )

[Hixie]

The use of StringBuffer in general is intended to avoid expensive serialisations, but in this code, the serialisation happens anyway (and if StringBuffer doesn't cache, it might even be more expensive).

I think the lint should mute itself if it notices that there is only one += between each toString.

[alexeieleusis]

I think the lint shouldn't be changed, I will definitely won't oppose doing so if that is the consensus.

That said, I think the idea behind linting is to avoid common and frequent mistakes and there are cases where you don't want to follow the lint, the provided example is one of those if you can't use the provided suggestion

Iterable<S> scan<T, S>(Iterable<T> iterable, S initialValue, S combine(S s, T t)) {
  var acc = initialValue;
  return iterable.map((t) => acc = combine(acc, t));
}

The original example:

      final List<String> routeParts = initialRouteName.split('/');
      if (initialRouteName.isNotEmpty) {
        String routeName = '';
        for (String part in routeParts) {
          routeName += '/$part';   // LINT
          plannedInitialRouteNames.add(routeName);
          plannedInitialRoutes.add(_routeNamed(routeName, allowNull: true));
        }
      }

would convert into:

final List<String> routeParts = initialRouteName.split('/');
final parts = scan(routeParts, '', (acc, part) => '$acc/$part');
for (final part in parts) {
  plannedInitialRoutes.add(part);
  plannedInitialRoutes.add(_routeNamed(part, allowNull: true));
});

Just one occurrence of the pattern makes the code shorter even introducing the scan function, below a snippet trying the idea in dartpad:

Iterable<S> scan<T, S>(Iterable<T> iterable, S initialValue, S combine(S s, T t)) {
  var acc = initialValue;
  return iterable.map((t) => acc = combine(acc, t));
}

void main() {
  final path = 'this/is/an/artificial/example/but/works'.split('/');
  final breadcrumbs = scan(path, '', (acc, part) => '$acc/$part').toList();
  print(breadcrumbs);
  // [/this, /this/is, /this/is/an, /this/is/an/artificial, /this/is/an/artificial/example, /this/is/an/artificial/example/but, /this/is/an/artificial/example/but/works]
}

[bwilkerson]

I think the idea behind linting is to avoid common and frequent mistakes ...

That is certainly one of the uses cases for a linter, and the most valuable, imo. (Though I could quibble about the "common and frequent".)

... and there are cases where you don't want to follow the lint ...

True, but the goal should be zero false positives. Lint rules should only need to be ignored if the lint is valid but there is a reason why the rule should be violated in some rare situation.

The cases where you don't want to follow a lint need to be examined carefully, and if there are code patterns that can be statically detected that negate the value of the lint in all such cases, then the lint rule should detect those cases and not produce a lint. This appears to be such a case. I can't think of a time when it would be better to replace concatenation with a StringBuffer when the intermediate results are needed, especially with the limitation of still producing the lint if there is more than one concatenation between toStrings.

[Hixie]

@alexeieleusis As far as I can tell your code is less efficient than the original. It allocates significantly more memory, does two iterations instead of one, and does not have any fewer string concatenations. It's also somewhat more difficult to understand at a glance.

[alexeieleusis]

Apologies for the slow replies, I am in the final stage of a project.

@bwilkerson I agree with your reasoning, but I think the lint triggering is an opportunity to think about solving the problem in a different way that does not require a StringBuffer which certainly hurts performance in cases like this where you need the intermediate strings.

@Hixie In terms of readability, style and preferences come into play, I definitely think the proposal is an improvement starting from the fact that is declarative instead of imperative and is less code. But the coding style each project has is their choice. In terms of complexity I think they are both the same, timewise the iterable is traversed only once (see snippet below) and memory wise none of them can get around the fact that the expected output uses O(n^2) where n is the number of times / occurs in the original string.

void main() {
  int sideEffectsCount = 0;
  final path =
   'this/is/an/artificial/example/but/works'
    .split('/')
    .map((p) {
      sideEffectsCount += 1;
      return p;
    });

  String concat(acc, part) => '$acc/$part';
  final breadCrumbs = scan(path, '', concat);
 
  breadCrumbs.forEach(print);
  // Iterable is iterated only once!
  assert(sideEffectsCount == 7);
  print('sideEffectsCount: $sideEffectsCount');
}

Output:

/this
/this/is
/this/is/an
/this/is/an/artificial
/this/is/an/artificial/example
/this/is/an/artificial/example/but
/this/is/an/artificial/example/but/works
sideEffectsCount: 7

I can perform the needed change.

[Hixie]

I just don't see anything wrong with the original code. The lint shouldn't complain about it, and we shouldn't change the original code.

[alexeieleusis]

FWIW this lint does not trigger if routeName += '/$part'; is replaced by routeName = '$routeName/$part';.

[a14n]

@alexeieleusis As mentioned in this comment routeName = '$routeName/$part'; still produces a lint (see the test https://github.com/dart-lang/linter/blob/master/test/rules/use_string_buffers.dart#L30-L35).

[alexeieleusis]

I need to test again, but I was playing with the code and doing the change disabled the warning in intellij. Probably another bug in the rule?