From 0b1deb55c7caadad1fb67e34b7184918e7e5c8c0 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 4 Jul 2023 15:22:18 -0700 Subject: [PATCH 1/3] Disallow Options source gen produce diagnostics from outside the compilation --- .../gen/Parser.cs | 12 ++- .../tests/SourceGeneration.Unit.Tests/Main.cs | 85 +++++++++++++++++++ ...Options.SourceGeneration.Unit.Tests.csproj | 1 + 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs b/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs index dad40ee3d7f43e..a7f324d0203a43 100644 --- a/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs +++ b/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs @@ -667,12 +667,20 @@ private static string EscapeString(string s) private void Diag(DiagnosticDescriptor desc, Location? location) { - _reportDiagnostic(Diagnostic.Create(desc, location, Array.Empty())); + // ignore the diagnostics outside the current compilation + if (location?.Kind == LocationKind.SourceFile) + { + _reportDiagnostic(Diagnostic.Create(desc, location, Array.Empty())); + } } private void Diag(DiagnosticDescriptor desc, Location? location, params object?[]? messageArgs) { - _reportDiagnostic(Diagnostic.Create(desc, location, messageArgs)); + // ignore the diagnostics outside the current compilation + if (location?.Kind == LocationKind.SourceFile) + { + _reportDiagnostic(Diagnostic.Create(desc, location, messageArgs)); + } } } } diff --git a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs index 08805c76b308d3..fb8d88feef70ec 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs @@ -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; @@ -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; @@ -1175,6 +1179,87 @@ 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" outside of its compilation. + // The generator should not emit this diagnostics in this case. + 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).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 + { + } + + 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)), + }, + new List { source1 }) + .ConfigureAwait(false); + + Assert.Empty(diagnostics); // no diagnostics should be produced + _ = Assert.Single(generatedSources); + }, assemblyPath).Dispose(); + + File.Delete(assemblyPath); // cleanup + } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))] public async Task CantValidateOpenGenericMembersInEnumeration() { diff --git a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Microsoft.Extensions.Options.SourceGeneration.Unit.Tests.csproj b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Microsoft.Extensions.Options.SourceGeneration.Unit.Tests.csproj index c18245aa1b5f44..b544bab78b882b 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Microsoft.Extensions.Options.SourceGeneration.Unit.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Microsoft.Extensions.Options.SourceGeneration.Unit.Tests.csproj @@ -7,6 +7,7 @@ true $(DefineConstants);ROSLYN4_0_OR_GREATER;ROSLYN4_4_OR_GREATER;ROSLYN_4_0_OR_GREATER + true From 7d5cd9ec70d752618bdd40a9d83264a199330f87 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 6 Jul 2023 18:08:36 -0700 Subject: [PATCH 2/3] Apply propagating location inside the compilation to diags. --- .../gen/Parser.cs | 56 +++++++++---------- .../tests/SourceGeneration.Unit.Tests/Main.cs | 10 +++- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs b/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs index a7f324d0203a43..d71138daa91736 100644 --- a/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs +++ b/src/libraries/Microsoft.Extensions.Options/gen/Parser.cs @@ -93,7 +93,11 @@ public IReadOnlyList 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 @@ -239,7 +243,7 @@ private static bool HasOpenGenerics(ITypeSymbol type, out string genericType) return null; } - private List GetMembersToValidate(ITypeSymbol modelType, bool speculate) + private List GetMembersToValidate(ITypeSymbol modelType, bool speculate, Location lowerLocationInCompilation) { // make a list of the most derived members in the model type @@ -263,7 +267,11 @@ private List GetMembersToValidate(ITypeSymbol modelType, bool s var membersToValidate = new List(); 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) @@ -279,7 +287,7 @@ private List 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) @@ -362,7 +370,7 @@ private List GetMembersToValidate(ITypeSymbol modelType, bool s if (transValidatorTypeName == null) { transValidatorIsSynthetic = true; - transValidatorTypeName = AddSynthesizedValidator(memberType, member); + transValidatorTypeName = AddSynthesizedValidator(memberType, member, location); } // pop the stack @@ -425,7 +433,7 @@ private List GetMembersToValidate(ITypeSymbol modelType, bool s if (enumerationValidatorTypeName == null) { enumerationValidatorIsSynthetic = true; - enumerationValidatorTypeName = AddSynthesizedValidator(enumeratedType, member); + enumerationValidatorTypeName = AddSynthesizedValidator(enumeratedType, member, location); } // pop the stack @@ -455,7 +463,7 @@ private List 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 @@ -467,10 +475,10 @@ private List 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); } } } @@ -483,10 +491,10 @@ private List 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); } } } @@ -511,7 +519,7 @@ private List 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) @@ -525,11 +533,11 @@ private List 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; } @@ -665,22 +673,10 @@ private static string EscapeString(string s) return sb.ToString(); } - private void Diag(DiagnosticDescriptor desc, Location? location) - { - // ignore the diagnostics outside the current compilation - if (location?.Kind == LocationKind.SourceFile) - { - _reportDiagnostic(Diagnostic.Create(desc, location, Array.Empty())); - } - } + private void Diag(DiagnosticDescriptor desc, Location? location) => + _reportDiagnostic(Diagnostic.Create(desc, location, Array.Empty())); - private void Diag(DiagnosticDescriptor desc, Location? location, params object?[]? messageArgs) - { - // ignore the diagnostics outside the current compilation - if (location?.Kind == LocationKind.SourceFile) - { - _reportDiagnostic(Diagnostic.Create(desc, location, messageArgs)); - } - } + private void Diag(DiagnosticDescriptor desc, Location? location, params object?[]? messageArgs) => + _reportDiagnostic(Diagnostic.Create(desc, location, messageArgs)); } } diff --git a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs index fb8d88feef70ec..869cb7715b1293 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs @@ -1194,8 +1194,8 @@ public class ClassInAnotherAssembly [Required] public string? Foo { get; set; } - // line below causes the generator to emit a warning "SYSLIB1212" outside of its compilation. - // The generator should not emit this diagnostics in this case. + // 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; } } @@ -1253,8 +1253,12 @@ public class MyOptions new List { source1 }) .ConfigureAwait(false); - Assert.Empty(diagnostics); // no diagnostics should be produced + Assert.Equal(1, diagnostics.Count()); + Assert.Equal(DiagDescriptors.PotentiallyMissingTransitiveValidation.Id, diagnostics[0].Id); _ = Assert.Single(generatedSources); + + // 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,", diagnostics[0].Location.GetLineSpan().ToString()); }, assemblyPath).Dispose(); File.Delete(assemblyPath); // cleanup From d485ae50593417584abd87ba7edd34157b0e287d Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 7 Jul 2023 09:21:36 -0700 Subject: [PATCH 3/3] Address the feedback --- .../tests/SourceGeneration.Unit.Tests/Main.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs index 869cb7715b1293..17ea4bca02cb07 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Main.cs @@ -1253,12 +1253,12 @@ public class MyOptions new List { source1 }) .ConfigureAwait(false); - Assert.Equal(1, diagnostics.Count()); - Assert.Equal(DiagDescriptors.PotentiallyMissingTransitiveValidation.Id, diagnostics[0].Id); _ = 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,", diagnostics[0].Location.GetLineSpan().ToString()); + Assert.StartsWith("src-0.cs: (12,", diag.Location.GetLineSpan().ToString()); }, assemblyPath).Dispose(); File.Delete(assemblyPath); // cleanup