dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.07k stars 4.04k forks source link

EditorBrowsable(EditorBrowsableState.Never) in VS 2015 does not work as well as it does in VS 2013 #4434

Open TyreeJackson opened 9 years ago

TyreeJackson commented 9 years ago

Problem

Consider the following projects and classes:

ProjectA

is not included in the solution

namespace ProjectA
{

    using System;
    using System.ComponentModel;

    public class BaseClass
    {
        [EditorBrowsable(EditorBrowsableState.Never)]
        public new Type GetType() { return base.GetType(); }

        [EditorBrowsable(EditorBrowsableState.Never)]
        public new static bool Equals(object objA, object objB) { return Object.Equals(objA, objB); }

        [EditorBrowsable(EditorBrowsableState.Never)]
        public override bool Equals(object obj)  { return base.Equals(obj); }

        [EditorBrowsable(EditorBrowsableState.Never)]
        public override int GetHashCode() { return base.GetHashCode(); }

        [EditorBrowsable(EditorBrowsableState.Never)]
        protected new object MemberwiseClone() { return base.MemberwiseClone(); }

        [EditorBrowsable(EditorBrowsableState.Never)]
        public new static bool ReferenceEquals(object objA, object objB) { return Object.ReferenceEquals(objA, objB); }

        [EditorBrowsable(EditorBrowsableState.Never)]
        public override string ToString() { return base.ToString(); }

        public static void DoSomeBasicThing() {}

    }

}

namespace ProjectA
{

    public class ClassA : BaseClass
    {

        public static void InitializeClassA() { }

        public void DoSomethingA() {}

    }

}

ProjectB

is included in the solution; has a binary reference to ProjectA

namespace ProjectB
{

    using System.ComponentModel;
    using ProjectA;

    [EditorBrowsable(EditorBrowsableState.Never)]
    public class ClassB : BaseClass, IInterfaceA<ClassB>
    {

        public void DoSomethingB()
        {
            var objectA = new ClassA();
        }

    }

}

ProjectC

is included in the solution; has a binary reference to ProjectA; has a project reference to ProjectB

namespace ProjectC
{

    using ProjectB;

    public class ClassC 
    {

        public void TestEm()
        {
            var objectB  = new ClassB();
        }

    }

}

In Visual Studio 2013 the EditorBrowsable attribute is honored correctly for members of types defined in assemblies that are included using a binary reference. However, in Visual Studio 2015, some of the methods defined on the System.Object [static Equals, static ReferenceEquals, and GetType] appear in the Intellisense menu despite being marked with the EditorBrowsable attribute with the EditorBrowsableState.Never argument in a sub class via new hiding methods.

Here are some screenshots:

Visual Studio 2013 - viewing ProjectA.ClassA's Intellisense menu from ProjectB.ClassB: Visual Studio 2015 - viewing ProjectA.ClassA's Intellisense menu from ProjectB.ClassB:
Visual Studio 2013 - viewing ProjectA.ClassA's Intellisense menu from ProjectB.ClassB Visual Studio 2015 - viewing ProjectA.ClassA's Intellisense menu from ProjectB.ClassB
Visual Studio 2013 - viewing objectA's Intellisense menu from ProjectB.ClassB: Visual Studio 2015 - viewing objectA's Intellisense menu from ProjectB.ClassB:
Visual Studio 2013 - viewing objectA's Intellisense menu from ProjectB.ClassB Visual Studio 2015 - viewing objectA's Intellisense menu from ProjectB.ClassB
Visual Studio 2013 - viewing ProjectB.ClassB's Intellisense menu from ProjectC.ClassC: Visual Studio 2015 - viewing ProjectB.ClassB's Intellisense menu from ProjectC.ClassC:
Visual Studio 2013 - viewing ProjectB.ClassB's Intellisense menu from ProjectC.ClassC Visual Studio 2015 - viewing ProjectB.ClassB's Intellisense menu from ProjectC.ClassC
Visual Studio 2013 - viewing objectB's Intellisense menu from ProjectC.ClassC: Visual Studio 2015 - viewing objectB's Intellisense menu from ProjectC.ClassC:
Visual Studio 2013 - viewing objectB's Intellisense menu from ProjectC.ClassC Visual Studio 2015 - viewing objectB's Intellisense menu from ProjectC.ClassC

Note that this is the additional problem I mentioned in #4358 in the footnote of this comment.

hmahajanHM commented 8 years ago

Is this fixed? I can still see this bug on VS 2015 Professional Update 1 build 14.0.24720.00

