Fix prefix hover and go to definition for parts with imports

[FMorschel]

Consider:

main.dart:

import 'dart:async' as alias;

part 'part.dart';

part.dart

part of 'main.dart';

import 'dart:math' as alias;

void foo(int a, int b) => alias.max(a, b);
alias.FutureOr bar() {}

I was under the impression that when creating alias inside part, it would override the main definition for it. But I might be mistaken.

If this is intended, then the "Go to definition" and hover for the import prefix should take this into consideration.

[eernstg]

A part can't currently have imports, so you may be assuming that the feature parts with imports is available.

That document has some rather detailed discussions about this particular topic. There was some discussion here, too: dart-lang/language#4082.

[FMorschel]

From the specification you mentioned:

That is: The combined import scope of a Dart file is a chain of the combined import scopes of the file and its parent files, each step adding two scopes: The (unnamed) import scope of the unprefixed imports and the prefix scope with prefixed imports, each shadowing names further up in the chain.

I got under the impression from the above quote that in part, since I declared a new import with alias prefix, it would shadow the namespace with the same prefix from the parent file. Did I understand this correctly?

[eernstg]

I think the example that I unfolded in dart-lang/language#4082 still uses the rules as specified (just double checked, but it is rather complex, so caveat emptor).

This means that the prefix scopes are chained such that you don't shadow an entire prefix, you shadow individual names in there. For example, 'part.dart' can use foo, bar, v3 (because they are declared in the library+parts at the top level), and it can use v1 (imported with no prefix from 'l2.dart') and v2 (via the prefix scope, import scope, main prefix scope, main import scope, from 'l1.dart'), p.p1 (from 'p1b.dart'), p.p2 (from 'p1a.dart'), p.p3 (from 'p1b.dart'), p.p4 (from 'p1c.dart').

In your example, alias.max and alias.FutureOr would both succeed, but if 'dart:math' and 'dart:async' have any exported names in common then 'dart:math' wins.

[FMorschel]

I see. Thanks for the explanation!

In that case, I'll keep this open (and rename) to make sure prefix hover and "Go to Definition" work as intended.

Both only work inside the current file (if defined there), but should show all results from the current file up the chain.

[FMorschel]

Can someone also update the tags? Thanks!

---

I was testing it a bit just now, and I have one more question, but for the devexp team (@bwilkerson):

main.dart

import 'dart:math' as math;

part 'part.dart';

part.dart

part of 'main.dart';

int math = 0;

This currently triggers prefix_collides_with_top_level_member at the import prefix (not the file I would have open for editing). Which is different then if I create two int math = 0; on both files, that would trigger a duplicate_definition on the second (part) file.

I'm not sure we should trigger prefix_collides_with_top_level_member or if we do, probably should think about moving it to the new math declaration. Maybe trigger on both places if the user tried to rename the import to that name? I'm not sure. If you agree on changing the current behaviour we can open a new issue to track that.

[bwilkerson]

The compilation error prefix_collides_with_top_level_member is valid in the pre-enhanced-parts world. The specification reads:

It is a compile-time error if id is an import prefix, and the current library declares a top-level member with basename id.

I haven't looked closely enough at the feature spec to know whether this error is still enforced when the enhanced-parts feature is enabled. If the feature spec removes this error then we ought to not produce it when the experiment is enabled, but should still produce it when the experiment is not enabled. If there's a bug when the feature is enabled, then we should add a shared test for this case and fix the bug (whether it's just in the analyzer or in both the analyzer and the front end). If we add a shared test, then we don't need an issue because we'll have a failing test to remind us of the work to be done.

As for where the diagnostic is reported, the fix is to rename either the prefix or the top-level declaration, but I don't think we can say that either choice is the most likely or most common choice. Therefore, the choice of where to put the diagnostic is kind of arbitrary. (Though someone might have decided that renaming the prefix is more likely given that doing so is generally more localized that renaming a top-level declaration.) But I agree that it seems more reasonable for it to be reported at the location of the top-level declaration. Whether it's worth changing the current choice is questionable. I doubt that many users run into this diagnostic, and those that do probably aren't too confused by it.

It would be interesting to see where the CFE reports the error. It would be good if all of our tools were consistent.

[FMorschel]

After some thought about your answer, I figured out what my problem was with the errors above and opened #60668.

---

About fixing the title of this issue, the problem is that the fragments provided only span the current unit. I've not really done much under the analyzer package, so I may need some guidance on this.

Can someone tag this correctly now? Remove the "question" and add whatever tag this needs for the fix on the issue title? If you prefer, I can open a new one, but I don't think it is necessary. Thanks anyway!

[bwilkerson]

... the problem is that the fragments provided only span the current unit.

I don't know what "fragments provided" means. Provided by what?

If we're talking about an API then it's possible the API isn't working yet. The feature isn't complete and we're not actively working on it while the language team decides whether it's currently spec'd correctly given that we're not doing macros. It's also possible that the code asking for the fragments is using the wrong API.

[FMorschel]

Actually, the first one (CL - hovers) uses the PrefixElement.imports, the second one (CL - definition) uses the given ImportPrefixReference.element2.fragments (PrefixElement.fragments).

sdk/pkg/analyzer/lib/src/dart/element/display_string_builder.dart

Lines 261 to 277 in f59751f

	void writePrefixElement2(PrefixElementImpl2 element) {
	var libraryImports = element.imports;
	var displayName = element.displayName;
	if (libraryImports.isEmpty) {
	_write('as ');
	_write(displayName);
	return;
	}
	var first = libraryImports.first;
	_write("import '${first.libraryName}' as $displayName;");
	if (libraryImports.length == 1) {
	return;
	}
	for (var libraryImport in libraryImports.sublist(1)) {
	_write("\nimport '${libraryImport.libraryName}' as $displayName;");
	}
	}

sdk/pkg/analyzer_plugin/lib/src/utilities/navigation/navigation_dart.dart

Lines 442 to 448 in f59751f

	void visitImportPrefixReference(ImportPrefixReference node) {
	var element = node.element2;
	if (element == null) return;
	for (var fragment in element.fragments) {
	computer._addRegionForFragment(node.name, fragment);
	}
	}

If we're talking about an API then it's possible the API isn't working yet. The feature isn't complete and we're not actively working on it while the language team decides whether it's currently spec'd correctly given that we're not doing macros. It's also possible that the code asking for the fragments is using the wrong API.

It sure could be using the API the wrong way. But I take it that it is probably because it is not finished in this case. Either way, I'll leave this open until that is defined then. After that, I'll see if this still needs any work.