Partial partials: Require "partial" on only some parts of a partial type

[gafter]

Relax the C# language specification so that it is not required that every part of a partial type contain the partial modifier.

This is proposed in support of #5561.

[HaloFour]

Out of curiosity is there much of a reason to have any of the partial types actually contain the partial modifier after this change? The compiler would have to parse through all of the duplicate types just in case it'll eventually run into one marked partial anyway, if this change is to be implemented.

[mattwar]

The compiler has to look at all the duplicates anyway. The existence of a partial keyword lets the compiler know the difference between an intentional duplicate reference and an accidental one.

[dsaf]

Will there be any IDE enhancements to make it still obvious that a class is partial when writing them manually? It might become hard to tell whether substantial parts of a class are defined in a completely different file without exploration. Some sort of gutter icon or a tooltip info would be helpful. Not sure R# team will address this timely. Somewhat related to #5739.

Also it might feel inconsistent with partial methods.

[HaloFour]

If I assume correctly that this change is largely to support supercedes (or something like it) so that automatically generated code could contain a partial class definition without requiring the target class to be marked partial, correct? That would mean that in those scenarios a developer would never be explicitly using a partial modifier, but the types would be partial types and the protection against accidental duplication wouldn't help.

Note that I'm simply playing devil's advocate. I like the idea of not having to mark classes as partial.

[gafter]

@HaloFour That's right. Personally I don't like this.

[bondsbw]

For this situation:

Assembly 1:

namespace x.y.z
{
    public class Foo {}
}

Assembly 2:

namespace x.y.z
{
    public partial class Foo {}
}

If I decided to move Foo from Assembly 1 to Assembly 2, a compile-time error would be the current result ("Duplicate definition 'x.y.z.Foo'"). After this proposal, it would result in merging the classes. I prefer the error.

[alrz]

👎

[sharwell]

I am not convinced the IDE could overcome the usability problems (confusion) which could arise from this change. At least for now this seems like it would cause more problems than it would solve.

[mattwar]

@sharwell this is the same partial behavior we have for VB currently.

[paulomorgado]

👎

[alrz]

@mattwar VB is less strict than C# in a lot of areas, If someone is more comfortable with that level of strictness he surly would use VB. I'm saying that this one isn't a reasonable execuse to do this IMO. It's like making C# case-insensitive just because VB is.

[mattwar]

@alrz I was making the point to show that we already do have to overcome the IDE problems, because VB already works that way.

[alrz]

@mattwar

The existence of a partial keyword lets the compiler know the difference between an intentional duplicate reference and an accidental one.

What about the above then?

[mattwar]

@alzr I was claiming that the concern that @sharwell had expressed about the need to overcome an IDE problem if we did change the language was not an issue because the IDE already overcomes that problem for VB.

[alrz]

The nice thing about source.roslyn.io is that partial on types becomes a link to a list of all parts. and if it was not required you woudn't have a clue that the type is, well, partial, and rest of the definition is somewhere else. I presume IDE can help with this, but I think it should be bright and clear in the source code itself. I don't see why #5561 need this, just to omit seven characters?

[mattwar]

The reason why its important for this feature to exist, is that without it, all developers using source generation tools would have to be fully aware at all times of the minutia of how these tools work. You might think that its a great idea, and that all developers should always know. However, having to know becomes a burden and a barrier to entry for the new capabilities offered by the source generators. You have to know that when you put the [Feature] attribute on your class or member, you also have to make sure the class is declared as partial, or the whole thing just breaks with odd errors at compile time, because this feature specifically works by generating new source code and extending your class by making another partial declaration at compile time (as opposed to features that work by examining the attribute at runtime, or maybe post-compile rewrites, etc.)

You never get to the point where features introduced this way just melt into the background, and the feature becomes the intention/semantic and not its specific implementation.

[dsaf]

You have to know that when you put the [Feature] attribute on your class or member, you also have to make sure the class is declared as partial, or the whole thing just breaks with odd errors at compile time...

Isn't this just a workaround for something that is perfectly fixable by making AttributeTargets more exact?
I have always found them too broad to make users fall into the "pit of success", e.g.: http://stackoverflow.com/questions/27552731/why-can-you-expose-private-methods-publically-in-a-wcf-service

It could look like this

[AttributeUsage(AttributeTargets.PartialClass)]
public class FeatureAttribute : Attribute { }

or this

[AttributeUsage(AttributeTargets.Class)]
[AttributeTargetFlags(AttributeTargetFlags.Public | AttributeTargetFlags.Partial | AttributeTargetFlags.Instance)]
public class FeatureAttribute : Attribute { }

@gafter would it make sense to open this as a separate alternative suggestion?

Failing that, custom Roslyn analyzers are also an option (helps dog-fooding as well). I am using post-compile custom validation in my project and it works well (e.g. "attribute should only be applied to public instance string properties" kind of exactness and flexibility).

I personally prefer explicitness and preciseness to hidden magic and "conveniece". We already have some of the latter: http://stackoverflow.com/questions/4111627/why-arent-there-implicit-conversions-in-f .

[HaloFour]

@dsaf

Given that partial classes don't exist in the CLR I don't think it would make sense to require a modification to the AttributeTargets enum in the BCL for this.

However, maybe having an attribute that the compiler would recognize as requiring the target class be partial could be useful. Either the compiler could treat those decorated classes as silently partial, or the compiler could error with a more useful diagnostic so that the developer is aware that they must make the class partial themselves.

[paulomorgado]

@dsaf, there is no such thing as a "partial class" but "partial 'class definitions'".

In order for an attribute to be applied only to a partial class definition, it would need to be a compile-time-only attribute.

[alrz]

it would need to be a compile-time-only attribute.

What if they expose #5561 API through source-only attributes like #8075? I think that would make it more coherent and structured rather than one global CodeInjector.

[dsaf]

@paulomorgado #6671

[fabianoliver]

@HaloFour and @paulomorgado

I agree on paulo's idea of considering a separate, compile-time only attribute that works similar to AttributeUsage. Guess compile time checks should be fine, as to my understanding there is no way to use code generation during runtime anyway?

Having said that, I think it should check for replacability of a target, instead of just checking for partial classes - as I'd assume there will be additional ways to block methods etc. from being replaced, even if defined in partial classes.

[HaloFour]

@BogeyCode

I agree on paulo's idea of considering a separate, compile-time only attribute that works similar to AttributeUsage.

Perhaps, but C# doesn't really have the notion of "compile-time only" attributes. A handful of pseudo-attributes (like SerializableAttribute and DllImportAttribute) are elided during the compilation process, but there's no way to define your own attribute that would enable that.

Also, as far as I'm aware there's nothing about source generators that actually requires the use of attributes at all. I imagine many/most AOP scenarios would use attributes but it would be up to the generator to analyze and act on them.

Guess compile time checks should be fine, as to my understanding there is no way to use code generation during runtime anyway?

Correct, the code generation happens during compile time and the generated code becomes a part of that assembly. It is strictly a compiler feature.

Having said that, I think it should check for replacability of a target, instead of just checking for partial classes - as I'd assume there will be additional ways to block methods etc. from being replaced, even if defined in partial classes.

Nothing like that is in the current spec. There would be nothing that would prevent a partial class definition from using replace on any member in another partial class definition.

[davidroth]

Personally I like the idea. It would make switching from fody/postsharp/etc to source generators much easier, because one wouldn't have to change all existing classes to make them partial.

Also what's the story with base types? If I want them to be processed by source generators, do I have to declare all base types as partial as well? (E.x. in the INotifyPropertyChanged scenario, it's common to have some base VM class with additional properties + the event declaration)

[paulomorgado]

@davidroth, types aren't partial. Their source code can be. By the time a type is a base type of another type, that type is a whole and unchangable entity.

[gafter]

We are now taking language feature discussion in other repositories:

• https://github.com/dotnet/csharplang for C# specific issues
• https://github.com/dotnet/vblang for VB-specific features
• https://github.com/dotnet/csharplang for features that affect both languages

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to #18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.

---

I am not moving this particular issue because I don't particularly like it. If @mattwar (who was championing the idea) wants to continue with this madness, he will have to port this issue to the csharplang repo.