`using` directives inside namespace blocks should be recognized as redundant with ImplicitUsings: enable

[AArnott]

Version Used: VS 17.1 (31910.343.main)

Steps to Reproduce:

1. Create a C# 10 console application with <ImplicitUsings>enable</ImplicitUsings> in the project file.
2. Populate the Program.cs file with this:

   using System;
   
   namespace A
   {
       using System;
   
       public class C
       {
           static void Main()
           {
               String s = "Hi";
           }
       }
   }
   

Expected Behavior:

Both using System directives should be faded and candidates for removal. In fact removing both does not produce a compile error because of the implicit using.

Actual Behavior:

Only the directive outside the namespace is seen as redundant:

This impacts commands like Run Code Cleanup on the solution. In a solution where the coding style is to put usings inside the namespace, after enabling implicit usings and running Code Cleanup, the only usings removed are those in files with no namespace at all.

This also repros with the newer namespace statement syntax:

[CyrusNajmabadi]

This would be hard to do. The inner using is not redundant here as it is used by the 'String' identifier.

Determining if it is safe to remove because there is an outer using (global or otherwise) is non-trivial as it can change code meaning to get rid of it.

[AArnott]

Thanks for looking. I put the String use there deliberately, but both using directives are redundant because remember the project is configured with implicit usings. In the repro I can remove both and the code still compiles.

The problem repros if you remove the top using and leave only the inner one. I merely added the outer using to demonstrate that C# recognizes the outer one as redundant with the implicit using but not the inner one.

[AArnott]

I suspect the inner using is a harder redundancy to detect because it can be bound to a non-global namespace. But the compiler certainly knows which namespace it ultimately binds to and can recognize it as redundant when it binds to the global one, as happens in the repro case.

[CyrusNajmabadi]

Thanks for looking. I put the String use there deliberately, but both using directives are redundant because remember the project is configured with implicit usings. In the repro I can remove both and the code still compiles.

Yes, in this repro that may be the case. But consider the case of:

using System;

namespace A
{
    struct String { }
    namespace B
    {
        using System;

        public class C
        {
            static void Main()
            {
                String s = "Hi";
            }
        }
    }
}

In this case, it is not safe to remove the inner using System; If we do, then 'String' will now bind to A.String. This is a core problem in that there is a distinction between "this namespace is unused, and is therefor safe to remove" vs "this namespace is used, but code would mean the same thing if it were removed".

In your example, the latter is true, but it's certainly not universally true. And, it's very complex to determine this. You effectively have rebind everything to see if it means something else. Recall as well that usings bring in namespace, types and extensions. How do you ensure that post-removal that every single piece of code means the same thing.

