Skip to content

Disallow Options source gen produce diagnostics from outside the compilation #88396

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.

Sign up for GitHub

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

Merged
tarekgh merged 3 commits into from
Jul 7, 2023
Merged

Disallow Options source gen produce diagnostics from outside the compilation #88396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b1deb5
Disallow Options source gen produce diagnostics from outside the comp…
tarekgh Jul 4, 2023
7d5cd9e
Apply propagating location inside the compilation to diags.
tarekgh Jul 7, 2023
d485ae5
Address the feedback
tarekgh Jul 7, 2023
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
44 changes: 24 additions & 20 deletions src/libraries/Microsoft.Extensions.Options/gen/Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ public IReadOnlyList<ValidatorType> GetValidatorTypes(IEnumerable<(TypeDeclarati
continue;
}

var membersToValidate = GetMembersToValidate(modelType, true);
Location lowerLocationInCompilation = _compilation.ContainsSyntaxTree(modelType.GetLocation().SourceTree)
? modelType.GetLocation()
: syntax.GetLocation();

var membersToValidate = GetMembersToValidate(modelType, true, lowerLocationInCompilation);
if (membersToValidate.Count == 0)
{
// this type lacks any eligible members
Expand Down Expand Up @@ -239,7 +243,7 @@ private static bool HasOpenGenerics(ITypeSymbol type, out string genericType)
return null;
}

private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool speculate)
private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool speculate, Location lowerLocationInCompilation)
{
// make a list of the most derived members in the model type

Expand All @@ -263,7 +267,11 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
var membersToValidate = new List<ValidatedMember>();
foreach (var member in members)
{
var memberInfo = GetMemberInfo(member, speculate);
Location location = _compilation.ContainsSyntaxTree(member.GetLocation().SourceTree)
? member.GetLocation()
: lowerLocationInCompilation;

var memberInfo = GetMemberInfo(member, speculate, location);
if (memberInfo is not null)
{
if (member.DeclaredAccessibility != Accessibility.Public && member.DeclaredAccessibility != Accessibility.Internal)
Expand All @@ -279,7 +287,7 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
return membersToValidate;
}

private ValidatedMember? GetMemberInfo(ISymbol member, bool speculate)
private ValidatedMember? GetMemberInfo(ISymbol member, bool speculate, Location location)
{
ITypeSymbol memberType;
switch (member)
Expand Down Expand Up @@ -362,7 +370,7 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
if (transValidatorTypeName == null)
{
transValidatorIsSynthetic = true;
transValidatorTypeName = AddSynthesizedValidator(memberType, member);
transValidatorTypeName = AddSynthesizedValidator(memberType, member, location);
}

// pop the stack
Expand Down Expand Up @@ -425,7 +433,7 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
if (enumerationValidatorTypeName == null)
{
enumerationValidatorIsSynthetic = true;
enumerationValidatorTypeName = AddSynthesizedValidator(enumeratedType, member);
enumerationValidatorTypeName = AddSynthesizedValidator(enumeratedType, member, location);
}

// pop the stack
Expand Down Expand Up @@ -455,7 +463,7 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
// generate a warning if the member is const/static and has a validation attribute applied
if (validationAttributeIsApplied)
{
Diag(DiagDescriptors.CantValidateStaticOrConstMember, member.GetLocation(), member.Name);
Diag(DiagDescriptors.CantValidateStaticOrConstMember, location, member.Name);
}

// don't validate the member in any case
Expand All @@ -467,10 +475,10 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
{
if (!HasOpenGenerics(memberType, out var genericType))
{
var membersToValidate = GetMembersToValidate(memberType, false);
var membersToValidate = GetMembersToValidate(memberType, false, location);
if (membersToValidate.Count > 0)
{
Diag(DiagDescriptors.PotentiallyMissingTransitiveValidation, member.GetLocation(), memberType.Name, member.Name);
Diag(DiagDescriptors.PotentiallyMissingTransitiveValidation, location, memberType.Name, member.Name);
}
}
}
Expand All @@ -483,10 +491,10 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
{
if (!HasOpenGenerics(enumeratedType, out var genericType))
{
var membersToValidate = GetMembersToValidate(enumeratedType, false);
var membersToValidate = GetMembersToValidate(enumeratedType, false, location);
if (membersToValidate.Count > 0)
{
Diag(DiagDescriptors.PotentiallyMissingEnumerableValidation, member.GetLocation(), enumeratedType.Name, member.Name);
Diag(DiagDescriptors.PotentiallyMissingEnumerableValidation, location, enumeratedType.Name, member.Name);
}
}
}
Expand All @@ -511,7 +519,7 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
return null;
}

private string? AddSynthesizedValidator(ITypeSymbol modelType, ISymbol member)
private string? AddSynthesizedValidator(ITypeSymbol modelType, ISymbol member, Location location)
{
var mt = modelType.WithNullableAnnotation(NullableAnnotation.None);
if (mt.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
Expand All @@ -525,11 +533,11 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s
return "global::" + validator.Namespace + "." + validator.Name;
}

var membersToValidate = GetMembersToValidate(mt, true);
var membersToValidate = GetMembersToValidate(mt, true, location);
if (membersToValidate.Count == 0)
{
// this type lacks any eligible members
Diag(DiagDescriptors.NoEligibleMember, member.GetLocation(), mt.ToString(), member.ToString());
Diag(DiagDescriptors.NoEligibleMember, location, mt.ToString(), member.ToString());
return null;
}

Expand Down Expand Up @@ -665,14 +673,10 @@ private static string EscapeString(string s)
return sb.ToString();
}

private void Diag(DiagnosticDescriptor desc, Location? location)
{
private void Diag(DiagnosticDescriptor desc, Location? location) =>
_reportDiagnostic(Diagnostic.Create(desc, location, Array.Empty<object?>()));
}

private void Diag(DiagnosticDescriptor desc, Location? location, params object?[]? messageArgs)
{
private void Diag(DiagnosticDescriptor desc, Location? location, params object?[]? messageArgs) =>
_reportDiagnostic(Diagnostic.Create(desc, location, messageArgs));
}
}
}
89 changes: 89 additions & 0 deletions src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Emit;
using Microsoft.DotNet.RemoteExecutor;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Options.Generators;
using SourceGenerators.Tests;
Expand All @@ -11,6 +14,7 @@
using System.Collections.Immutable;
using System.ComponentModel.DataAnnotations;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
Expand Down Expand Up @@ -1175,6 +1179,91 @@ public SecondValidator(int _)
Assert.Equal(DiagDescriptors.ValidatorsNeedSimpleConstructor.Id, diagnostics[0].Id);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
public void ProduceDiagnosticFromOtherAssemblyTest()
{
string source = """
using System.ComponentModel.DataAnnotations;

#nullable enable

namespace AnotherAssembly;

public class ClassInAnotherAssembly
{
[Required]
public string? Foo { get; set; }

// line below causes the generator to emit a warning "SYSLIB1212" but the original location is outside of its compilation (SyntaxTree).
// The generator should emit this diagnostics pointing at the closest location of the failure inside the compilation.
public SecondClassInAnotherAssembly? TransitiveProperty { get; set; }
}

public class SecondClassInAnotherAssembly
{
[Required]
public string? Bar { get; set; }
}
""";

string assemblyName = Path.GetRandomFileName();
string assemblyPath = Path.Combine(Path.GetTempPath(), assemblyName + ".dll");

var compilation = CSharpCompilation
.Create(assemblyName, options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary))
.AddReferences(MetadataReference.CreateFromFile(AppDomain.CurrentDomain.GetAssemblies().FirstOrDefault(a => a.GetName().Name == "System.Runtime").Location))
.AddReferences(MetadataReference.CreateFromFile(typeof(string).Assembly.Location))
.AddReferences(MetadataReference.CreateFromFile(typeof(RequiredAttribute).Assembly.Location))
.AddReferences(MetadataReference.CreateFromFile(typeof(OptionsValidatorAttribute).Assembly.Location))
.AddReferences(MetadataReference.CreateFromFile(typeof(IValidateOptions<object>).Assembly.Location))
.AddSyntaxTrees(CSharpSyntaxTree.ParseText(source));

