New assist to add/edit hide at import for ambiguous import

[FMorschel]

This issue is to track https://dart-review.googlesource.com/c/sdk/+/386020 and #56762.

This change would add a new quick-fix option to ambiguous_import to add a hide combinator (or edit the current one to edit it). So cases like:

a.dart

class A {}

b.dart

class A {}

class B {}

c.dart

import 'a.dart';
import 'b.dart' hide B;

void foo(A a) {}      // <-- ambiguous_import

Would trigger this fix options to add hide to 'a.dart' or to edit the hide at 'b.dart'.

---

CC: @bwilkerson

[dart-github-bot]

Summary: This issue proposes a new quick-fix for ambiguous imports. It suggests adding a "hide" combinator to the import statement to resolve the ambiguity, either by adding a new "hide" or editing an existing one.

[bwilkerson]

It's possible for an ambiguous name to be imported through more than 2 imports. For example, given three libraries, L1, L2, and L3, that all define a class named C, a fourth library L4 could import the first three. Hiding the name in one import will only partially solve the problem (by reducing the number of ambiguous names).

Probably more common is that a single definition of the ambiguous name might be imported from multiple libraries. For example, given the libraries, L1 and L2, that both define a class named C, a library L3 that exports L1, and a library L4 that imports the first three, adding a hide to the import of either L1 or L3 won't actually improve the situation at all because the name will still be imported via the other.

That raises a question about the DX.

Rather than require the user to select one fix for every import that should have a hide added to it (potentially a multi-step process), I wonder whether it might be better to have the fix be framed in terms of choosing the one import from which the name shouldn't be hidden, and just hide the name in all the other locations.

Or maybe the set of imports from which it shouldn't be hidden, such as in the second example, where there's no point in hiding it from either L1 or L3 if the declaration in L1 is the one the user wants to use.

Or maybe what's important isn't the import(s) to be unchanged, but the declaration (element in API terms) to be used. Although identifying the declaration might be tricky because the defining library might be an internal library that the user wouldn't recognize.

Of maybe those scenarios are too complex and we should just not offer the fix in those cases.

Curious to hear your thoughts.

[FMorschel]

Rather than require the user to select one fix for every import that should have a hide added to it (potentially a multi-step process), I wonder whether it might be better to have the fix be framed in terms of choosing the one import from which the name shouldn't be hidden, and just hide the name in all the other locations.

Yes, thank you for the suggestion. I hadn't thought of the multi-step process.

However, I believe this would probably be hard to happen unless multiple libraries which export (say L2 exports L1) each other (or something similar) suddenly received a new declaration (say you declare E in L1) with the same name as a declaration in another library (L3) set somewhere else. In this case, if some library (L4) had imported all of the previous as you mentioned, this could happen, otherwise, I think it would be very unlikely that someone had this kind of problem.

---

Or maybe what's important isn't the import(s) to be unchanged, but the declaration (element in API terms) to be used. Although identifying the declaration might be tricky because the defining library might be an internal library that the user wouldn't recognize.

Yes, I do believe that this is ideal. The error message for ambiguous_import already shows all declarations for every element that is coinciding so maybe that would be a possible solution but probably not necessarily easy for the user since, as you mentioned, some of the time the person can be unfamiliar with the declaration itself but they would of course be familiar with the import that they want to keep using it from.

If we had something like the "Docs side panel" (not sure what to call that) that we have for auto-complete options, in assists, we could show the element in the assist text and make sure our user was aware of which imports was it coming from. But since that is not the case I don't believe this would be a good option.

---

Or maybe the set of imports from which it shouldn't be hidden, such as in the second example, where there's no point in hiding it from either L1 or L3 if the declaration in L1 is the one the user wants to use.

Although I see your very valid point here, I don't believe it would be easy for everyone to understand why more than one import was left without the hide. And it would probably make no difference for them if the "other imports" (the ones that export the same element) had the hide added since it wouldn't change their code in any way.