(As a bonus 'hardness' consider how extension methods may be used without being directly referenced. For example, an extension GetEnumerator() used in a foreach loop) :(

[AArnott]

Well, that's even worse than I supposed, but I figured complexity had something to do with what was likely an intentional scoping decision rather than an oversight in this case.

[jmarolf]

If we were to tackle this issue I definitely would want the compiler team to be responsible for expanding the set of things that they do to report and "unnecessary using" diagnostic.

The other odd case that we care about but is super un-intuitive to most folks is that a using statement is considered "Used" if types from that namespace participated in overload resolution. We could potentially do more here and mark it unused if, after re-running overload resolution for everything in the document, we found that the exact same methods were called. For performance reasons the compiler does not do this today and just bails out saying "this namespace was used in overload resolution assume it is used." This has seemed a reasonable stop-gap in the performance-vs-correctness spectrum. But if we decided to try and re-bind everything in order to examine nested usings I propose that we also capture the overload resolution results to fix up this small area that has surprised a few developers.

[RReverser]

Note that this issue is particularly annoying when using the new file-scoped namespaces. When using those, (at least to me) it feels more natural to put namespace ... at the very top of the file like this:

namespace A;

using System;

public class C
{
    static void Main()
    {
        String s = "Hi";
    }
}

But then those usings are considered internal to the namespace, which can be not obvious from a quick glance, and they don't trigger the "unused using" rule.

[CyrusNajmabadi]

In this case, the using is used. So removing it would change the binding of the code. This is true regardless of if this is a file scoped namespace or not.

[RReverser]

@CyrusNajmabadi No, this is just file-scoped version of the original example in the issue description (minus the first using System;).

With implicit usings - which are the crux of the reported issue - this using System; can still be safely removed and should be marked as such.

[CyrusNajmabadi]

No, this is just file-scoped version of the original example

Right. There's no difference between the two as file-scoped and normal namespaces have the exact same semantics around usings (either inside of hte namespace or outside of htem). Source: i designed and implemented file scoped namespaces :)

With implicit usings - which are the crux of the reported issue - this using System; can still be safely removed and should be marked as such.

It can be safely removed. but that's not what the feature is. The feature isn't "remove save to remove usings". The feature is "remove unused usings". We have no analysis to determine which usings are safe to remove (as it would require rebinding the entire compilation unit and determining that absolutely no meaning changed). We do have analysis to determine which usings are unused, as we can simply mark them during binding. As such, that's what the feature actually delivers. :)

[RReverser]

Yes, I get it from the explanation above, just wanted to add another motivational example to the thread as I believe file-scoped namespaces make this issue even more annoying.

as it would require rebinding the entire compilation unit and determining that absolutely no meaning changed

I don't think this is necessarily true though? Roslyn analysers usually have no problem resolving symbols through the semantic model - this kind of scenarios is exactly where it shines and avoids said recompilation. In theory you could resolve all the available usings to namespace symbols and see which ones are duplicates via symbol comparer.

[CyrusNajmabadi]

I don't think this is necessarily true though?

It is true. The usings have different meaning in the code as they are looked through at different times. The usings within the namespace are looked through prior to the usings outside of the namespace. And the usings outside of the namespace are looked through after looking through all the namespaces the code is currently in.

This can trivially change semantics for any particular identifier.

[CyrusNajmabadi]

@RReverser there's an example higher up in this thread demonstrating the issue. There are even thornier problems that arise.

[RReverser]

Of course, but handling the semantic differences is exactly what resolving the usings via semantic model is for.

It's not like I'm suggesting to just go through the AST looking at the raw using nodes - reimplementing handling for those would be indeed far from trivial - but the resolved namespaces should already take current namespace & other usings into account.

[CyrusNajmabadi]

but the resolved namespaces should already take current namespace & other usings into account.

I don't know this means.

Fundamentally, if we remove a using that is actually being used, we have to ensure that the meaning of the code after is the same as the meaning of the code before. This is extremely difficult to do (to the point of being not practical).

[RReverser]

It's very difficult to do in general case, sure, that's why I'm suggesting to only remove explicitly duplicate usings.

This will cover the most common case of having explicit usings that duplicate those in GlobalUsings.g.cs - which includes both mine and the example that started this issue.

[CyrusNajmabadi]

Again, that can and will change code meaning.

[RReverser]

I don't think this particular case is hard to check for, but at this point it feels like it might be more productive for me to just try and build it as a separate Roslyn analyser first rather than continue the discussion :)

[CyrusNajmabadi]

I don't think this particular case is hard to check for

Can you be explicit about what case you are talking about?

Here's the general issue. You mentioned you wanted using System; removed in teh following code:

namespace A;

using System;

public class C
{
    static void Main()
    {
        String s = "Hi";
    }
}

The issue here is that removing that inner using System; means every piece of code within that namespace may change meaning. Determining that it didn't is extremely challenging. As an example, removing a using may take an extension method out of scope. And if that extension method is out of scope, a lambda method may bind differently (even if it didn't use that extension). Determining this is non-trivial. And this is just one sort of problem.

Using-statements affect all name lookup. That name lookup takes an entirely different path when you remove it. So that means any symbol that could be affected by name lookup (effectively, literally every name) now has to be rechecked, and perfectly determined to have the same meaning as before.