jcansdale commented 8 years ago

The NUnit team is suffering from this issue when developing our Assert API. We'd like to hide Equals, to prevent users from accidentally entering the following:

Assert.Equals(a, b);
Assert.That(a, Is.Equals...

When they should have entered:

Assert.AreEqual(a, b);
Assert.That(a, Is.EqualTo(b));

The following appears to work fine in Visual Studio 2013:

        [EditorBrowsable(EditorBrowsableState.Never)]
        new public static bool Equals(object ob1, object ob2)
        {
            throw new NotImplementedException("I'm afraid I can't do that");
        }

        [EditorBrowsable(EditorBrowsableState.Never)]
        new public static bool ReferenceEquals(object ob1, object ob2)
        {
            throw new NotImplementedException("I'm afraid I can't do that");
        }

But no longer works in Visual Studio 2015:

equals

You can find the issue here: https://github.com/nunit/nunit/issues/1726#issuecomment-255089011

See also: https://twitter.com/jcansdale/status/788704315695398912 😉

Pilchie commented 7 years ago

Also reported in https://developercommunity.visualstudio.com/content/problem/32768/editorbrowsable-attribute-not-respected-by-intelli.html.

kornelijepetak commented 7 years ago

I don't understand why this is so hard to fix? It should be a piece of cake now that Intellisense already got filtering by types, methods, properties, etc.

Is this a VS thing, or Roslyn? Which system is responsible for Intellisense population?

sebastianbk commented 7 years ago

So that's up with this? It was added to the 2.0 milestone but appears to still be an issue. Is there any work for a fix going on regarding this bug?

ryo0ka commented 7 years ago

Any update?

CyrusNajmabadi commented 7 years ago

@ryo0ka PRs welcome :)

sharwell commented 7 years ago

@dpoeschl and I are interested in seeing this completed, but it doesn't currently fix into our scheduling. I'm hoping a community user picks it up so it can ship in 15.5.

Requirements

Time requirement

This will probably take a new user up to a week or more, or an experienced user a few days.

Work would need to start within the next week to ensure time for this to ship in 15.5 (the earliest release where it would currently be possible).

Interested?

If you are interested, please let us know. If more than one person is interested, that would be acceptable for this issue. In particular, it will be good to have more than one person test the intended behavior in Visual Studio 2013, and more than one person review the test cases to ensure they accurately represent the intended behavior so we do not regress in the future.

kzu commented 6 years ago

As far as intended behavior, I'd say the members with EditorBrowsableState.Never should never be show, regardless of how the member was discovered (current project, project reference or assembly reference). An improvement would be to show it anyway for the first two, but I'd say it's very much a nice too have with very low pri.

apmorton commented 6 years ago

@JieCarolHu @sharwell what is the status of this? I am interested in helping to get this issue fixed.

