Two `ShowCombinator`s and two `HideCombinator`s should have error

[FMorschel]

If you have the following code:

import 'dart:core' show List show int hide String;   // Notice the double `show` but no analyzer error

void foo(List<int> list, String str) {}

Not List, int or String (of course) is correctly analysed showing errors like non_type_as_type_argument.

But if we remove the second show and add a , it works as expected.

I believe we should have an error message about writing down two shows or two hides if they make everything else stop working.

[dart-github-bot]

Summary: The analyzer fails to detect an error when two show combinators are used in a single import statement, leading to unexpected behavior where no types are correctly analyzed. The issue suggests adding an error message to flag this syntax error.

[lrhn]

You never need more than one show or one hide combinator to achieve any hiding-goal. I'd warn about any time you have more than one.

Here the second show, show int, should definitely warn, since it's showing a name that isn't available to be shown (because it was hidden by the prior show). Similarly hide String is hiding a name that isn't available after either of the first two shows.

[FMorschel]

Somewhat related:

• https://github.com/dart-lang/sdk/issues/59907
• https://github.com/dart-lang/sdk/issues/59722

[FMorschel]

@bwilkerson I'll add the tests you asked for at https://dart-review.googlesource.com/c/sdk/+/404525 but just so you have this in mind please take a look at the first comment here from @lrhn.

[bwilkerson]

I completely agree with Lasse's comment. But unless we're going to make it a compilation error for users to have more than one combinator, I think we need to support (as in, don't break) users that don't follow what we consider to be reasonable best practices.

[FMorschel]

Here is the CL for this https://dart-review.googlesource.com/c/sdk/+/413520

[FMorschel]

While working on the description for the new diagnostic, I came up with this example:

import 'dart:math' show Random, max, min show min;

var x = min(0, 1);

But the above triggers UNUSED_SHOWN_NAME for both Random and max. I'm unsure if they should trigger now that we will have the new warning and they can't ever be used, right?

I was thinking of the original example:

import 'dart:core' show List show int hide String;   // Notice the multiple combinators but no analyzer error

void foo(List<int> list, String str) {}

The important take here is that there is no analyzer error on the import line but no type can be resolved. I think unresolved_foo errors should probably show up for that case too, right? Since the second show is not doing anything at all and hide even more so.

---

So I guess to be more explicit, after this CL lands, I'd say:

• We should add a quick-fix/assist that would merge all combinators into either show or hide For this, I'd say we should show as an assist the other option every time but prefer show as a fix if there is a show combinator and prefer hide if there are only hide combinators.
• We should probably think about not showing UNUSED_SHOWN_NAME since that name can't ever be used and we'd have the new error to compensate for it
• We should probably update UNRESOLVED_FOO errors for triggering on the combinators after the first for consistency's sake (now I can see this more clearly, this is what I didn't understand in the original example and made me open this issue)

[bwilkerson]

We should add a quick-fix ...

Having a fix (or two) sounds reasonable. I also prefer using show, but I'm not sure we want to restrict the user's choice by not offering to help them use hide. Also, there's no reason for this to be an assist because it won't apply unless the warning is being produced (unless the user has disabled the warning, in which case they're unlikely to want the assist).

We should probably think about not showing UNUSED_SHOWN_NAME ...

Agreed. Users don't need two warnings for the same problem. Should we continue to produce it if the new warning is disabled? I'm undecided at this point.

We should probably update UNRESOLVED_FOO ...

If it's a compilation error then we're required to produce it. If it's a warning, then you'll need to be more specific about which error in order for me to have an opinion.

[FMorschel]

We should add a quick-fix ...

[T]here's no reason for this to be an assist

Alright then. I was only thinking of giving them different "priorities" depending on the code the user has but without them being assists, I don't think there is a way for us to do that. Not a problem, it would just be a better UX I think. Say:

import 'dart:math' show Random, max, min hide min;
import 'dart:math' hide Random, max, min hide min;

The first import would prefer the show option and the second would prefer hide. But again, not a real problem pressing one arrow key to get where you want.

We should probably think about not showing UNUSED_SHOWN_NAME

Should we continue to produce it if the new warning is disabled?

I know it allows the users to know they have not used names but the names can't be used. So I'm not sure either. I always feel like it is telling me to use that name rather then maybe removing it from the list.

We should probably update UNRESOLVED_FOO

If it's a compilation error then we're required to produce it. If it's a warning, then you'll need to be more specific about which error in order for me to have an opinion.

Take this as an example:

import 'dart:math' show max show min;

We have no min available in the second show. I'd say we should at least warn the user about it. There will be a compilation error if the user tries to use it because it will be unresolved. But it is currently not visible to the user this name can't be accessed there.

[bwilkerson]

I was only thinking of giving them different "priorities" ...

Makes sense. DartFixKind takes a priority, so that should be doable.

I always feel like it is telling me to use that name rather then maybe removing it from the list.

We can tailor the correction message to the situation if we decide to continue to produce the warning.

We have no min available in the second show.

True, but the language doesn't specify this to be an error, so there should be no diagnostic specific to it. The proposed warning would already flag this case. It sounds like the cases are covered. Is there a case where neither the proposed warning nor an existing diagnostic would be produced?

[FMorschel]

Is there a case where neither the proposed warning nor an existing diagnostic would be produced?

I don't think so, no. We should be fully covered with the new warning.