Continue to label that is not for a loop

[scheglov]

Is this valid code?

int findOverlap(List list, List sublist) {
  for (int i = 0; i < list.length; ++i) {
    outer:
    {
      for (int j = 0; j < sublist.length && i + j < list.length; ++j) {
        if (list[i + j] != sublist[j]) continue outer;
      }
      return i;
    }
  }
  return list.length;
}

Here, outer labels the block, not for (int i. So, according to 18.15 Continue in the specification, continue outer; should be a compile-time error? But this code runs :-)

And the analyzer also does not report it as an error.
And I suspect that it might do something unexpected for flow analysis as well, possibly because of how the analyzer drives it.

https://github.com/dart-lang/sdk/blob/578b602deca0790f4e2bf25acaaa9571ed17192c/pkg/vm/lib/transformations/type_flow/utils.dart#L322-L330

[munificent]

Here, outer labels the block, not for (int i. So, according to 18.15 Continue in the specification, continue outer; should be a compile-time error? But this code runs :-)

😱

I still think we should just get rid of labels entirely. :)

[jensjoha]

I still think we should just get rid of labels entirely. :)

Please don't.

[osa1]

Shouldn't this be reported as an SDK bug?

[eernstg]

So, according to 18.15 Continue in the specification, continue outer; should be a compile-time error?

Right, as specified it should be flagged as a compile-time error, because continue L; is only defined to restart loops, and to jump to a case in a switch. However, I don't see any particular problems in allowing it to restart an arbitrary labeled statement. The form continue; would still require a loop.

So we could report the error (and deal with the breakage), or we could generalize the spec to allow it. It is a tiny generalization, and if it is actually working already then it seems reasonable to generalize the spec.

@munificent, @kallentu, @natebosch, @stereotype441, @lrhn, @leafpetersen, @jakemac53, WDYT? Presumably the first step would be to create a language test and verify that it is actually working.

[Jetz72]

Does using continue like this actually cause a different behavior than break? I tried throwing together a few simple test loops but it didn't seem to make much difference in practice.

for(int i = 0; i == 0; i++) //Useless loop, but the continue is illegal outside of it.
{
  what: {
    print("hello");
    continue what;
    print("dead code");
  }
  print("goodbye"); //Oddly enough this gets flagged as dead code, even though it runs fine.
}

I have found label-break outside of loops useful at times, particularly if a large section of code has multiple cases where it needs to be aborted prematurely. Usually that's just a cue to make a new function and use return but that isn't always viable or pretty. Allowing label-continue to restart an arbitrary block seems suitably analogous to label-break's behavior of exiting it, so in that respect it seems like a natural feature to have. However, expanding in that direction does feel like a slippery slope, at the bottom of which lies the dreaded goto statement...

[eernstg]

continue L; should jump to the beginning of the target statement, break L; should jump to the beginning of the next statement (the statement that follows the one which is labeled L). But I can see that continue L; actually jumps to the next statement, so the assumption that "it works already" isn't true after all.

In that case it's probably better to start reporting the compile-time error for continue L; when L isn't the label of an enclosing loop or of a case in an enclosing switch. Surely there is no need for two different syntaxes meaning break L;.

[lrhn]

I'd be vary about allowing continue to continue an arbitrary block. The semantics of continue of a loop, so far, has been to break the loop body, which brings us back to the loop condition (or increment-then-condition, for for(;;)).
Allowing continue to restart any block means that any block can be a loop. I think I prefer the while (true) + break approach to that, so I can see that the block is a loop before I start reading it. A labeled block statement is then recognizable as being a break-target only.

Also, that's not what it currently does. As @Jetz72 says, it looks like a continue to any label between the loop header and the continue works as a break instead.

void main() {
  // label:
  {
    for (var i = 0; i < 3; i++) {
      print("a:$i");
      label:
      {
        for (var j = 0; j < 2; j++) {
          print("b:$i:$j");
          // label:
          continue label;
          print("c:$i:$j");
        }
      }
      print("d:$i");
    }
    print("e");
  }
  print("done");
}

