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

Ensure comment formatting is idempotent. #1610

Merged
merged 4 commits into from
Dec 6, 2024
Merged

Conversation

munificent
Copy link
Member

In some cases, a line comment can appear between two tokens that otherwise never split, like after "if (" and before the condition. That leads to some tricky edge case behavior. If you formatted:

if (
  // Comment
  condition) {
  ;
}

It would see the newline before the comment and decide the comment was a "leading comment" which means it gets attached to the condition expression. Then the formatter would output it like:

if (// Comment
  condition) {
  ;
}

That's because leading comments don't write a newline before themselves. Then if you format that again, there's no newline before the //, so now it's a hanging comment. Hanging comments get a space before them, so you get:

if ( // Comment
  condition) {
  ;
}

Really, leading comments (as the name implies) are intended to always begin a line. So this PR makes sure they do that.

While I was at it, I modified the test runner to run the formatter twice on every test to ensure that everything is idempotent. That doesn't prove that the formatter will always produce idempotent output, but it at least gives us pretty good test coverage that it does behave idempotent-ly.

In practice, most normal looking code would never hit this edge case. You have to put a comment in an unusual spot where a split doesn't occur.

This still feels like a fairly brittle part of the formatter to me. Comments appearing between tokens that never split otherwise is handled on a pretty ad hoc basis (which is why some of the tests in this PR have weird indentation). I'd like a cleaner more systematic solution, but I'm not sure what that would look like.

Fix #1606.

In some cases, a line comment can appear between two tokens that otherwise never split, like after "if (" and before the condition. That leads to some tricky edge case behavior. If you formatted:

```dart
if (
  // Comment
  condition) {
  ;
}
```

It would see the newline before the comment and decide the comment was a "leading comment" which means it gets attached to the condition expression. Then the formatter would output it like:

```dart
if (// Comment
  condition) {
  ;
}
```

That's because leading comments don't write a newline before themselves. Then if you format that again, there's no newline before the `//`, so now it's a hanging comment. Hanging comments get a space before them, so you get:

```dart
if ( // Comment
  condition) {
  ;
}
```

Really, leading comments (as the name implies) are intended to always begin a line. So this PR makes sure they do that.

While I was at it, I modified the test runner to run the formatter twice on *every* test to ensure that everything is idempotent. That doesn't *prove* that the formatter will always produce idempotent output, but it at least gives us pretty good test coverage that it *does* behave idempotent-ly.

In practice, most normal looking code would never hit this edge case. You have to put a comment in an unusual spot where a split doesn't occur.

This still feels like a fairly brittle part of the formatter to me. Comments appearing between tokens that never split otherwise is handled on a pretty ad hoc basis (which is why some of the tests in this PR have weird indentation). I'd like a cleaner more systematic solution, but I'm not sure what that would look like.

Fix #1606.
@munificent munificent requested a review from natebosch December 5, 2024 01:35
lib/src/back_end/code_writer.dart Outdated Show resolved Hide resolved
test/utils.dart Outdated Show resolved Hide resolved
test/utils.dart Outdated Show resolved Hide resolved
munificent and others added 2 commits December 5, 2024 15:40
Co-authored-by: Nate Bosch <nbosch@google.com>
@munificent munificent merged commit 1208f9e into main Dec 6, 2024
7 checks passed
@munificent munificent deleted the idempotent-comments branch December 6, 2024 01:48
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

Successfully merging this pull request may close these issues.

Two runs needed to format with awkward comment
2 participants