---

While I prefer the "hide every other import" option, I was wondering what should we do in cases where one of the other imports is showing the specific element in question. Should we not offer support in those cases? Wondering because it might be tricky to tell the user about that unless we change the error message for ambiguous_import and make sure it also informs the user about the show in the current imports or something else along these lines.

[FMorschel]

I was wondering what should we do in cases where one of the other imports is showing the specific element in question.

Maybe we could change the text in these cases for the fix adding something like (remove 'show') at the end so that the user knows about it?

[srawlins]

Great points Brian about how maybe we should offer choices to the user like "Use the one from L1.dart".

I worry about the length of a single quick fix text if it includes 2 or more URIs. Like if package:foo/thingy/foo.dart, package:foo/thingy/bar.dart, and package:foo/thingy/baz.dart' all contribute to the ambiguous import, then offering to hide would result in a long quick fix text, like "Hide X from the 'package:foo/thingy/foo.dart' and 'package:foo/thingy/bar.dart' imports." Yikes.

And like @FMorschel says, what if the imports are like:

import 'package:foo/thingy/foo.dart';
import 'package:foo/thingy/bar.dart' show X;
import 'package:foo/thingy/baz.dart';

Then "Hide X from the 'package:foo/thingy/foo.dart' and 'package:foo/thingy/bar.dart' imports." is a little technically not correct, because we would (probably) add a hide combinator for the foo.dart import, and remove the shown X name from the bar.dart import.

So I like the "Use X from 'package:foo/thingy/baz.dart'" message. I don't think we need the "(removing 'show')" text (in some cases you would remove X, in some cases X and a comma, and in some cases show and X so 🤷 ); I don't feel strongly though.

WDYT, @bwilkerson ?

[FMorschel]

I don't think we need the "(removing 'show')" text (in some cases you would remove X, in some cases X and a comma, and in some cases show and X)

Yes, I'm aware. This suggestion was only to make sure the user knows we are removing a show that was there, not necessarily what exactly what is being removed.

