Introduce new InternalsVisibleToAnalyzer analyzer (ACZ0112) with FriendAttribute concept

[christothes]

Summary

In scenarios where it is necessary to expose internal between client libraries via the InternalVisibleToAttribute, we want a more robust mechanism to ensure that we can be explicit about which APIs are intended to be consumed externally.

This new analyzer validates that when a client library references an internal type, method, or property from another assembly that it is decorated with a FriendAttribute.

There is a related API View PR (#7116) to treat types, methods, and properties decorated with the FriendAttribute as public API surface, so that changes can be raised as breaking.

[KrzysztofCwalina]

@stephentoub, @terrajobst, is that something you'd like to provide in the BCL?

[heaths]

This analyzer wouldn't cause issues with the many test projects referencing internal members, would it?

[christothes]

This analyzer wouldn't cause issues with the many test projects referencing internal members, would it?

No, because Test projects don't reference this analyzer by default (link)

[heaths]

Maybe it's fine as-is, but would we also want to check for a namespace to avoid conflicts with any other FriendAttributes down the road (third-party, shared source, whatever - however unlikely)? What does .NET do in similar cases of look-alike attributes e.g., nullable attributes? IMO, we should follow .NET's practice.

[pallavit]

Do we want to test against inheritance scenarios?

[pallavit]

Also curious what are the scenarios where we see this being used? Is it a temporary feature we want to light-up or is this a more longer term dependency? We restricted the usage of InternalsVisibleToAttribute in .NET Framework so curious why do we want to make it more acceptable here?

[christothes]

Do we want to test against inheritance scenarios?

I think we have that covered for interfaces and base classes.

[christothes]

Also curious what are the scenarios where we see this being used? Is it a temporary feature we want to light-up or is this a more longer term dependency? We restricted the usage of InternalsVisibleToAttribute in .NET Framework so curious why do we want to make it more acceptable here?

Azure.Identity will make use of this. I'm not sure if there are others that might in the future. In that case it was a design tradeoff we felt was the best choice given the alternatives.

[pallavit]

Azure.Identity will make use of this. I'm not sure if there are others that might in the future. In that case it was a design tradeoff we felt was the best choice given the alternatives.

Should we consider restricting it to Azure.Identity for now? Putting this in APIView ensures we scan what we are taking dependency on - which is great! However how do we ensure that the internal type will continue to be backward compatible and follow the same principles like any public type would do?

[christothes]

Azure.Identity will make use of this. I'm not sure if there are others that might in the future. In that case it was a design tradeoff we felt was the best choice given the alternatives.

Should we consider restricting it to Azure.Identity for now? Putting this in APIView ensures we scan what we are taking dependency on - which is great! However how do we ensure that the internal type will continue to be backward compatible and follow the same principles like any public type would do?

I don't think we need to restrict it. The PR and API review processes should prevent mass adoption of this pattern. Once this is exposed in API View, any change to a decorated internal type, property, method, etc. will be flagged for review as with any other public change.

[stephentoub]

@stephentoub, @terrajobst, is that something you'd like to provide in the BCL?

This looks very familiar from a few years back ;-)

I don't fundamentally have a problem with it, though if we were to pursue it I'd want to run it through the C# / LDM, as InternalsVisibleTo is in part a compiler feature, and this is really just an extension of it. I'd also want to name it something more aligned with InternalsVisibleTo, like VisibleTo, or InternallyVisibleTo, or something along those lines. I'd leave it up to Immo if it's something he wants to pursue.

[terrajobst]

@stephentoub @KrzysztofCwalina

I'd leave it up to Immo if it's something he wants to pursue.

I asked on Twitter to gauge interest. I think interest would be limited, like private protected, but that doesn't mean the C# compiler folks wouldn't care. They shipped a similar feature in Roslyn itself to mark their interfaces as closed, i.e. failing people for trying to implement them.

[github-actions]

Hi @christothes. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.