From 66ffd6fde381f7008448a6ff55be45c0f13d4fc7 Mon Sep 17 00:00:00 2001 From: Nikola Irinchev Date: Wed, 4 Oct 2023 18:01:30 +0200 Subject: [PATCH] Emit errors for collection assignments (#3456) --- CHANGELOG.md | 1 + .../Realm.PlatformHelpers.csproj | 1 + .../Realm.SourceGenerator/ClassCodeBuilder.cs | 2 +- Realm/Realm.SourceGenerator/Diagnostics.cs | 22 +++++++ Realm/Realm.SourceGenerator/Parser.cs | 50 +++++++++++++++- .../ComparisonTests.cs | 60 ++++++++++--------- .../SourceGenerationTest.cs | 23 ++++--- .../ErrorClasses/CollectionErrors.cs | 31 ++++++++-- .../CollectionErrors.diagnostics.cs | 46 ++++++++++---- 9 files changed, 173 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac111eeb7c..ef47bc7ec2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ realm.custom_ignore_attribute = [System.Text.Json.Serialization.JsonIgnore] ``` (Issue [#2579](https://github.com/realm/realm-dotnet/issues/2579)) +* The Realm source generator will now error out in case a collection in the model classes is assigned to a non-null value either in a property initializer or in a constructor. Realm collections are initialized internally and assigning non-null values to the property is not supported, where the `null!` assignment is only useful to silence nullable reference type warnings, in reality the collection will never be null. (Issue [#3455](https://github.com/realm/realm-dotnet/issues/3455)) ### Fixed * None diff --git a/Realm/Realm.PlatformHelpers/Realm.PlatformHelpers.csproj b/Realm/Realm.PlatformHelpers/Realm.PlatformHelpers.csproj index e87158ac68..9232c21ff6 100644 --- a/Realm/Realm.PlatformHelpers/Realm.PlatformHelpers.csproj +++ b/Realm/Realm.PlatformHelpers/Realm.PlatformHelpers.csproj @@ -22,6 +22,7 @@ 9.0 enable Realms.PlatformHelpers + NETSDK1202 diff --git a/Realm/Realm.SourceGenerator/ClassCodeBuilder.cs b/Realm/Realm.SourceGenerator/ClassCodeBuilder.cs index 51d4d84ffc..cf61598d09 100644 --- a/Realm/Realm.SourceGenerator/ClassCodeBuilder.cs +++ b/Realm/Realm.SourceGenerator/ClassCodeBuilder.cs @@ -847,7 +847,7 @@ private string GenerateManagedAccessor() } else { - var forceNotNullable = type == "string" || type == "byte[]" ? "!" : string.Empty; + var forceNotNullable = type is "string" or "byte[]" ? "!" : string.Empty; var getterString = $@"get => ({type})GetValue(""{stringName}""){forceNotNullable};"; diff --git a/Realm/Realm.SourceGenerator/Diagnostics.cs b/Realm/Realm.SourceGenerator/Diagnostics.cs index 3695895d49..b321d5b754 100644 --- a/Realm/Realm.SourceGenerator/Diagnostics.cs +++ b/Realm/Realm.SourceGenerator/Diagnostics.cs @@ -51,6 +51,8 @@ private enum Id RealmObjectWithoutAutomaticProperty = 25, ParentOfNestedClassIsNotPartial = 27, IndexedPrimaryKey = 28, + InvalidCollectionInitializer = 29, + InvalidCollectionInitializerInCtor = 30, InvalidGeneratorConfiguration = 1000, } @@ -318,6 +320,26 @@ public static Diagnostic ParentOfNestedClassIsNotPartial(string className, strin location); } + public static Diagnostic InvalidCollectionInitializer(string className, string propertyName, Location location) + { + return CreateDiagnosticError( + Id.InvalidCollectionInitializer, + "Invalid collection initializer", + $"{className}.{propertyName} is a collection with an initializer that is not supported. Realm collections are always initialized internally and initializing them to a non-null value is not supported.", + location, + description: $"Either remove the initializer or replace it with '= null!' to silence the 'Non-nullable field '{propertyName}' is uninitialized' error."); + } + + public static Diagnostic InvalidCollectionInitializerInCtor(string className, string propertyName, Location location) + { + return CreateDiagnosticError( + Id.InvalidCollectionInitializerInCtor, + "Invalid collection initializer in constructor", + $"{className}.{propertyName} is a collection that is initialized in a constructor. Realm collections are always initialized internally and initializing them to a non-null value is not supported.", + location, + description: $"Either remove the initializer or replace it with '= null!' to silence the 'Non-nullable field '{propertyName}' is uninitialized' error."); + } + #endregion #region Warnings diff --git a/Realm/Realm.SourceGenerator/Parser.cs b/Realm/Realm.SourceGenerator/Parser.cs index b419854a9f..8def84a5b4 100644 --- a/Realm/Realm.SourceGenerator/Parser.cs +++ b/Realm/Realm.SourceGenerator/Parser.cs @@ -19,6 +19,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Text.RegularExpressions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; @@ -27,6 +28,9 @@ namespace Realms.SourceGenerator { internal class Parser { + private static readonly Regex _equalsNullRegex = new(@"\s*=\s*null!?"); + private static readonly Regex _nullRegex = new(@"^null!?$"); + private readonly GeneratorExecutionContext _context; private readonly GeneratorConfig _generatorConfig; @@ -112,6 +116,8 @@ public ParsingResults Parse(IEnumerable realmClasses) classInfo.Diagnostics.Add(Diagnostics.MultiplePrimaryKeys(classInfo.Name, firstClassDeclarationSyntax.GetIdentifierLocation())); } + ValidateConstructorBody(classInfo, classDeclarations); + result.ClassInfo.Add(classInfo); } catch (Exception ex) @@ -269,6 +275,11 @@ private IEnumerable GetProperties(ClassInfo classInfo, IEnumerable } } + if (info.TypeInfo.IsCollection && !string.IsNullOrEmpty(info.Initializer) && !_equalsNullRegex.IsMatch(info.Initializer!)) + { + classInfo.Diagnostics.Add(Diagnostics.InvalidCollectionInitializer(classInfo.Name, info.Name, propSyntax.GetLocation())); + } + yield return info; } } @@ -461,12 +472,45 @@ INamedTypeSymbol when typeSymbol.IsAnyRealmObjectType() => PropertyTypeInfo.Obje return propInfo; } - private static bool HasParameterlessConstructor(List classDeclarations) + private static bool HasParameterlessConstructor(IEnumerable classDeclarations) { var constructors = classDeclarations.SelectMany(cd => cd.ChildNodes().OfType()).ToArray(); return !constructors.Any() || constructors.Any(c => !c.ParameterList.Parameters.Any()); } + private static void ValidateConstructorBody(ClassInfo classInfo, IEnumerable classDeclarations) + { + // This finds all assignment expressions in the ctor body + var assignments = classDeclarations.SelectMany(cd => cd.ChildNodes().OfType()) + .Where(c => c.Body != null) + .SelectMany(c => c.Body!.Statements) + .OfType() + .Select(e => e.Expression) + .OfType(); + + foreach (var assignment in assignments) + { + if (assignment.Left is not IdentifierNameSyntax leftIdentifier) + { + continue; + } + + // If the assignment is to `null`, we ignore it + if (_nullRegex.IsMatch(assignment.Right.ToString())) + { + continue; + } + + // If the left operand of the assignment is a collection property, we should report an error as it's not valid to + // initialize a collection to something other than null. + var initializedProp = classInfo.Properties.FirstOrDefault(p => p.TypeInfo.IsCollection && p.Name == leftIdentifier.Identifier.ValueText); + if (initializedProp != null) + { + classInfo.Diagnostics.Add(Diagnostics.InvalidCollectionInitializerInCtor(classInfo.Name, initializedProp.Name, assignment.GetLocation())); + } + } + } + private static IList GetEnclosingClassList(ClassInfo classInfo, ITypeSymbol classSymbol, ClassDeclarationSyntax classDeclarationSyntax) { var enclosingClassList = new List(); @@ -480,8 +524,8 @@ private static IList GetEnclosingClassList(ClassInfo classIn classInfo.Diagnostics.Add(Diagnostics.ParentOfNestedClassIsNotPartial(classSymbol.Name, ts.Name, cs.GetIdentifierLocation())); } - var enclosingClassinfo = new EnclosingClassInfo(ts.Name, ts.DeclaredAccessibility); - enclosingClassList.Add(enclosingClassinfo); + var enclosingClassInfo = new EnclosingClassInfo(ts.Name, ts.DeclaredAccessibility); + enclosingClassList.Add(enclosingClassInfo); currentSymbol = ts; currentClassDeclaration = cs; diff --git a/Tests/SourceGenerators/Realm.SourceGenerator.Tests/ComparisonTests.cs b/Tests/SourceGenerators/Realm.SourceGenerator.Tests/ComparisonTests.cs index 162a546d37..c9d79740b7 100644 --- a/Tests/SourceGenerators/Realm.SourceGenerator.Tests/ComparisonTests.cs +++ b/Tests/SourceGenerators/Realm.SourceGenerator.Tests/ComparisonTests.cs @@ -16,6 +16,7 @@ // //////////////////////////////////////////////////////////////////////////// +using System.Text.RegularExpressions; using Realms.SourceGenerator; using RealmGeneratorVerifier = SourceGeneratorTests.CSharpSourceGeneratorVerifier; @@ -24,38 +25,26 @@ namespace SourceGeneratorTests [TestFixture] internal class ComparisonTests : SourceGenerationTest { - [TestCase("AllTypesClass")] - [TestCase("ClassWithoutParameterlessConstructor")] - [TestCase("DifferentNamespaces", "NamespaceObj", "OtherNamespaceObj")] - [TestCase("NoNamespaceClass")] - [TestCase("PartialClass")] - [TestCase("AutomaticPropertiesClass")] - [TestCase("ConfusingNamespaceClass")] - [TestCase("InitializerNamespaceClass")] - [TestCase("NestedClass")] - [TestCase("NullableClass")] - [TestCase("PersonWithDog", "Person", "Dog")] - [TestCase("IndexedClass")] - public async Task ComparisonTest(string filename, params string[] classNames) + public record ComparisonTestInfo(string File, string[] ClassNames) { - await RunComparisonTest(filename, classNames); + public override string ToString() => File; } - [TestCase("ClassWithBaseType")] - [TestCase("MultiplePrimaryKeys")] - [TestCase("NoPartialClass")] - [TestCase("RealmintegerErrors")] - [TestCase("RealmObjectAndEmbeddedObjectClass")] - [TestCase("UnsupportedIndexableTypes")] - [TestCase("UnsupportedPrimaryKeyTypes")] - [TestCase("UnsupportedRequiredTypes")] - [TestCase("NestedClassWithoutPartialParent")] - [TestCase("NullableErrorClass")] - [TestCase("IgnoreObjectNullabilityClass")] - [TestCase("UnsupportedBacklink", "UnsupportedBacklink", "BacklinkObj")] - public async Task ErrorComparisonTest(string filename, params string[] classNames) + private static readonly Regex _classNameRegex = new(@"class (?[^\s]*) :.*I(Realm|Embedded|Asymmetric)Object"); + + public static ComparisonTestInfo[] SuccessTestCases => GetTestInfos(_testClassesPath); + public static ComparisonTestInfo[] ErrorTestCases => GetTestInfos(_errorClassesPath); + + [TestCaseSource(nameof(SuccessTestCases))] + public async Task ComparisonTest(ComparisonTestInfo testCase) { - await RunErrorTest(filename, classNames); + await RunComparisonTest(testCase.File, testCase.ClassNames); + } + + [TestCaseSource(nameof(ErrorTestCases))] + public async Task ErrorComparisonTest(ComparisonTestInfo testCase) + { + await RunErrorTest(testCase.File, testCase.ClassNames); } [Test] @@ -79,5 +68,20 @@ public async Task IgnoreObjectNullabilityTest() await test.RunAsync(); } + + private static ComparisonTestInfo[] GetTestInfos(string folder) + => Directory.GetFiles(folder) + .Select(f => + { + var filename = Path.GetFileNameWithoutExtension(f); + var content = File.ReadAllText(f); + var classNames = _classNameRegex.Matches(content) + .Select(m => m.Groups["ClassName"].Value) + .Distinct() + .ToArray(); + + return new ComparisonTestInfo(filename, classNames); + }) + .ToArray(); } } diff --git a/Tests/SourceGenerators/Realm.SourceGenerator.Tests/SourceGenerationTest.cs b/Tests/SourceGenerators/Realm.SourceGenerator.Tests/SourceGenerationTest.cs index 015e4a3347..1d29e4cbb8 100644 --- a/Tests/SourceGenerators/Realm.SourceGenerator.Tests/SourceGenerationTest.cs +++ b/Tests/SourceGenerators/Realm.SourceGenerator.Tests/SourceGenerationTest.cs @@ -25,12 +25,17 @@ namespace SourceGeneratorTests { internal abstract class SourceGenerationTest { - private string _testClassesPath; - private string _errorClassesPath; - private string _supportClassesPath; - private string _generatedFilesPath; + private static readonly string _buildFolder = Path.GetDirectoryName(typeof(SourceGenerationTest).Assembly.Location)!; + private static readonly string _testFolder = _buildFolder.Substring(0, _buildFolder.IndexOf("Realm.SourceGenerator.Test", StringComparison.InvariantCulture)); + private static readonly string _assemblyToProcessFolder = Path.Combine(_testFolder, "SourceGeneratorAssemblyToProcess"); - private string[] _supportClasses = new[] { + protected static readonly string _testClassesPath = Path.Combine(_assemblyToProcessFolder, "TestClasses"); + protected static readonly string _errorClassesPath = Path.Combine(_assemblyToProcessFolder, "ErrorClasses"); + private static readonly string _supportClassesPath = Path.Combine(_assemblyToProcessFolder, "SupportClasses"); + private static readonly string _generatedFilesPath = Path.Combine(_assemblyToProcessFolder, "Generated", + "Realm.SourceGenerator", "Realms.SourceGenerator.RealmGenerator"); + + private readonly string[] _supportClasses = { "RealmObj", "EmbeddedObj", }; @@ -38,14 +43,6 @@ internal abstract class SourceGenerationTest [OneTimeSetUp] public void Setup() { - var buildFolder = Path.GetDirectoryName(typeof(SourceGenerationTest).Assembly.Location)!; - var testFolder = buildFolder.Substring(0, buildFolder.IndexOf("Realm.SourceGenerator.Test", StringComparison.InvariantCulture)); - var assemblyToProcessFolder = Path.Combine(testFolder, "SourceGeneratorAssemblyToProcess"); - _testClassesPath = Path.Combine(assemblyToProcessFolder, "TestClasses"); - _errorClassesPath = Path.Combine(assemblyToProcessFolder, "ErrorClasses"); - _supportClassesPath = Path.Combine(assemblyToProcessFolder, "SupportClasses"); - _generatedFilesPath = Path.Combine(assemblyToProcessFolder, "Generated", - "Realm.SourceGenerator", "Realms.SourceGenerator.RealmGenerator"); Environment.SetEnvironmentVariable("NO_GENERATOR_DIAGNOSTICS", "true"); } diff --git a/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/ErrorClasses/CollectionErrors.cs b/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/ErrorClasses/CollectionErrors.cs index ac5bdb129c..24e437aa84 100644 --- a/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/ErrorClasses/CollectionErrors.cs +++ b/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/ErrorClasses/CollectionErrors.cs @@ -18,9 +18,6 @@ using System; using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; using Realms; namespace SourceGeneratorPlayground @@ -28,15 +25,37 @@ namespace SourceGeneratorPlayground public partial class CollectionErrors : IRealmObject { public IDictionary UnsupportetDictionaryKeyProp { get; } - + public ISet SetOfEmbeddedObj { get; } - + public IList CollectionWithSetter { get; set; } public IList> CollectionOfRealmInteger { get; } public IList CollectionOfUnsupportedType { get; } - + public List ListInsteadOfIList { get; } + + public IList CollectionWithInitializer { get; } = new List + { + 1, + 2, + 3 + }; + + public IList CollectionWithCtorInitializer { get; } + + // This should not generate error as it's initialized to null + public IList ValidCollectionInitializer { get; } = null!; + + public IList ValidCollectionInitializerInCtor { get; } + + public CollectionErrors() + { + CollectionWithCtorInitializer = new List(); + + // This should not generate error as it's initialized to null + ValidCollectionInitializerInCtor = null!; + } } } diff --git a/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/Generated/Realm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/CollectionErrors.diagnostics.cs b/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/Generated/Realm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/CollectionErrors.diagnostics.cs index 1fecafaa44..7ce41a2723 100644 --- a/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/Generated/Realm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/CollectionErrors.diagnostics.cs +++ b/Tests/SourceGenerators/SourceGeneratorAssemblyToProcess/Generated/Realm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/CollectionErrors.diagnostics.cs @@ -4,9 +4,9 @@ "Severity": 3, "Message": "CollectionErrors.UnsupportetDictionaryKeyProp is a Dictionary but only string keys are currently supported by Realm.", "Location": { - "StartLine": 30, + "StartLine": 27, "StartColumn": 9, - "EndLine": 30, + "EndLine": 27, "EndColumn": 78 } }, @@ -15,9 +15,9 @@ "Severity": 3, "Message": "CollectionErrors.SetOfEmbeddedObj is a Set which is not supported. Embedded objects are always unique which is why List already has Set semantics.", "Location": { - "StartLine": 32, + "StartLine": 29, "StartColumn": 9, - "EndLine": 32, + "EndLine": 29, "EndColumn": 59 } }, @@ -26,9 +26,9 @@ "Severity": 3, "Message": "CollectionErrors.CollectionWithSetter has a setter but its type is a List which only supports getters.", "Location": { - "StartLine": 34, + "StartLine": 31, "StartColumn": 9, - "EndLine": 34, + "EndLine": 31, "EndColumn": 61 } }, @@ -37,9 +37,9 @@ "Severity": 3, "Message": "CollectionErrors.CollectionOfRealmInteger is an List which is not supported.", "Location": { - "StartLine": 36, + "StartLine": 33, "StartColumn": 9, - "EndLine": 36, + "EndLine": 33, "EndColumn": 74 } }, @@ -48,9 +48,9 @@ "Severity": 3, "Message": "CollectionErrors.CollectionOfUnsupportedType is an List but its generic type is System.DateTime which is not supported by Realm.", "Location": { - "StartLine": 38, + "StartLine": 35, "StartColumn": 9, - "EndLine": 38, + "EndLine": 35, "EndColumn": 68 } }, @@ -59,10 +59,32 @@ "Severity": 3, "Message": "CollectionErrors.ListInsteadOfIList is declared as List which is not the correct way to declare to-many relationships in Realm. If you want to persist the collection, use the interface IList, otherwise annotate the property with the [Ignored] attribute.", "Location": { - "StartLine": 40, + "StartLine": 37, "StartColumn": 9, - "EndLine": 40, + "EndLine": 37, "EndColumn": 53 } + }, + { + "Id": "RLM029", + "Severity": 3, + "Message": "CollectionErrors.CollectionWithInitializer is a collection with an initializer that is not supported. Realm collections are always initialized internally and initializing them to a non-null value is not supported.", + "Location": { + "StartLine": 39, + "StartColumn": 9, + "EndLine": 44, + "EndColumn": 11 + } + }, + { + "Id": "RLM030", + "Severity": 3, + "Message": "CollectionErrors.CollectionWithCtorInitializer is a collection that is initialized in a constructor. Realm collections are always initialized internally and initializing them to a non-null value is not supported.", + "Location": { + "StartLine": 55, + "StartColumn": 13, + "EndLine": 55, + "EndColumn": 63 + } } ] \ No newline at end of file