@JieCarolHu it looks like you have done some work on this in the past (#25290), but the PR is closed and appears dead for several months. Is there still work on this? Was it killed because of the required public API changes?

The work in #25290, although incomplete, does appear to fix some cursory unit tests I wrote up for this issue. What is the blocker on finishing this work? Presumably approval or buy-in from whomever owns the relevant public APIs?

MikeLimaSierra commented 5 years ago

After more than 4 years this is still an issue with VS 2017. Can we expect a fix or is it just that low a priority that nobody has time for it?

CyrusNajmabadi commented 5 years ago

Can we expect a fix or is it just that low a priority that nobody has time for it?

@MikeLimaSierra In this case, i believe this would be taken as a community contribution if someone was interested. However, what's important (to me at least) is that a design be created that doesn't break very specific and intentional design changes that were intentionally made to this feature.

Specifically, it was intentional, and based on enough feedback, that the EditorBrowable attribute have different behavior for your current project versus references. This was because there were many teams that effectively wanted to have members that they would see, but that they wanted to hide from others.

So, the design of the attribute was updated to very intentionally operate different within projects in a solution vs metadata references pulled in. The intuition being: if this is your own solution, you understand the design and want to have access to your own members, but when distributing as a library you want to be able to control how others view your assembly.

If we can fix these issues while keeping the above design intact, I'm personally happy.

sharwell commented 5 years ago

@CyrusNajmabadi The bug here is unrelated to the situation you describe, which is covered by #37478. This bug is about a failure of the Roslyn implementation to also eliminate methods hidden by a method whenever it eliminates a method with this attribute.

Scottj1s commented 4 years ago

Ping... it sounds like a conservative fix - one that does not change the original constraints of the attribute, but simply restores VS 2013 behavior - has been validated. What's the status of releasing this fix? Many code generators would benefit greatly.

CyrusNajmabadi commented 4 years ago

@Scottj1s There has been no forward progress on this issue. Sorry!

TyreeJackson commented 4 years ago

Can we expect a fix or is it just that low a priority that nobody has time for it?

@MikeLimaSierra In this case, i believe this would be taken as a community contribution if someone was interested. However, what's important (to me at least) is that a design be created that doesn't break very specific and intentional design changes that were intentionally made to this feature.

Specifically, it was intentional, and based on enough feedback, that the EditorBrowable attribute have different behavior for your current project versus references. This was because there were many teams that effectively wanted to have members that they would see, but that they wanted to hide from others.

So, the design of the attribute was updated to very intentionally operate different within projects in a solution vs metadata references pulled in. The intuition being: if this is your own solution, you understand the design and want to have access to your own members, but when distributing as a library you want to be able to control how others view your assembly.

If we can fix these issues while keeping the above design intact, I'm personally happy.

Can we have a new EditorBrowsableState.Never_We_Really_Mean_It_This_Time state for those of us who don't want to see members even in their own projects? Or perhaps a more appropriate option would be to add a EditorBrowsableState.InternalOnly for those who want to see the member in their projects but not outside of their project. It's strange to change the meaning of Never after so many years of respecting it.

Scottj1s commented 4 years ago

I'm not opposed to a Never_We_Really_Mean_It_This_Time enhancement, as long as it doesn't hold hostage just getting the regression fixed. The former has a higher acceptance bar (new feature), so should be tracked and implemented as a separate issue.

CyrusNajmabadi commented 4 years ago

I've marked this up for grabs. We would be ok taking in a contribution as per the rules that Sam has laid out. Thanks!

stevenbrix commented 4 years ago

We seemed to have traded one evil for another and at the same time introduced a breaking change. Making changes like this doesn't seem like good engineering practice to me.

@Scottj1s @TyreeJackson, something like EditorBrowsableState.Never_We_Really_Mean_It_This_Time feels like the incorrect solution to a problem that never should've existed. I imagine if we went back in time before this breaking change was introduced, we wouldn't come up with this solution.

I understanding wanting to preserve the current behavior of EditorBrowsableState.Never for those that are now dependent on it. But I would vote for an MSBuild project property that can used to opt-in to this behavior:

<PropertyGroup>
   <EditorBrowsableNeverReallyMeansInternalOnly>true</EditorBrowsableNeverReallyMeansInternalOnly>
</PropertyGroup>

Then we can have a more correct solution using your suggestion of EditorBrowsableState.InternalOnly. This should behave identically to the internal keyword, meaning that assemblies who are granted internal access via InternalsVisibleTo would also see metadata tagged with EditorBrowsableState.InternalOnly.

CyrusNajmabadi commented 4 years ago

As above, we woudl be willing to take a contribution here that matches the change that Sam laid out.

aradalvand commented 3 years ago

No updates on this after 5 years?!

CyrusNajmabadi commented 3 years ago

@AradAral See https://github.com/dotnet/roslyn/issues/4434#issuecomment-675676789

sharwell commented 3 years ago

Fixing this just requires this code:

https://github.com/dotnet/roslyn/blob/089be987071b4d088dda1f5a1e35a9cf8d03d2b5/src/Workspaces/Core/Portable/Shared/Extensions/ISymbolExtensions.cs#L664-L676

to use this code instead:

https://github.com/dotnet/roslyn/blob/089be987071b4d088dda1f5a1e35a9cf8d03d2b5/src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs#L4121-L4149

sharwell commented 3 years ago

And the VB side would be derived from this:

https://github.com/dotnet/roslyn/blob/089be987071b4d088dda1f5a1e35a9cf8d03d2b5/src/Compilers/VisualBasic/Portable/Symbols/Source/OverrideHidingHelper.vb#L278-L314

kzu commented 3 years ago

This is a case where given that a team member even knows what code needs to be touched, waiting for the community to figure out how to make a unit test for it, understand how that fits with other unit tests, solely to (mostly) blindly apply the suggested code change, is not very productive work for a contributor (maybe yes?).

Hopefully @sharwell will just submit a PR 🙏

CyrusNajmabadi commented 3 years ago

@kzu everyone is chock full of a ton of work to do. If you could help out, it would be very valuable and appreciated.

sharwell commented 3 years ago

The problem here is the implementations we want are in the compiler and do not have any public API surface.

kzu commented 3 years ago

Totally understood @CyrusNajmabadi, as a maintainer of oss myself, I know that's pretty much always the case 😢 .

@sharwell I suppose that makes it a plus rather than a problem? 😉

jnm2 commented 3 years ago

Source generators are making this more painful. Mentioned in https://github.com/dotnet/roslyn/issues/44929 and a community member was asking about it on Gitter.

jods4 commented 3 years ago

I am the surprised community member :)

