Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XML doc comments need to respect accessibility limitations for referenced assemblies #71442

Open
jaredpar opened this issue Jan 2, 2024 · 6 comments

Comments

@jaredpar
Copy link
Member

jaredpar commented Jan 2, 2024

XML doc comments do not respect accessibility today when referring to symbols in assemblies. Instead they act as if they have [InternalsVisibleTo] any assembly. This is behavior that goes back to C# 1.0 but it is becoming a problem in today's ecosystem where there is an increasing amount of shared code between assemblies and source generators that produce types with identical FQN. When this happens XML doc comment references to such types become ambiguous and the only resolution available to customers is extern alias's.

A real world example of this problem is winforms PR 10320. In this case the CSWin32 generator is used in two assemblies: System.Drawing.Common and System.Windows.Forms.Primitives. The consuming assembly has IVT for one but not the other. That means uses of the types in code is fine because the compiler only considers the assembly with IVT. Yet the exact same references in XML doc comments are ambiguous because they have effectively IVT to both assemblies.

This feels like a problem that we're going to hit more and more as source generators and shared code increase in popularity. Starting in .NET 9 I think we need to consider changing XML doc comments to respect accessibility when using language version 13.0 or higher. That is a breaking change and it's likely to impact a non-trivial number of customers as this is behavior that goes back to C# 1.0. To mitigate we should introduce a feature flag to control this behavior: XmlDocLegacyVisibility. When set to true or language version is 12 or earlier the compiler will exhibit the existing behavior, otherwise it will use normal accessibility when considering external symbols.

Note: no change is being proposed for XML doc accessibility to symbols in the same assembly. Those will continue to have unrestricted access.

Issues for context:

@elachlan
Copy link
Contributor

elachlan commented Jan 2, 2024

Related:
#4033
#51013

@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 3, 2024
@jaredpar jaredpar added this to the 17.10 milestone Jan 3, 2024
@sharwell
Copy link
Member

sharwell commented Jan 3, 2024

In the past, this issue has been fairly easy to resolve with using alias directives. This includes dotnet/winforms#10525 and dotnet/winforms#10320.

@JeremyKuhne
Copy link
Member

In the past, this issue has been fairly easy to resolve with using alias directives. This includes dotnet/winforms#10525 and dotnet/winforms#10320.

@sharwell If this involves manually creating an alias for every single conflicting type I wouldn't call this easy. We have a significant amount of interop (thousands of definitions).

@sharwell
Copy link
Member

sharwell commented Jan 3, 2024

@JeremyKuhne It only applies to identifiers that appear in a cref attribute, which is many fewer than exist.

Note that one possible resolution to the issue is for the C# compiler to silently prefer accessible symbols before issuing ambiguity warnings.

@JeremyKuhne
Copy link
Member

It only applies to identifiers that appear in a cref attribute, which is many fewer than exist.

I also applies to <inheritdoc>, which we're also doing.

@sharwell
Copy link
Member

sharwell commented Jan 3, 2024

Also related to #77

@jaredpar jaredpar modified the milestones: 17.10, Backlog Apr 1, 2024
ericstj added a commit to carlossanlop/runtime that referenced this issue Oct 19, 2024
See dotnet/roslyn#71442 We refer to MathF from
a comment which hits this bug problem in Roslyn where it thinks it's
ambiguous because visibility is not considered when resolving cref's in
doc comments.
carlossanlop pushed a commit to carlossanlop/runtime that referenced this issue Oct 30, 2024
See dotnet/roslyn#71442 We refer to MathF from
a comment which hits this bug problem in Roslyn where it thinks it's
ambiguous because visibility is not considered when resolving cref's in
doc comments.
carlossanlop pushed a commit to carlossanlop/runtime that referenced this issue Nov 7, 2024
See dotnet/roslyn#71442 We refer to MathF from
a comment which hits this bug problem in Roslyn where it thinks it's
ambiguous because visibility is not considered when resolving cref's in
doc comments.
carlossanlop added a commit to dotnet/runtime that referenced this issue Nov 22, 2024
#108806)

* Bump dependency versions of packages that now ship out of maintenance-packages (preview)

- Microsoft.Bcl.HashCode
- System.Buffers
- System.Memory
- System.Threading.Tasks.Extensions
- System.Runtime.CompilerServices.Unsafe

* Workaround name conflict for MathF: See dotnet/roslyn#71442 We refer to MathF from
a comment which hits this bug problem in Roslyn where it thinks it's
ambiguous because visibility is not considered when resolving cref's in
doc comments.
* Permit maintenance-packages prebuilts
* Suppress CS0618 on site in System.Data.Common.Tests - 'Sql*' is obsolete: 'Use the Microsoft.Data.SqlClient package instead.
---------
Co-authored-by: Eric StJohn <ericstj@microsoft.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants