Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/OpenApi/gen/Helpers/AssemblyTypeSymbolsVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Threading;
using Microsoft.AspNetCore.OpenApi.SourceGenerators.Xml;
using Microsoft.CodeAnalysis;

namespace Microsoft.AspNetCore.OpenApi.SourceGenerators;
Expand Down Expand Up @@ -44,7 +45,7 @@ public override void VisitNamedType(INamedTypeSymbol type)
{
_cancellationToken.ThrowIfCancellationRequested();

if (!IsDeclaredInAssembly(type) || !_exportedTypes.Add(type))
if (!IsDeclaredInAssembly(type) || !type.IsAccessibleType() || !_exportedTypes.Add(type))
{
return;
}
Expand All @@ -61,7 +62,7 @@ public override void VisitNamedType(INamedTypeSymbol type)
foreach (var property in properties)
{
_cancellationToken.ThrowIfCancellationRequested();
if (IsDeclaredInAssembly(property) && _exportedProperties.Add(property))
if (IsDeclaredInAssembly(property) && property.IsAccessibleType() && _exportedProperties.Add(property))
{
property.Type.Accept(this);
}
Expand All @@ -70,7 +71,7 @@ public override void VisitNamedType(INamedTypeSymbol type)
foreach (var method in methods)
{
_cancellationToken.ThrowIfCancellationRequested();
if (IsDeclaredInAssembly(method) && _exportedMethods.Add(method))
if (IsDeclaredInAssembly(method) && method.IsAccessibleType() && _exportedMethods.Add(method))
{
method.Accept(this);
}
Expand Down
22 changes: 22 additions & 0 deletions src/OpenApi/gen/Helpers/ISymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,26 @@ public static IEnumerable<INamedTypeSymbol> GetBaseTypes(this ITypeSymbol? type)
current = current.BaseType;
}
}

public static bool IsAccessibleType(this ISymbol symbol)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps us avoid generating XML comments for non-public types and internal properties in non-public types.

Note: this does mean that DTOs have to be defined as public types if they are defined in the same assembly as the API (see where this is called in the AssemnlyTypeSymbolVisitor above). We can consider changing this behavior to allow XML comments on internal types in the application assembly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit overaggressive. I'm guessing we'll change this behavior a bit in the future.

Any reason to disallow the type entirely just because it has internal types?

Copy link
Member

@martincostello martincostello Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just hit this error in preview 2:

C:\Coding\martincostello\alexa-london-travel-site\artifacts\obj\LondonTravel.Site\debug\Microsoft.AspNetCore.OpenApi.SourceGenerators\Microsoft.AspNetCore.OpenApi.SourceGenerators.XmlCommentGenerator\OpenApiXmlCommentSupport.generated.cs(207,125): error CS0122: 'CustomHttpHeadersMiddleware.Csp' is inaccessible due to its protection level

This relates to a private nested class in public class that has some /// comments on the internal const string members in it.

Would this change fix that scenario too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more scenario for you - seems like it's trying to document things to do with the Regex source generator (example):

C:\Coding\martincostello\adventofcode\artifacts\obj\AdventOfCode.Site\debug\Microsoft.AspNetCore.OpenApi.SourceGenerators\Microsoft.AspNetCore.OpenApi.SourceGenerators.XmlCommentGenerator\OpenApiXmlCommentSupport.generated.cs(1589,94): error CS0234: The type or namespace name 'HexColor_1' does not exist in the namespace 'System.Text.RegularExpressions.Generated' (are you missing an assembly reference?)

If not catered for by this change, I can open a new issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on whether the type in question is defined in or outside the assembly.

I believe these bits have landed in the nightly preview3 SDK if you want to take another bump to try it out.

Alternatively, I'd recommend filing an issue so we can at least capture this scenario in a test case even if it is already fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try out a nightly tomorrow and see if you think it should be fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed these two are catered for as of 10.0.100-preview.3.25169.19, but confirmed #61019 is still an issue and found two new ones 😅. Issues shortly...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they're probably the same one issue: #61035

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these bits have landed in the nightly preview3 SDK if you want to take another bump to try it out.

Alternatively, I'd recommend filing an issue so we can at least capture this scenario in a test case even if it is already fixed.

Found a different case where a private type causes a compiler error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
// Check if the symbol itself is public
if (symbol.DeclaredAccessibility != Accessibility.Public)
{
return false;
}

// Check if all containing types are public as well
var containingType = symbol.ContainingType;
while (containingType != null)
{
if (containingType.DeclaredAccessibility != Accessibility.Public)
{
return false;
}
containingType = containingType.ContainingType;
}

return true;
}
}
21 changes: 16 additions & 5 deletions src/OpenApi/gen/XmlCommentGenerator.Emitter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ internal static string GenerateXmlCommentSupportSource(string commentsFromXmlFil
// </auto-generated>
//------------------------------------------------------------------------------
#nullable enable
// Suppress warnings about obsolete types and members
// in generated code
#pragma warning disable CS0612, CS0618
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppress obsoletion warnings in generated code. If someone happens to be using an obsolete type in their API, we don't want to resurface this warning in generated code where we call typeof(ObsolteType).


namespace System.Runtime.CompilerServices
{
Expand Down Expand Up @@ -487,7 +490,7 @@ internal static string EmitSourceGeneratedXmlComment(XmlComment comment)
else
{
codeWriter.Write("[");
for (int i = 0; i < comment.Examples.Count; i++)
for (var i = 0; i < comment.Examples.Count; i++)
{
var example = comment.Examples[i];
codeWriter.Write(FormatStringForCode(example));
Expand All @@ -506,13 +509,18 @@ internal static string EmitSourceGeneratedXmlComment(XmlComment comment)
else
{
codeWriter.Write("[");
for (int i = 0; i < comment.Parameters.Count; i++)
for (var i = 0; i < comment.Parameters.Count; i++)
{
var parameter = comment.Parameters[i];
var exampleLiteral = string.IsNullOrEmpty(parameter.Example)
? "null"
: FormatStringForCode(parameter.Example!);
codeWriter.Write($"new XmlParameterComment(@\"{parameter.Name}\", @\"{parameter.Description}\", {exampleLiteral}, {(parameter.Deprecated == true ? "true" : "false")})");
codeWriter.Write("new XmlParameterComment(");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below we format strings in the emitted code so that we can properly escape quoted strings inside XML descriptions/summaries/etc.

codeWriter.Write(FormatStringForCode(parameter.Name) + ", ");
codeWriter.Write(FormatStringForCode(parameter.Description) + ", ");
codeWriter.Write(exampleLiteral + ", ");
codeWriter.Write(parameter.Deprecated == true ? "true" : "false");
codeWriter.Write(")");
if (i < comment.Parameters.Count - 1)
{
codeWriter.Write(", ");
Expand All @@ -528,10 +536,13 @@ internal static string EmitSourceGeneratedXmlComment(XmlComment comment)
else
{
codeWriter.Write("[");
for (int i = 0; i < comment.Responses.Count; i++)
for (var i = 0; i < comment.Responses.Count; i++)
{
var response = comment.Responses[i];
codeWriter.Write($"new XmlResponseComment(@\"{response.Code}\", @\"{response.Description}\", {(response.Example is null ? "null" : FormatStringForCode(response.Example))})");
codeWriter.Write("new XmlResponseComment(");
codeWriter.Write(FormatStringForCode(response.Code) + ", ");
codeWriter.Write(FormatStringForCode(response.Description) + ", ");
codeWriter.Write(response.Example is null ? "null)" : FormatStringForCode(response.Example) + ")");
if (i < comment.Responses.Count - 1)
{
codeWriter.Write(", ");
Expand Down
6 changes: 5 additions & 1 deletion src/OpenApi/gen/XmlCommentGenerator.Parser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ public sealed partial class XmlCommentGenerator
var comments = new List<(MemberKey, XmlComment?)>();
foreach (var (name, value) in input.RawComments)
{
if (DocumentationCommentId.GetFirstSymbolForDeclarationId(name, compilation) is ISymbol symbol)
if (DocumentationCommentId.GetFirstSymbolForDeclarationId(name, compilation) is ISymbol symbol &&
symbol.IsAccessibleType() &&
// Skip static classes that are just containers for members with annotations
// since they cannot be instantiated.
symbol is not INamedTypeSymbol { TypeKind: TypeKind.Class, IsStatic: true })
{
var parsedComment = XmlComment.Parse(symbol, compilation, value, cancellationToken);
if (parsedComment is not null)
Expand Down
6 changes: 3 additions & 3 deletions src/OpenApi/gen/XmlComments/MemberKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public static MemberKey FromMethodSymbol(IMethodSymbol method, Compilation compi

returnType = actualReturnType.TypeKind == TypeKind.TypeParameter
? "typeof(object)"
: $"typeof({actualReturnType.ToDisplayString(_typeKeyFormat)})";
: $"typeof({ReplaceGenericArguments(actualReturnType.ToDisplayString(_typeKeyFormat))})";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure that all generics are emitted as open generics for method declarations. We already do this elsewhere in the MemberKey for the declaring type and the property type but not for methods.

}

// Handle extension methods by skipping the 'this' parameter
Expand All @@ -62,10 +62,10 @@ public static MemberKey FromMethodSymbol(IMethodSymbol method, Compilation compi
// For params arrays, use the array type
if (p.IsParams && p.Type is IArrayTypeSymbol arrayType)
{
return $"typeof({arrayType.ToDisplayString(_typeKeyFormat)})";
return $"typeof({ReplaceGenericArguments(arrayType.ToDisplayString(_typeKeyFormat))})";
}

return $"typeof({p.Type.ToDisplayString(_typeKeyFormat)})";
return $"typeof({ReplaceGenericArguments(p.Type.ToDisplayString(_typeKeyFormat))})";
})
.ToArray();

Expand Down
2 changes: 1 addition & 1 deletion src/OpenApi/sample/Controllers/TestController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class RouteParamsContainer
[FromRoute]
[MinLength(5)]
[UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode", Justification = "MinLengthAttribute works without reflection on string properties.")]
public string? Name { get; set; }
public string Name { get; set; }
}

public record MvcTodo(string Title, string Description, bool IsCompleted);
Expand Down
8 changes: 2 additions & 6 deletions src/OpenApi/sample/Sample.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

<PropertyGroup>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
<Nullable>enable</Nullable>
<!-- Disable to match nullability annotations in test environment. -->
<Nullable>disable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<!-- Required to generated trimmable Map-invocations. -->
<EnableRequestDelegateGenerator>true</EnableRequestDelegateGenerator>
Expand All @@ -24,11 +25,6 @@
<Reference Include="Microsoft.AspNetCore.Mvc" />
</ItemGroup>

<ItemGroup>
<Compile Include="../test/Microsoft.AspNetCore.OpenApi.Tests/Shared/SharedTypes.cs" />
<Compile Include="../test/Microsoft.AspNetCore.OpenApi.Tests/Shared/SharedTypes.Polymorphism.cs" />
</ItemGroup>

<!-- Required to generated trimmable Map-invocations. -->
<ItemGroup>
<ProjectReference Include="$(RepoRoot)/src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.RequestDelegateGenerator/Microsoft.AspNetCore.Http.RequestDelegateGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />
Expand Down
181 changes: 181 additions & 0 deletions src/OpenApi/sample/Shared/SharedTypes.DeeplyNestedType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

public class DeeplyNestedLevel1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we require all types to be public for discovery, we moved the shared types that are used by tests and the sample app to the sample app since the tests reference the sample app.

Previously, we used compile includes with internal types which doesn't work in this new model.

{
public required DeeplyNestedLevel2 Item2 { get; set; }
}

public class DeeplyNestedLevel2
{
public required DeeplyNestedLevel3 Item3 { get; set; }
}

public class DeeplyNestedLevel3
{
public required DeeplyNestedLevel4 Item4 { get; set; }
}

public class DeeplyNestedLevel4
{
public required DeeplyNestedLevel5 Item5 { get; set; }
}

public class DeeplyNestedLevel5
{
public required DeeplyNestedLevel6 Item6 { get; set; }
}

public class DeeplyNestedLevel6
{
public required DeeplyNestedLevel7 Item7 { get; set; }
}

public class DeeplyNestedLevel7
{
public required DeeplyNestedLevel8 Item8 { get; set; }
}

public class DeeplyNestedLevel8
{
public required DeeplyNestedLevel9 Item9 { get; set; }
}

public class DeeplyNestedLevel9
{
public required DeeplyNestedLevel10 Item10 { get; set; }
}

public class DeeplyNestedLevel10
{
public required DeeplyNestedLevel11 Item11 { get; set; }
}

public class DeeplyNestedLevel11
{
public required DeeplyNestedLevel12 Item12 { get; set; }
}

public class DeeplyNestedLevel12
{
public required DeeplyNestedLevel13 Item13 { get; set; }
}

public class DeeplyNestedLevel13
{
public required DeeplyNestedLevel14 Item14 { get; set; }
}

public class DeeplyNestedLevel14
{
public required DeeplyNestedLevel15 Item15 { get; set; }
}

public class DeeplyNestedLevel15
{
public required DeeplyNestedLevel16 Item16 { get; set; }
}

public class DeeplyNestedLevel16
{
public required DeeplyNestedLevel17 Item17 { get; set; }
}

public class DeeplyNestedLevel17
{
public required DeeplyNestedLevel18 Item18 { get; set; }
}

public class DeeplyNestedLevel18
{
public required DeeplyNestedLevel19 Item19 { get; set; }
}

public class DeeplyNestedLevel19
{
public required DeeplyNestedLevel20 Item20 { get; set; }
}

public class DeeplyNestedLevel20
{
public required DeeplyNestedLevel21 Item21 { get; set; }
}

public class DeeplyNestedLevel21
{
public required DeeplyNestedLevel22 Item22 { get; set; }
}

public class DeeplyNestedLevel22
{
public required DeeplyNestedLevel23 Item23 { get; set; }
}

public class DeeplyNestedLevel23
{
public required DeeplyNestedLevel24 Item24 { get; set; }
}

public class DeeplyNestedLevel24
{
public required DeeplyNestedLevel25 Item25 { get; set; }
}

public class DeeplyNestedLevel25
{
public required DeeplyNestedLevel26 Item26 { get; set; }
}

public class DeeplyNestedLevel26
{
public required DeeplyNestedLevel27 Item27 { get; set; }
}

public class DeeplyNestedLevel27
{
public required DeeplyNestedLevel28 Item28 { get; set; }
}

public class DeeplyNestedLevel28
{
public required DeeplyNestedLevel29 Item29 { get; set; }
}

public class DeeplyNestedLevel29
{
public required DeeplyNestedLevel30 Item30 { get; set; }
}

public class DeeplyNestedLevel30
{
public required DeeplyNestedLevel31 Item31 { get; set; }
}

public class DeeplyNestedLevel31
{
public required DeeplyNestedLevel32 Item32 { get; set; }
}

public class DeeplyNestedLevel32
{
public required DeeplyNestedLevel33 Item33 { get; set; }
}

public class DeeplyNestedLevel33
{
public required DeeplyNestedLevel34 Item34 { get; set; }
}

public class DeeplyNestedLevel34
{
public required DeeplyNestedLevel35 Item35 { get; set; }
}

public class DeeplyNestedLevel35
{
public required DeeplyNestedLevel36 Item36 { get; set; }
}

public class DeeplyNestedLevel36
{
}
Loading
Loading