includedSuggestionSets contains duplicates

[DanTup]

I noticed I was seeing dupes in the completion list, and it seems like the server can include suggestion sets multiple times in a completion response.

The libraries here are all in my local workspace alongside the workfile.dart file.

{
	"id": "1",
	"replacementOffset": 11,
	"replacementLength": 11,
	"results": [/*...*/],
	"isLast": true,
	"libraryFile": "/Users/dantup/Desktop/import_test_completion/lib/workfile.dart",
	"includedSuggestionSets": [
		{
			"id": 169, // <----------
			"relevance": 5
		},
		{
			"id": 168, // <----------
			"relevance": 5
		},
		{
			"id": 167, // <----------
			"relevance": 5
		},
		{
			"id": 169, // <----------
			"relevance": 5
		},
		{
			"id": 168, // <----------
			"relevance": 5
		},
		{
			"id": 167, // <----------
			"relevance": 5
		},
		{
			"id": 91,
			"relevance": 3
		},/*...*/
	],
	"includedElementKinds": [
		"CONSTRUCTOR",
		"CLASS",
		"CLASS_TYPE_ALIAS",
		"ENUM",
		"FUNCTION_TYPE_ALIAS",
		"MIXIN",
		"ENUM_CONSTANT",
		"FUNCTION",
		"TOP_LEVEL_VARIABLE"
	],
	"includedSuggestionRelevanceTags": []
}

Source project is like:

lib/proxy1.dart

export 'real_def.dart';

lib/proxy2.dart

export 'real_def.dart';

lib/real_def.dart

class DannyRealClass {}

lib/workfile.dart

main() {
  DannyRealCl // invoke completion here
}

pubspec.yaml

name: danny

I use using the latest nightly build of the Dart SDK.

@scheglov I can add code to skip these, but it seems like it might be better done in the server so that each client doesn't need to know to do it?

[scheglov]

I was not able to reproduce this.

[DanTup]

Hmm, I tested it a few times and it seems to be intermittent on the same project (I can restart the analyzer, then sometimes I'll see it and sometimes I won't). I'll do some more digging tomorrow to see if I can understand it better.

[DanTup]

Ok, I'm failing to understand what's going on here, but I seem to be able to reliably reproduce it in one folder, and reliably not reproduce it in another folder. The folders are identical (just copy/pasted) except for their paths.

Could this be related to something in ~/.dartServer? Is anything cached by path?

Here are the logs:

analyzer_with_dupes.txt
analyzer_without_dupes.txt

The logs have some requests in different orders, but I don't think that should matter. One has duplicates in includedSuggestionSets and the other does not.

The exact repo I'm using (though I commented out the SDK path in .vscode/settings.json) is here:

https://github.com/DanTup/dart_repro_37211

[DanTup]

Saw this again in Flutter. I updated to master (91213e2ed) and then opened lib/src/emulator.dart and when I typed PlatformType I get it four times:

Next version of VS Code filters them out, however LSP probably doesn't, so worth getting to the bottom of it (I'll try and do some more digging later).

[DanTup]