This is mainly because today all hides/shows are mostly (if not all the time) explicitly added by the user (if not all, at least the first one is - Dart-Code/Dart-Code#5238 is a good example of future shows thar are not directly user made but the first one still is). So this way the user would be aware that this would potentially break some existing code (we expect all shows to be used because we have a warning - and fix - for when they are not).

I actually wanted something shorter but could not think of a shorter message that would convey the same meaning.

[bwilkerson]

Sorry for the slow response. I was out for a couple of days a couple of weeks ago and I'm still digging my way out from under.

Rather than require the user to select one fix for every import that should have a hide added to it ...

However, I believe this would probably be hard to happen ...

I wouldn't expect it to be very common, but then I would hope that most developers don't re-use names at all and almost never need to worry about resolving conflicts. (Something that isn't true in the analyzer code bases, sadly enough.) But if that were true then this assist would be a low-value proposition.

... I was wondering what should we do in cases where one of the other imports is showing the specific element in question.

Good question. I would take the explicit show to be a strong signal that the user wanted to use that element.

Maybe the better solution is to add a prefix for one of the elements in order to allow both to be referenced.

I worry about the length of a single quick fix text if it includes 2 or more URIs.

I worry about the length of a message that contains a single URI. 😄

So I like the "Use X from 'package:foo/thingy/baz.dart'" message.

The messages don't need to, and I'd go so far as to say shouldn't, spell out every edit that needs to be made. They should be as short as possible while still telling the user what to expect. I'm not sure the "Use X ..." message does that because it doesn't indicate whether it will be done by updating combinators or by adding prefixes, but I do like the brevity.

I'm not sure we can formulate a good message until we know exactly what the fix will do, and I'm not convinced we've gotten to that point yet.

[FMorschel]

After considering your last comments I was considering maybe renaming it to:

Hide others to use X from 'package:foo/thingy/baz.dart'

About the show I think maybe we could add a second assist - tied to the same producer - with a lower priority that would say something like:

Remove show to use X from 'package:foo/thingy/baz.dart'

It would replace the first assist whenever any of the other imports had a show combinator with (obviously) X in the list.

WDYT?

[FMorschel]

To note, I found #56255 which I think this is a dupe of.

[bwilkerson]

I think naming is hard.

I still don't like the fact that they describe the edits to be made ("Hide others" and "Remove show").

What if the current state of the imports is such that we'd need to add hides to some imports and remove shows from others in order to make the reference unambiguous?

[FMorschel]

I still don't like the fact that they describe the edits to be made

I was thinking about it too but then I figured this would be better for when we create a fix that triggers an import rename it could say that there as well before the "to use" (like "Rename import to use...").

What if the current state of the imports is such that we'd need to add hides to some imports and remove shows from others in order to make the reference unambiguous?

For this case - I know this is not ideal but - I'd choose the "remove show" since that would have a bigger impact on the user's code meaning. It would also be lower on the list, as suggested above, so less likely that the user will go for it.

[bwilkerson]

... I figured this would be better for when we create a fix that triggers an import rename ...

What do you mean by "an import rename"? If you mean a rename refactoring on the import prefix, then I don't believe that there's any way to do that. The fixes are currently required to produce the edits before the option is presented to the user, and that precludes them from prompting for more information. But maybe you mean something different?

What if the current state of the imports is such that we'd need to add hides to some imports and remove shows from others in order to make the reference unambiguous?

On further reflection, I'm not sure it's possible to be in a state where both operations would need to be performed.

I still don't like the fact that they describe the edits to be made

I still don't like it, but I'm not going to refuse to support the feature just because we can't find an ideal name for it.

[FMorschel]

What do you mean by "an import rename"? If you mean a rename refactoring on the import prefix, then I don't believe that there's any way to do that. The fixes are currently required to produce the edits before the option is presented to the user, and that precludes them from prompting for more information.

I was not aware of that. I was talking about this second suggestion (maybe add a specific not-in-use name and trigger a rename? Not a discussion for this issue but anyway):

sdk/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml

Lines 158 to 164 in 253e715

	CompileTimeErrorCode.AMBIGUOUS_IMPORT:
	status: needsFix
	notes: |-
	1. For each imported name, add a fix to hide the name.
	2. For each imported name, add a fix to add a prefix. We wouldn't be able to
	add the prefix everywhere, but could add it wherever the name was already
	unambiguous.

---

On further reflection, I'm not sure it's possible to be in a state where both operations would need to be performed.

On normal day-to-day use I wouldn't expect so. But if you have the following:

lib1.dart / lib2.dart / lib3.dart, all with the same class name:

class A {}

And your code looks something like this:

import 'lib1.dart' show A; // There could be other classes here that would conflict or the user just prefers using 'show'
import 'lib2.dart' hide A; // There could be other classes here that are needed
import 'lib3.dart';   // <-- Just added

A? a;

Now we have the error being triggered and our suggestions would show Remove show for using either lib2.dart or lib3.dart. But it would still need to hide the declaration on lib3.dart if the user chose the option of lib2.dart.

As I said, I would not expect this to be the selected choice since shows are explicitly added and both these options would be under Hide others ... for lib1.dart.

---

I know it is not ideal, but if we ever think of a shorter name that expresses the same meaning we could always change this description and do some kind of public announcement for this kind of thing (not sure if it would be that important because of the same meaning part, but anyway).

If you still dislike the action description we could go back to simply "Use X..." but even though this is shorter I would prefer the longer action descriptions for clarity (mainly for removing shows).

[bwilkerson]

... but could add it wherever the name was already unambiguous.

I might have written that, but I don't like that suggestion. I don't think there would be any places where the name would be ambiguous.

But it would still need to hide the declaration on lib3.dart if the user chose the option of lib2.dart.

I think that's an uncommon enough occurrence that we should just not provide a fix in that case. If we solve the common case we've still improved life for a lot of users.

I know it is not ideal, but if we ever think of a shorter name that expresses the same meaning we could always change this description ...

Exactly. Let's go with the "Hide others" and "Remove show" versions that you suggested earlier.

[FMorschel]

@bwilkerson, I've rebased, migrated to the new element model and refactored considering the reviews from srawlins (he only looked at the correction producers and not the tests, although he said he would look at them today).

Could you take a look at the CL when you have some time, please? Thanks a lot!

[FMorschel]

After that issue, I made a new push that solved the current part files. I'm unsure about the new enhanced parts. I think this is good enough for now and we can open a new issue following this one to make it work for those cases.

---

The problem for them currently, from what I understand:

Say three files a, b and c. Being c a part of b and b being a part of a.
If c has an import for dart:math and used Random and another import for the same name, I would not need to look up the files, right?

That is what I need to work on to fix.

The way I understand the enhanced parts feature, there won't be any case where I would need to edit two different files, like for fixing a problem on c would never need to hide imports from b and a. Right, @bwilkerson?

[FMorschel]

Also to note, this could also maybe be a plausible fix for ambiguous_extension_member_access. I know the message tells the user to use an extension override to fix it, but that may not work when both enums have the same name anyway so I think we could consider it.

[FMorschel]

@srawlins and @bwilkerson can I get new reviews and an answer here so we can close this? Thanks for all the help!

[FMorschel]

I've added a failing test for multi-level part files. Copied from the "Go to imports". Studied it a bit and I think I get it now.

I don't think I did it right or there is still something missing in the implementation for it. I was expecting an error there but it found none.

[FMorschel]

I found out that #58326 was the reason the test failed. Edited the test.

[FMorschel]

Hi again @bwilkerson and @srawlins. I've merged main into this branch again. Everything is in order. Can I get new reviews here? Thanks a lot!

[FMorschel]

@scheglov noticed that we have a small case to worry about here. How should we handle cases like:

lib1.dart

var foo = 0;
var bar = 0;
var baz = 0;

lib2.dart

var foo = 0;

main.dart

import 'lib1.dart' hide bar;
import 'lib1.dart' hide baz;
import 'lib2.dart';

void f() {
  print(bar);
  print(baz);
  print(foo);
}

In the sense that we have two imports with the same URL.

1. Should avoid showing the fix here
2. Should we fix both with a single-fix option
3. If we decide to have multiple fixes showing for all imports, how should we differentiate them?
4. If we have both show should it be the same
5. What if we have multiple (three or more) show and hide options?

Up until my last patchset where uris were handled with a set we were handling this if they matched with a single option but maybe we should think a bit more on this.

WDYT @bwilkerson?

[bwilkerson]

Say three files a, b and c. Being c a part of b and b being a part of a.
If c has an import for dart:math and used Random and another import for the same name, I would not need to look up the files, right?

Correct. The imports from a parent part are only used if a name couldn't be resolved using the imports in the child part.

Also to note, this could also maybe be a plausible fix for ambiguous_extension_member_access. I know the message tells the user to use an extension override to fix it, but that may not work when both enums have the same name anyway so I think we could consider it.

This is another case that I think is likely to be very uncommon, and hence probably not worth fixing at this time.

In the sense that we have two imports with the same URL.

I think we should take option (1) and not display the fix. It's perfectly reasonable to ship a fix that handles the 90% case and doesn't appear for the remaining 10%, and doing so means that we get a solution for the majority of users that much sooner.

(I will point out that having two or more imports of the same URI, all with either hide or no combinator and all having the same prefix, means that the only thing hidden is the intersection of the hide lists. In the example above that means that there are no hidden names, so all three names will be imported. More useful than a fix would be a warning that these hide clauses are not really hiding anything.)