In my case the EditorBrowsable.Never properties were internal.

As internal members are never visible outside their own project, I think it's clear that the intention here was to hide them even in the project that declares them.

I suppose EditorBrowsableState.Never + internal could be ruled as hidden in IDE, even in same project.

(For context, this is inside generated code, marked with <auto-generated> at top of file.)

sharwell commented 3 years ago

@jods4 The EditorBrowsable attribute was never design to handle that case, so it would be a new feature request that isn't a copy of this bug report. Can you file it separately?

jnm2 commented 3 years ago

My bad, I didn't realize the in-same-project vs in-same-solution distinction was important.

jods4 commented 3 years ago

@sharwell I am not sure why you say this isn't the right issue and why the attribute was never designed to handle that case.

The MSDN doc says this about the attribute:

EditorBrowsableAttribute is a hint to a designer indicating whether a property or method is to be displayed. You can use this type in a visual designer or text editor to determine what is visible to the user. For example, the IntelliSense engine in Visual Studio uses this attribute to determine whether to show a property or method.

And that's what I'm using it for: I wanted to hide a member from Intellisense.

This issue seems to be about the fact that VS doesn't respect EditorBrowsable from source code in the current solution.

This is what I experimented: I was surprised the member was showing in Intellisense, then found out that it's because it was in the same solution.

(Admittedly MSDN does point out this limitation, I just didn't read everything thoroughly:)

In Visual C#, EditorBrowsableAttribute does not suppress members from a class in the same assembly.

I can still open a new spin-off issue if you think it's worth it.

sharwell commented 3 years ago

@jods4 This issue tracks fixing a bug where the behavior of the attribute changed in Visual Studio 2015 relative to Visual Studio 2013. The cause of the behavior change, and the resulting behavior after the bug is fixed are both well understood. However, the behavior change requested in https://github.com/dotnet/roslyn/issues/4434#issuecomment-780692732 does not align with the behavior of the feature in any previous version of Visual Studio, making it a separate request; if it isn't tracked as a separate issue it's likely to be lost once this issue is fixed.

CyrusNajmabadi commented 3 years ago

This is what I experimented: I was surprised the member was showing in Intellisense, then found out that it's because it was in the same solution.

Note that this behavior is very intentional and is explicitly what we have been asked to deliver in the past. At one point we did actually hide these items (even in the same project), but the feedback was overwhelming that this is not what teams wanted. They wanted a way to have apis they could see and use, but which their external consumers would not see.

Changing this behavior would not be acceptable as this would regress this feature for all teams currently using it in this fashion.

YegorStepanov commented 2 years ago

The proposal that hides Equals and ReferenceEquals for all static types: https://github.com/dotnet/roslyn/discussions/59333

CyrusNajmabadi commented 2 years ago

@YegorStepanov that does not seem to be related to this issue.

YegorStepanov commented 2 years ago

@CyrusNajmabadi My suggestion closes some cases of this problem without writing [EditorBrowsable]

2/4 cases will be automatically closed:

image

image

The third comment will closed too: https://github.com/dotnet/roslyn/issues/4434#issuecomment-255092565

dessyordanova commented 2 years ago

This is still an issue in VS 2022 :-(

CyrusNajmabadi commented 2 years ago

@dessyordanova see https://github.com/dotnet/roslyn/issues/4434#issuecomment-675676789

komdil commented 1 year ago

What is the status of this issue? Is there any progress or PR?

CyrusNajmabadi commented 1 year ago

@komdil see https://github.com/dotnet/roslyn/issues/4434#issuecomment-1211986803

komdil commented 1 year ago

@komdil see #4434 (comment)

I didn't understand, is this something that will not be implemented? In that comments, there are some requirements, but I am not seeing any PR and the issue is still opened

CyrusNajmabadi commented 1 year ago

@komdil

As above, we woudl be willing to take a contribution here that matches the change that Sam laid out.

We would take a contribution to apply the design if a motivated party were to provide it.