Seems to be related to this code here in available_declarations.dart / DeclarationsContext / getLibraries... Seems like _contextPathsList might contain dupes (though I haven't tracked down where from yet):

[DanTup]

Ok, it seems like the dupes already exist in _analysisContext.contextRoot.analyzedFiles():

@scheglov is it expected that there may be dupes here? If so, then I think the bug is in _scheduleContextFiles which adds all the dupes to _contextPathList. If not, the bug is somewhere earlier.

I thought this might be confused because I'm working in the Flutter repo (so I have types in the loaded SDK, as well as in my current project), however my original repro above was with my own local types, so I don't think that's related.

[scheglov]

No, _analysisContext.contextRoot.analyzedFiles() should not have duplicates. It should be a subset of files in a directory. So, it might exclude some files, but should not add duplicates. @bwilkerson to confirm and maybe explain why it might happen.

[bwilkerson]

Correct; analyzedFiles should not return the same file more than once. It walks the directory structure, starting at the root of the context (typically a directory) and skipping any files that have been excluded (either directly or by excluding a parent directory).

The only thing I can think of off the top of my head that would cause it to return the same file more than once is if there is a symbolic link inside the context root. Under the covers we're using directory.listSync(recursive: false) to get the children of a directory. I can't easily check at the moment whether that's the right way to protect against links.

[DanTup]

I can only repro this in Flutter right now (using the changeset above), but here's what I found...

If I hack some logging into ContextRootImpl that prints the folders in _includedFilesInFolder, like this:

It just prints the folder and all of its children when it gets to the lib folder, it outputs this:

1561539496144:Info:enumerating children of      /Users/dantup/Dev/Google/flutter/packages/flutter_tools/lib
1561539496145:Info:     /Users/dantup/Dev/Google/flutter/packages/flutter_tools/lib/runner.dart
1561539496145:Info:     /Users/dantup/Dev/Google/flutter/packages/flutter_tools/lib/executable.dart
1561539496145:Info:     /Users/dantup/Dev/Google/flutter/packages/flutter_tools/lib/src
1561539496145:Info:     /Users/dantup/Dev/Google/flutter/packages/flutter_tools/lib/src

So it seems like Folder.getChildren() has dupes in it. On the context, includedPaths was just /Users/dantup/Dev/Google/flutter/packages/flutter_tools.

The only symlinks in the tree (assuming this command works as I expect) appear to only point further down their own trees:

dantup-macbookpro:flutter dantup$ pwd
/Users/dantup/Dev/Google/flutter
dantup-macbookpro:flutter dantup$ find . -type l -ls
4344687092        0 lrwxr-xr-x    1 dantup           5762                   26 26 Jun 09:22 ./bin/cache/artifacts/engine/darwin-x64/FlutterMacOS.framework/Resources -> Versions/Current/Resources
4344687093        0 lrwxr-xr-x    1 dantup           5762                    1 26 Jun 09:22 ./bin/cache/artifacts/engine/darwin-x64/FlutterMacOS.framework/Versions/Current -> A
4344687094        0 lrwxr-xr-x    1 dantup           5762                   24 26 Jun 09:22 ./bin/cache/artifacts/engine/darwin-x64/FlutterMacOS.framework/Headers -> Versions/Current/Headers
4344687095        0 lrwxr-xr-x    1 dantup           5762                   24 26 Jun 09:22 ./bin/cache/artifacts/engine/darwin-x64/FlutterMacOS.framework/Modules -> Versions/Current/Modules
4344687096        0 lrwxr-xr-x    1 dantup           5762                   29 26 Jun 09:22 ./bin/cache/artifacts/engine/darwin-x64/FlutterMacOS.framework/FlutterMacOS -> Versions/Current/FlutterMacOS
dantup-macbookpro:flutter dantup$ 

But to eliminate them, I deleted the darwin-x64 folder and there was no change.

I'll see if I can attach a debugger and do some more digging later (and if you have any ideas about what it could be in the meantime, do lmk!).

[DanTup]

Ok, I had another look at this, and I found the first place the duplicates show up here:

sdk/pkg/analyzer/lib/file_system/overlay_file_system.dart

Lines 343 to 365 in 7acecda

	List<Resource> getChildren() {
	List<Resource> children;
	try {
	children = _folder
	.getChildren()
	.map((child) => new _OverlayResource._from(_provider, child))
	.toList();
	} on FileSystemException {
	children = [];
	}
	for (String overlayPath in _provider._overlaysInFolder(path)) {
	pathos.Context context = _provider.pathContext;
	if (context.dirname(overlayPath) == path) {
	children.add(_provider.getFile(overlayPath));
	} else {
	String relativePath = context.relative(overlayPath, from: path);
	String folderName = context.split(relativePath)[0];
	children.add(_provider.getFolder(context.join(path, folderName)));
	}
	}
	return children;
	}
	}

For my Flutter example above (where /Users/dantup/Dev/Google/flutter/packages/flutter_tools/lib/src shows up twice), it's being returned in _folder.getChildren on line 346, and then it's also being added into children on line 360.

If I'm understanding the code correctly, this is loading children from the file system and then adding the overlay ones on top? If so, shouldn't there be some logic to handle when things are already in children when adding the overlays?

[DanTup]

@bwilkerson if my understanding above is correct, I think this is a test (added to overlay_file_system_test.dart) that may illustrate it:

@soloTest
test_getChildren_existing_withOverlay() {
  Folder folder = _folder(exists: true);
  Folder child1 = _folder(
      exists: true, path: provider.pathContext.join(folder.path, 'lib'));
  _file(
      exists: true,
      path: provider.pathContext.join(child1.path, 'a.dart'),
      withOverlay: true);
  List<Resource> children = folder.getChildren();
  children.forEach((r) => print(r.path));
  expect(children, hasLength(1));
}

Output looks like this:

00:00 +0: FolderTest | test_getChildren_existing_withOverlay
/foo/bar/lib
/foo/bar/lib

Is the answer to change the list in the code above to use a Map so that the overlays overwrite the file system ones, then return only the unique (by-path) values?

(if this is the problem though, I'm not sure why it doesn't seem to affect more things 🤔)

[bwilkerson]

If I'm understanding the code correctly, this is loading children from the file system and then adding the overlay ones on top? If so, shouldn't there be some logic to handle when things are already in children when adding the overlays?

Yes, the returned list should not contain duplicates.

Is the answer to change the list in the code above to use a Map so that the overlays overwrite the file system ones, then return only the unique (by-path) values?

If by "code above" you mean the implementation of OverlayFileSystem.getChildren(), then yes, a map from path to Resource seems like a reasonable approach for removing the duplication. I don't believe, however, that there's any reason to "overwrite" the resources provided for the files and directories from the base provider. IIRC, the objects we create all look the same whether they represent real files or overlaid files, so we could reduce the number of short-lived objects we create by only creating one resource per path.

[DanTup]

Ok great, I've opened a fix:

https://dart-review.googlesource.com/c/sdk/+/108600/

There was another path that resulted in dupes - multiple descendant overlays (the overlay loop would add their containing folders one-per-file). I think that one might've been a bigger impact because instead of getting just one extra dupe (file + overlay file), you could get folders repeated for every descendant overlay file at each level of the tree it (eg. five overlays down in /a/b/c/d would result in 5x b in the result of getChildren('a'), 5x c in the result of getChildren('b'), etc.).

Both issues should be fixed by the CL above and have tests.