EmitResult emitResult = compilation.Emit(assemblyPath);
Assert.True(emitResult.Success);

RemoteExecutor.Invoke(async (assemblyFullPath) => {
string source1 = """
using Microsoft.Extensions.Options;

namespace MyAssembly;

[OptionsValidator]
public partial class MyOptionsValidator : IValidateOptions<MyOptions>
{
}

public class MyOptions
{
[ValidateObjectMembers]
public AnotherAssembly.ClassInAnotherAssembly? TransitiveProperty { get; set; }
}
""";

Assembly assembly = Assembly.LoadFrom(assemblyFullPath);

var (diagnostics, generatedSources) = await RoslynTestUtils.RunGenerator(
new Generator(),
new[]
{
assembly,
Assembly.GetAssembly(typeof(RequiredAttribute)),
Assembly.GetAssembly(typeof(OptionsValidatorAttribute)),
Assembly.GetAssembly(typeof(IValidateOptions<object>)),
},
new List<string> { source1 })
.ConfigureAwait(false);

_ = Assert.Single(generatedSources);
var diag = Assert.Single(diagnostics);
Assert.Equal(DiagDescriptors.PotentiallyMissingTransitiveValidation.Id, diag.Id);

// validate the location is inside the MyOptions class and not outside the compilation which is in the referenced assembly
Assert.StartsWith("src-0.cs: (12,", diag.Location.GetLineSpan().ToString());
}, assemblyPath).Dispose();

File.Delete(assemblyPath); // cleanup
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
public async Task CantValidateOpenGenericMembersInEnumeration()
{
Expand Down
1 change: 1 addition & 0 deletions ...urceGeneration.Unit.Tests/Microsoft.Extensions.Options.SourceGeneration.Unit.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<EnableDefaultItems>true</EnableDefaultItems>
<!-- <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules> -->
<DefineConstants>$(DefineConstants);ROSLYN4_0_OR_GREATER;ROSLYN4_4_OR_GREATER;ROSLYN_4_0_OR_GREATER</DefineConstants>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
</PropertyGroup>

<ItemGroup>
Expand Down