Roslyn: Generate Constructor should not copy "protected" from abstract base class

[davkean]

1. CTRL+. on Abstract and choose Generate Constructor

``` C#
public class Concrete : Abstract
{

}

public abstract class Abstract
{
protected Abstract(string foo)
{

}

}

Expected:

``` C#
public class Concrete : Abstract
{
    public Concrete(string foo) : base(foo)
    {
    }
}

public abstract class Abstract
{
    protected Abstract(string foo)
    {

    }
}

Actual:

``` C#
public class Concrete : Abstract
{
protected Concrete(string foo) : base(foo)
{
}
}

public abstract class Abstract
{
protected Abstract(string foo)
{

}

}
```

[angelina-dev]

Hi, I'd like to work on the issue. It's the first time for me in Open Source )

[davkean]

@angelina-dev Absolutely, @sharwell can you get someone to help @angelina-dev on an approach?

[sharwell]

@angelina-dev I am very excited to help you but it's the middle of the night here. I made a note and copied it to my manager to make sure we get back to you with instructions (tips to get started) by Monday at the latest.

[Joe4evr]

Additional thought: What of the case of inheriting with another abstract class?

public abstract class Abstract2 : Abstract
{
}

I would imagine/hope that the protected modifier stays intact in this case.

[angelina-dev]

Hi @sharwell,

What shall we do in this case? Create a public constructor or protected?
```c#
public abstract class Abstract2 : Abstract
{

}

public abstract class Abstract
{
public Abstract(string foo)
{

}

}
```

I think the code should be changed in AbstractGenerateDefaultConstructorsService.AbstractCodeAction CreateConstructorDefinition(...) to check the value of _state.ClassType.IsAbstractClass(). If the current class is not abstract, the constructor should always be generated as public.

[heejaechang]

@angelina-dev for design question, sounds good. dont forget to add test once you fix the bug!

[heejaechang]

I believe you meant here for fix?

http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/GenerateMember/GenerateDefaultConstructors/AbstractGenerateDefaultConstructorsService.AbstractCodeAction.cs,62

and I agree. that looks like where we should make the fix.

the parameter name "constructor" is little bit confusing though. can you rename it to "baseConstructor" since that is the IMethodSymbol actually pointing to?

[CyrusNajmabadi]

Thanks for helping out with this!

'abstract' derived types should preserve the 'protected' accessibility. Non-'abstract' derived types can switch to public accessibility.

Note: if the original constructor was 'internal', we should likely preserve that. So:

| | public | protected | internal | protected internal | private protected |
|--|--|--|--|--|--|
| abstract derived class | same | same | same | same | same |
| derived class | same| public | same | same | same |

In other words, i think we should only switch to 'public' for the case of a 'protected' constructor in a non-abstract class. Everything else we should not touch.

[Neme12]

I'm not quite sure why the accessibility should be changed to public if the base class constructor is not?

[Joe4evr]

I'm not quite sure why the accessibility should be changed to public?

Because what is the point of a non-abstract class with only a protected constructor?

[Neme12]

There are many legitemate reasons to restrict visibility of a constructor,

[heejaechang]

I think we can make it more complex like if there is other public constructor then keep accessibility otherwise, make it public?

but, I think making public when type is not abstract seems majority than the other way. and simpler.

[CyrusNajmabadi]

I'm not quite sure why the accessibility should be changed to public if the base class constructor is not?

Because that's the common pattern with inheritance. People making intermediary abstract types, but only have the constructors be protected so that they should only be accessible to subclasses. People make the actual non-abstract derived class and then want to instantiate it.

The default behavior we have here, of copying the base constructor's accessibility doesn't understand that pattern and basically generates code for that case that almost every user will want to change the accessibility for.

Given that, we should change the behavior here for that specific case.

--

Note: i wrote this feature. And i have never liked this behavior whenever i used it. I just never got around to actually fixing it unfortunately :)

[CyrusNajmabadi]

There are many legitemate reasons to restrict visibility of a constructor,

They are the rarer case. Better to have the default be sensible, and allow users to restrict visibility than to make a concrete type with a protected constructor.

[Neme12]

People making intermediary abstract types, but only have the constructors be protected so that they should only be accessible to subclasses.

I see. I usually keep them public. I guess this comes down to preference and is similar to the question of whether methods in an internal class should be public or internal. If we had a refactoring to make internal classes public, should it change all internal methods as well? I can see how people would say yes if their preference is to make everything internal, even though personally I'd say no to that - if I have an internal method in an internal class, it would usually be intentional.

[heejaechang]

I think we can change design to if base type is abstract and protected but derived type is not abstract but internal, then we put "internal" for ctor rather than "public". otherwise, "public"?

[Joe4evr]

I think we can change design to if base type is abstract and protected but derived type is not abstract but internal, then we put "internal" for ctor rather than "public". otherwise, "public"?

:thinking: If the class is internal, then the ctor is effectively internal anyway, so I'm not sure if that's too useful.

[heejaechang]

@Joe4evr I am with you. just said this

I think we can change design to if base type is abstract and protected but derived type is not abstract but internal, then we put "internal" for ctor rather than "public". otherwise, "public"?

to see how others think. I do think "public" is most common one.

[CyrusNajmabadi]

I Agree with heejae. Just put public. It's very common in .net for people to use 'public' inside something that is itself internal.

[davkean]

I personally always use public inside internal types.

[angelina-dev]

Hey,
If a base class is not abstract, it looks like we should make the constructor of the derived class public. What do you think?
The code in AbstractGenerateDefaultConstructorsService.AbstractCodeAction.cs affects this case too.

[angelina-dev]

Hi, when I am trying to sign .NET CLA, I see this issue:

What shall I do in this case?

[davkean]

@jongalloway?

[angelina-dev]

I've signed it by a reference on GitHub, so it does not block me now.

[angelina-dev]

Here is the pull request: https://github.com/dotnet/roslyn/pull/25866

[jongalloway]

cla2 was shut down a while ago, the CLA signing link on the pull request links to https://cla.dotnetfoundation.org/dotnet/roslyn?pullRequest=25866 - does that work for you?

[angelina-dev]

@jongalloway, it worked for me

[angelina-dev]

Hi,

Thank you for code review. I've made some changes in pull request.
It looks like CyrusNajmabadi's approval is not enough to merge the code:

[heejaechang]

can you link your PR?