False negative for curly_braces_in_control_flow_structures

[lrhn]

The lint curly_braces_in_control_flow_structures should warn if an if statement body has no braces.
An exception is granted if the entire if statement fits in one line, based on the corresponding Effective Dart rule.

No lint warning is given for the if statement of this code:

Object? foo() {  
  if ("long condition and other long condition" != "another long condition" &&
     "more tests".isNotEmpty) return null;
  return 42;
}

The body of this if statement fits in one line, but the entire statement does not, so the lint should trigger.
It does not.

(Found this by seeing similar code in a code review.)

[navaronbracke]

But what if someone wants to enforce that curly_braces_in_control_flow_structures enforces all control flow to have braces?
Due to the exception in Effective Dart, the statement that one should use braces for strictly all statements is false, because there is an exception.

IMO, the whole point of the lint was to avoid if (condition) return; ? (yes, the exception in the Style Guide is a bit unfortunate)

Personally, I'd like to keep the lint strict and keep the old behaviour. If someone wants to allow if (condition) return; then they should just disable the lint?

[srawlins]

But what if someone wants to enforce that curly_braces_in_control_flow_structures enforces all control flow to have braces?

I don't think that's related to this issue...

[navaronbracke]

With the change proposal above, the statement if (condition) return; would not be flagged by the lint (if it's less than the configured line length), per the rule in the style guide. However, I propose to keep the current behavior which lints on if (condition) return; regardless of the line length for the full statement.

[srawlins]

if (condition) return; is not flagged today though. The change proposal above would not change that.

Here's the test case.

[navaronbracke]

Ah, my mistake. Should I file a new issue for having it linted?

[srawlins]

Yes please :D.

Without a backing Effective Dart rule though, we are unlikely to add a style-oriented rule, or change the Effective Dart rule. You can file a request to change the style guide at https://github.com/dart-lang/site-www/issues.

[lrhn]

Agree. The issue here is that the expected exception seems to be implemented by checking that the ) ending the condition and the end of the statement are on the same line.
It should instead check that the if and the end of the statement is on the same line, since that's what's required to check that "the entire if statement is on a single line".
(I checked the code, that's precisely what happens. Fix seems easy, so I wrote that too.

[srawlins]

Sorry, forgot to mention I've been working on this and cleaning up the SDK. https://dart-review.googlesource.com/c/sdk/+/353140

But it will take a lot of work to land. Google internal needs a lot of cleanup.

[lrhn]

SGTM, ship it! (But yes, signficant clean-up will likely be needed.)

Feel free to take the extra tests I added, if you think they can be useful.

[srawlins]

Fixed by FMorschel/sdk@2c6dec6

[FMorschel]

Just to be clear, I've nothing to do with the fix. I'm not sure why, but some of the time when I rebase my fork on GH it keeps warning issues about changes from other people but it is simply because the link is commented on the commit description. Sorry for the confusion!