Server format results can be incorrect if sent immediately after updateContent

[DanTup]

This came up at https://github.com/dart-lang/dart-pad/issues/3092 - the format tests for DartPad are flaky against main.

I can reproduce in an analysis server test at test\edit\format_test.dart:

@soloTest
Future<void> test_danny() async {
  var content = 'void main() {}';
  var updatedContent = "void main() {\n  print('hello world');\n}\n";

  await handleSuccessfulRequest(
    AnalysisUpdateContentParams({
      testFile.path: AddContentOverlay(content),
    }).toRequest('1', clientUriConverter: server.uriConverter),
  );
  // Not awaiting is important to trigger this.
  unawaited(
    handleSuccessfulRequest(
      AnalysisUpdateContentParams({
        testFile.path: AddContentOverlay(updatedContent),
      }).toRequest('3', clientUriConverter: server.uriConverter),
    ),
  );
  var formatResult = await _formatAt(0, 0);

  expect(formatResult.edits, isNotNull);
  expect(formatResult.edits, hasLength(0));
}

By not awaiting the updateContent, the format request can return incorrect results. There should be no format result, but we get:

00:00 +0 -1: FormatTest | test_danny [E]

  Expected: an object with length of <0>
    Actual: [SourceEdit:{"offset":0,"length":14,"replacement":"void main() {}\n"}]
     Which: has length of <1>

@bwilkerson can you confirm my assumption here is correct - there should be no need to await the overlay update, because the server should synchronously process the overlay update enough that no future requests use stale results?

[DanTup]

This was broken by 3f6ff63774bc3b9eaf9f31c2eb031c000a177087 which changed from reading the content directly from the resource provider (and later the resolved unit for the version) to using driver.parseFileSync to only used a parsed result.

If I change the format handler to do this:

// Original code, before we needed to parse to get version.
var resourceProviderContent = server.resourceProvider.getFile(file).readAsStringSync();

// New code, to get parsed version.
var driver = server.getAnalysisDriver(file);
var unit = driver?.parseFileSync(file);

var parsedContent = unit is ParsedUnitResult ? unit.content : null;

print('Content of file (resourceProvider) is ${jsonEncode(resourceProviderContent)}');
print('Content of file (parsed) is ${jsonEncode(parsedContent)}');

Then the output looks like:

Content of file (resourceProvider) is "void main() {\n  print('hello world');\n}\n"
Content of file (parsed) is "void main() {}"

@bwilkerson @scheglov it's not clear to me if this was an unsound change, or if this is a bug. If the overlay resource provider has been updated and driver.changeFile/driver.addFile called, is it expected that parseFileSync could return the previous content?

[DanTup]

I remembered about applyPendingFileChanges, however it is async and I'm not sure if introducing an await here is a good idea (I feel like it could make things inconsistent in other ways).

[bwilkerson]

It sounds like a bug to me. In order to be consistent, the file states need to be the source of truth. It sounds like we have code that's ignoring the file states and going directly to the resource manager to get the content of a file. That's just as much a bug as using dart.io to bypass the resource manager and then having problems because the file system doesn't know about the overlays.

[DanTup]

It sounds like we have code that's ignoring the file states and going directly to the resource manager to get the content of a file.

That's what the old version of this code did, and the resource provider was up-to-date. However with the change above, we instead ask the driver for the parsed unit, and we get a stale version.

I think I have a reasonable fix though, will open a CL shortly.

[DanTup]

My change breaks another test that seems to expect this behaviour - I've pushed https://dart-review.googlesource.com/c/sdk/+/396003 with some comments for your feedback.