Uncommenting different label:s here shows that continue label works just like break label.

Not going to try to guess why (who am I kidding, I'm guessing we desugar continue to break, and forget to check the validity before desugaring). The tl;dr is that we forget to report an error where the continue target is not valid, and we do this consistently across backends, so it's probably something which should happen fairly early.

Also, the analyzer thinks the print("c:$i:$j"); is dead code, even when the label: on the continue is uncommented, which suggests we might have soundness issues as well. (It thinks the code is dead for label: break label; as well, which is just wrong. It's not about continue.)

And yes, it's an implementation bug, so it should really be in the SDK repo.

[munificent]

I still think we should just get rid of labels entirely. :)

Please don't.

Why not? Do you have use cases for it?

[lrhn]

I still think we should just get rid of labels entirely. :)

Please don't.

Why not? Do you have use cases for it?

I use labels often enough that I don't want to lose them.

If we remove break entirely from the language, then I think we can remove labels too. 😉

Until that, if we accept that you can break out of a loop, it would be a design flaw if you couldn't break a loop anyway, just because the break is inside another breakable construct (say a switch, it doesn't even have to be nested loops).
That's why we need to be able to specify which surrounding construct is being broken from. A plain break; always picks the closest enclosing one, and that's too restrictive. Labels on loops allow us to break other loops too, and is really needed in order to not arbitrarily handicapping the feature.

We could just remove labels from constructs other than loops and switches then. That should simplify things. Right?
Yes and no. It would not change what you can write since do { ... } while (false) is a loop that is equivalent to { ... }.

A cases for breaks, which is not just breaking from an outer loop (from #171):

var particularElement;
found: {
  for (var e in someList) {
    if (predicate(e)) {
      particularElement = e;
      break found;
     }
  }
  return;
}
useElement(particularElement);

Sure, I could just inline everything after the found: { ... } block inside the loop, but that's not particularly readable either.
(This code actually isn't as useful any more with null safety.)

Let's keep labels, they are there for a good reason.

I'd be fine with disallowing labels on non-composite statements (maybe only on loops/switch/block-statements, not even try/catch), allowing label: break label; was never useful.
However, if we ever decide to change break to be an expression, like we did for throw, then that might come back and bite us.

[stereotype441]

(Still catching up on email after a week vacation) I'm in favor of removing support for continue to arbitrary labels. As documented above (and in dart-lang/sdk#49852) it doesn't work as expected, and it leads to unsoundness in flow analysis.

As far as the question of whether we should remove label support entirely, maybe we should discuss that in a separate issue. But as long as we're discussing it here, here's my opinion. Labels are definitely an obscure feature, and I hardly ever use them (digging through my own code I could only find one example). If the language didn't already support them, I would definitely argue against adding them; our time and energy is better spent on other features. However, I don't really see a lot of benefit to removing them either. It would not simplify our implementations very much, and given how little they're used in pracice I don't think it would really ease cognitive burden for our customers very much either. But what it would do is frustrate a lot of existing customers by forcing them to go to one or two spots in their program that use labels, possibly in areas of their code they haven't touched in a while, and rewrite them in a non-trivial way, for no obvious benefit to themselves. I think it's important not to create that kind of customer frustration without a clear benefit.

[xhc-code]

I don't know why I didn't see the usage and explanation of the break tag in Dart's tutorial. Is this a bug in the documentation?

[munificent]

continue and break to label are very rarely used features, so I think it's probably a deliberate omission that the tutorial doesn't talk about them.

[rrousselGit]

continue and break to label are very rarely used features, so I think it's probably a deliberate omission that the tutorial doesn't talk about them.

Typical Chicken and Egg scenario :p

It's in part used rarely because folks don't know labels exist.

[xhc-code]

continue and break to label are very rarely used features, so I think it's probably a deliberate omission that the tutorial doesn't talk about them.

Okay, I've looked at the official documentation a few times and didn't find any support for the break tag. I thought it was not supported, but seeing other people's code using it, I felt a contradiction. Although it's not a complicated syntax, I think the official website should be comprehensive.