[CyrusNajmabadi]

but at this point it feels like it might be more productive for me to just try and build it as a separate Roslyn analyser first

That seems like a great idea. You can take the risks on the code change meaning you're ok with :)

[RReverser]

Can you be explicit about what case you are talking about?

The one you quoted, yes.

The issue here is that removing that inner using System; means every piece of code within that namespace may change meaning. Determining that it didn't is extremely challenging. As an example, removing a using may take an extension method out of scope.

Yes, but if there's already an outer using System; - whether in the same file or in the global usings - that the semantic model says resolves to the same namespace and not e.g. A.System, then the inner one should be safe to remove, because we know that exactly same using is already in scope together with any relevant types and extension methods.

You can take the risks on the code change meaning you're ok with :)

Indeed, and still a bit easier than removing those usings by grepping all files for known using ... patterns each time 😅 [pushes it to the back of side projects]

[RReverser]

because we know that exactly same using is already in scope together with any relevant types and extension methods.

Hm although I think I'm starting to understand which edge case you're talking about regarding lookup order. That might be trickier... but still interesting enough to try.

[CyrusNajmabadi]

Yes, but if there's already an outer using System; - whether in the same file or in the global usings - that the semantic model says resolves to the same namespace

That's not relevant. What's relevant is hte name-lookup of the names within the namespace. Without the inner using those names can now bind to different symbols.

[CyrusNajmabadi]

then the inner one should be safe to remove, because we know that exactly same using is already in scope together with any relevant types and extension methods.

I don't think you're understanding the example i gave:

using System;

namespace A
{
    struct String { }
    namespace B
    {
        using System;

        public class C
        {
            static void Main(String s)
            {
            }
        }
    }
}

Here, both the inner and outer using System; resolve to the exact same namespace symbol. But removing the inner using will change the meaning of the code.

Again, this is not speculative concern. Many projects have code exactly like this.

[CyrusNajmabadi]

Here's a trivial example with extension methods:

using System;

namespace MyNamespace
{
  using X;
  using Y;

  class B
  {
    static void Main()
    {
        Bar(x => x.Goo(), null); // Prints 1
    }

    static void Bar(Action<string> x, object y)
    {
        Console.WriteLine(1);
    }

    static void Bar(Action<int> x, string y)
    {
        Console.WriteLine(2);
    }
  }
}

namespace X
{
    public static class A
    {
        public static void Goo(this int x)
        {
        }

        public static void Goo(this string x)
        {
        }
    }
}

namespace Y
{
    public static class B
    {
        public static void Goo(this int x)
        {
        }
    }
}

If you remove either of the inner using X/Y; (Because you see they came in from a global namespace), this will change the meaning of the code due to extension method and lambda inference. This is true regardless of whether the outer global using binds to the same namespace as the inner one.

[RReverser]

Yeah those examples make sense, thanks. Definitely makes it trickier.

[RReverser]

Kind of makes me wish file-scoped namespaces were designed to be order-agnostic (always encompassing the whole file regardless of whether they're placed before or after usings) heh.

[CyrusNajmabadi]

Kind of makes me wish file-scoped namespaces were designed to be order-agnostic

We explciitly rejected this. Because it would mean people couldn't move safely to them. Today, you can safely move an existing normal namespace ot a file-scoped-one because we explicitly designed the semantics to be identical. That allowed the feature to be both easy to implement, and allowed for the ecosystem to move safely over without concerns. :)

Note: having different semantics for inside/outside is a goal. As otherwise you could be screwed (depending on which interpretation we took). There's a reason the language allows both, and people use both in codebases. It's because you need those different semantics in different cases :)

[CyrusNajmabadi]

CLosing out as by design. These usings are not redundant. They are the usings that the language and compiler say the user is using. Detection that a using can be removed and that no code meaning will change is out of scope for the IDE. If compiler believes they could supply an API to solve htis, we would use it. But it would be on them to make that possible.