diff --git a/CHANGELOG.md b/CHANGELOG.md index 556f85e4..28b18801 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,9 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add an editor validation engine: implement `IValidationRule` and run it across the whole project a few milliseconds per editor tick instead of freezing for thirty seconds. Only the assets a rule claims are loaded. See [Asset Validation](./docs/features/editor-tools/asset-validation.md) ([#288](https://github.com/Ambiguous-Interactive/unity-helpers/issues/288)). -- Add `[WProtoSubtype(typeof(Base))]`, so a subtype joins a WallstopProto hierarchy without picking a field number. The editor assigns and commits the number after the reload that first sees it, and **Assign WallstopProto Subtype Tags** retires a removed one so it is never reused. See [Polymorphism](./docs/features/serialization/serialization.md#polymorphism) ([#587](https://github.com/Ambiguous-Interactive/unity-helpers/issues/587), [#601](https://github.com/Ambiguous-Interactive/unity-helpers/issues/601)). +- Add an editor validation engine: implement `IValidationRule` and run it across the whole project a few milliseconds per tick, not one thirty-second freeze. Only claimed assets load, and one `-executeMethod` runs it in CI with a JSON report and a reviewable suppression file. See [Asset Validation](./docs/features/editor-tools/asset-validation.md) ([#288](https://github.com/Ambiguous-Interactive/unity-helpers/issues/288)). +- Add `[WProtoSubtype(typeof(Base))]`, so a subtype joins a WallstopProto hierarchy without picking a field number. The editor assigns and commits the number on the next reload, and **Assign WallstopProto Subtype Tags** retires a removed one -- hand-numbered or not -- so it is never reused. See [Polymorphism](./docs/features/serialization/serialization.md#polymorphism) ([#587](https://github.com/Ambiguous-Interactive/unity-helpers/issues/587), [#601](https://github.com/Ambiguous-Interactive/unity-helpers/issues/601), [#606](https://github.com/Ambiguous-Interactive/unity-helpers/issues/606)). - Add `Sfc64Random`, the Small Fast Chaotic generator: a published-pedigree 64-bit generator with a very small hot path that answers `NextUlong` in one state advance. See [Random Generators](./docs/features/utilities/random-generators.md) ([#516](https://github.com/Ambiguous-Interactive/unity-helpers/issues/516)). +- Add `[WProtoReserved]`, which records a field number or name a removed `[WProtoMember]` held. Taking one again is a build error rather than a save that reads back as the wrong thing, and the exported schema carries the matching proto3 `reserved` lines. See [Retiring a member](./docs/features/serialization/serialization.md#retiring-a-member) ([#608](https://github.com/Ambiguous-Interactive/unity-helpers/issues/608)). - Add a proto schema exporter: **Tools > Wallstop Studios > Unity Helpers > Proto Schema Exporter** writes `proto3` for your `[WProtoContract]` types, so anything downstream can read your saves. Search and tick the exact types, name a package, and write one file or one per assembly, namespace or type ([#424](https://github.com/Ambiguous-Interactive/unity-helpers/issues/424), [#595](https://github.com/Ambiguous-Interactive/unity-helpers/issues/595)). - Add strict UTF-8 validation to WallstopProto strings and Uri: wire bytes that are not valid UTF-8 refuse the payload as malformed instead of decoding to replacement characters, as proto3 requires ([#580](https://github.com/Ambiguous-Interactive/unity-helpers/issues/580)). - Add `IntMap`, an int-keyed open-addressing map measured at 1.26x–2.19x `Dictionary` on hit-heavy lookups, with no comparer indirection on the lookup path. See [Data Structures](./docs/features/utilities/data-structures.md#intmap-int-keyed-open-addressing-map) ([#578](https://github.com/Ambiguous-Interactive/unity-helpers/issues/578)). @@ -149,6 +150,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fix two documentation examples that could not work as printed: the enum display-name sample imported `Core.Attribute` rather than `Core.Attributes`, and the `link.xml` sample preserved an assembly name that does not exist, which strips silently ([#441](https://github.com/Ambiguous-Interactive/unity-helpers/issues/441)). - Fix fifteen broken documentation links on `AssetDatabaseBatchScope`: its `` references to `AssetDatabase.Refresh`, `CreateAsset` and `ImportAsset` named an ambiguous overload or an unresolvable type, so an IDE linked the wrong overload or nothing ([#594](https://github.com/Ambiguous-Interactive/unity-helpers/issues/594)). - Fix the IntelliSense tooltip on ten public `ReflectionHelpers` delegate factories, which carried two `` tags and showed the vaguer one ([#441](https://github.com/Ambiguous-Interactive/unity-helpers/issues/441)). - Fix zero-valued `ValueTuple` components and fixed-width map keys being omitted by WallstopProto where protobuf-net writes them explicitly, including enum tuple map keys ([#399](https://github.com/Ambiguous-Interactive/unity-helpers/issues/399)). diff --git a/Editor/Tools/WProtoSubtypeTagAssigner.cs b/Editor/Tools/WProtoSubtypeTagAssigner.cs index e65fc472..8b888f56 100644 --- a/Editor/Tools/WProtoSubtypeTagAssigner.cs +++ b/Editor/Tools/WProtoSubtypeTagAssigner.cs @@ -312,6 +312,28 @@ WProtoIncludeAttribute include in baseType.GetCustomAttributes( + false + ) + ) + { + foreach (int fieldNumber in held.FieldNumbers) + { + inventory.Reserved.Add( + new WProtoSubtypeTagPlan.Entry( + "[WProtoReserved]", + baseName, + fieldNumber + ) + ); + } + } + const BindingFlags Declared = BindingFlags.Public | BindingFlags.NonPublic diff --git a/Editor/Tools/WProtoSubtypeTagPlan.cs b/Editor/Tools/WProtoSubtypeTagPlan.cs index bf29f704..bf8ed756 100644 --- a/Editor/Tools/WProtoSubtypeTagPlan.cs +++ b/Editor/Tools/WProtoSubtypeTagPlan.cs @@ -129,6 +129,7 @@ WProtoSubtypeTagDiscovery discovery ) { List tagless = new List(); + List pinned = new List(); Dictionary> taken = new Dictionary>( StringComparer.Ordinal ); @@ -146,9 +147,14 @@ WProtoSubtypeTagDiscovery discovery if (declaration.HasTag) { + string pinnedKey = PairKey(declaration.SubTypeName, declaration.BaseTypeName); Claim(taken, declaration.BaseTypeName, declaration.Tag); - explicitTags[PairKey(declaration.SubTypeName, declaration.BaseTypeName)] = - declaration.Tag; + if (!explicitTags.ContainsKey(pinnedKey)) + { + explicitTags[pinnedKey] = declaration.Tag; + pinned.Add(declaration); + } + continue; } @@ -169,6 +175,9 @@ WProtoSubtypeTagDiscovery discovery Dictionary retiredByPair = new Dictionary( StringComparer.Ordinal ); + Dictionary allRetired = new Dictionary( + StringComparer.Ordinal + ); foreach (Entry entry in Safe(retired)) { if (!entry.IsUsable) @@ -182,6 +191,12 @@ WProtoSubtypeTagDiscovery discovery { retiredByPair[key] = entry; } + + // Keyed by pair AND number. retiredByPair keeps one entry per pair, which is all a + // restore needs and is NOT enough to re-emit: a pair that retired two numbers -- + // what a hand-edited number leaves behind -- lost one of them on the next run, and + // a dropped retirement is a number that is free again a run later. + allRetired[RetirementKey(entry)] = entry; } List assignments = new List(); @@ -190,7 +205,11 @@ WProtoSubtypeTagDiscovery discovery StringComparer.Ordinal ); HashSet keptPairs = new HashSet(StringComparer.Ordinal); - HashSet restoredPairs = new HashSet(StringComparer.Ordinal); + // Keyed by pair AND number, not by pair. A pair can hold more than one retirement -- a + // hand-edited number leaves one and a later deletion leaves another -- and re-adding + // the type under the first would otherwise free the second, which is the exact reuse + // the record exists to forbid. + HashSet restoredRetirements = new HashSet(StringComparer.Ordinal); foreach (Entry entry in Safe(existing)) { @@ -216,15 +235,27 @@ WProtoSubtypeTagDiscovery discovery continue; } - // The subtype pinned its own number and pinned the same one, so the manifest entry - // is simply redundant. Retiring it would forbid the very declaration that now holds - // it, and the next build would refuse a hierarchy that changed in no way at all. - if ( - explicitTags.TryGetValue(key, out int pinned) - && pinned == entry.Tag - && !retiredByPair.ContainsKey(key) - ) + // A number written by hand is as durable a wire contract as one this tool + // assigned, and until it was recorded here the only trace that the number had ever + // been spent was the declaration itself -- which is deleted along with the type it + // sits on. Keeping the entry is what turns that deletion into a retirement (#606). + if (explicitTags.TryGetValue(key, out int pinnedTag)) { + Claim(taken, entry.BaseTypeName, pinnedTag); + assignments.Add( + pinnedTag == entry.Tag + ? entry + : new Entry(entry.SubTypeName, entry.BaseTypeName, pinnedTag) + ); + + // Editing a shipped number in place is the one thing the guidance forbids, and + // it used to leave no trace at all. The number it left still means this type to + // every payload written under it. + if (pinnedTag != entry.Tag) + { + retirements[RetirementKey(entry)] = entry; + } + continue; } @@ -245,6 +276,33 @@ WProtoSubtypeTagDiscovery discovery retirements[RetirementKey(entry)] = entry; } + // Before the tag-less passes, because an explicit number is stated by the source and + // needs neither restoring nor inventing -- it only needs recording. + pinned.Sort(CompareDeclarations); + foreach (Declaration declaration in pinned) + { + string key = PairKey(declaration.SubTypeName, declaration.BaseTypeName); + if (!keptPairs.Add(key)) + { + continue; + } + + assignments.Add( + new Entry(declaration.SubTypeName, declaration.BaseTypeName, declaration.Tag) + ); + + // Remove-then-re-add for the explicit form: the type is back under the number it + // held, so the retirement that was standing in for it is lifted rather than left to + // forbid the very declaration now holding it. + if ( + retiredByPair.TryGetValue(key, out Entry wasRetired) + && wasRetired.Tag == declaration.Tag + ) + { + restoredRetirements.Add(RetirementKey(wasRetired)); + } + } + foreach (Entry entry in retiredByPair.Values) { string key = PairKey(entry.SubTypeName, entry.BaseTypeName); @@ -256,7 +314,7 @@ WProtoSubtypeTagDiscovery discovery // Remove-then-re-add, which is the case the whole design exists for: the number the // type had is still held for it, so it comes back rather than being handed out. assignments.Add(entry); - restoredPairs.Add(key); + restoredRetirements.Add(RetirementKey(entry)); keptPairs.Add(key); } @@ -284,9 +342,9 @@ WProtoSubtypeTagDiscovery discovery fresh.Add(assignment); } - foreach (Entry entry in retiredByPair.Values) + foreach (Entry entry in allRetired.Values) { - if (!restoredPairs.Contains(PairKey(entry.SubTypeName, entry.BaseTypeName))) + if (!restoredRetirements.Contains(RetirementKey(entry))) { retirements[RetirementKey(entry)] = entry; } @@ -328,15 +386,21 @@ public string Render(string assemblyName) "// Subtype Tags. Commit it: these numbers are the wire contract for every\r\n" ); builder.Append( - "// [WProtoSubtype] declared without one, so a payload saved today is read back by\r\n" + "// [WProtoSubtype] in this assembly, so a payload saved today is read back by\r\n" + ); + builder.Append( + "// this file. A subtype that wrote its own number is recorded here too, because\r\n" + ); + builder.Append( + "// deleting the type deletes the only other record that the number was spent.\r\n" ); builder.Append( - "// this file. Do not renumber an entry, and do not delete a retired one -- a\r\n" + "// Do not renumber an entry, and do not delete a retired one -- a retired number\r\n" ); builder.Append( - "// retired number is held so a later subtype cannot be given a number old saves\r\n" + "// is held so a later subtype cannot be given a number old saves already mean\r\n" ); - builder.Append("// already mean something else by.\r\n"); + builder.Append("// something else by.\r\n"); builder.Append( "//\r\n// The editor rewrites this file automatically after an assembly reload that finds a\r\n" ); diff --git a/Editor/Validation/Continuous/ValidationBatch.cs b/Editor/Validation/Continuous/ValidationBatch.cs new file mode 100644 index 00000000..e02120c1 --- /dev/null +++ b/Editor/Validation/Continuous/ValidationBatch.cs @@ -0,0 +1,397 @@ +// MIT License - Copyright (c) 2026 wallstop +// Full license text: https://github.com/wallstop/unity-helpers/blob/main/LICENSE + +namespace WallstopStudios.UnityHelpers.Editor.Validation.Continuous +{ +#if UNITY_EDITOR + using System; + using System.Collections.Generic; + using System.IO; + using UnityEditor; + using UnityEngine; + + /// + /// Runs every in the project from the command line and reports + /// what it found, so a build can refuse to publish an asset nobody would have looked at. + /// + /// + /// + /// The whole point of the engine is continuous, lightweight checks. That only becomes a + /// guarantee when something other than a person is running it, which is what this is for: + /// + /// + /// Unity -batchmode -quit -projectPath <project> \ + /// -executeMethod WallstopStudios.UnityHelpers.Editor.Validation.Continuous.ValidationBatch.ValidateFromCommandLine \ + /// -validationOutput validation.json -validationFailOn Warning + /// + /// + /// It exits non-zero when anything at or above the threshold stands unsuppressed, and when any + /// rule threw -- a rule that threw produced no answer for that asset, so passing on it would be + /// reporting coverage the run does not have. + /// + /// + /// Rules are found through TypeCache and constructed with their parameterless + /// constructor. A rule that cannot be constructed is reported and skipped rather than ending + /// the run, because the alternative is one broken rule hiding every other rule's findings. + /// + /// + public static class ValidationBatch + { + /// The argument naming where the JSON report is written. + public const string OutputArgument = "-validationOutput"; + + /// The argument naming the suppression file to read. + public const string SuppressionsArgument = "-validationSuppressions"; + + /// The argument naming the lowest severity that fails the run. + public const string FailOnArgument = "-validationFailOn"; + + /// The argument naming a folder to restrict the run to; repeatable. + public const string FolderArgument = "-validationFolder"; + + /// + /// Validates the project and exits with 0 when nothing blocking stands. + /// + public static void ValidateFromCommandLine() + { + Result result = Run(Environment.GetCommandLineArgs()); + Debug.Log(result.Summary); + EditorApplication.Exit(result.ExitCode); + } + + /// + /// Validates the project according to a command line, without exiting. + /// + /// The process arguments; null is treated as none. + /// What happened, including the exit code the caller should use. + /// + /// Separated from so the decision can be made without + /// killing the editor, which is what lets a menu item and a test reach it. + /// + public static Result Run(string[] commandLine) + { + List folders = ValuesOf(commandLine, FolderArgument); + string outputPath = ValueOf(commandLine, OutputArgument); + string suppressionsPath = ValueOf(commandLine, SuppressionsArgument); + ValidationSeverity threshold = ParseSeverity( + ValueOf(commandLine, FailOnArgument), + ValidationSeverity.Error + ); + + List problems = new List(); + List rules = DiscoverRules(problems); + ValidationSuppressions suppressions = ReadSuppressions(suppressionsPath, problems); + + ValidationRun run = new ValidationRun( + rules, + ValidationTargets.Enumerate(folders.ToArray()) + ); + while (!run.Step(double.MaxValue)) { } + + problems.AddRange(CoverageProblems(rules.Count, run.TotalCount, folders)); + + string json = ValidationReport.ToJson(run, suppressions); + if ( + !string.IsNullOrEmpty(outputPath) && !TryWrite(outputPath, json, out string failure) + ) + { + problems.Add(failure); + } + + bool blocking = ValidationReport.HasBlockingResults(run, suppressions, threshold); + return new Result(run, suppressions, json, problems, blocking || 0 < problems.Count); + } + + /// + /// Constructs one instance of every concrete rule the project defines. + /// + /// Receives one line per rule that could not be constructed. + /// The rules, ordered by type name so two runs agree. + public static List DiscoverRules(List problems) + { + List candidates = new List(); + foreach (Type candidate in TypeCache.GetTypesDerivedFrom()) + { + if ( + candidate == null + || candidate.IsAbstract + || candidate.IsInterface + || candidate.ContainsGenericParameters + ) + { + continue; + } + + candidates.Add(candidate); + } + + // TypeCache's order is not a property of the project, and a report whose findings + // arrive in a different order on two machines cannot be diffed. + candidates.Sort( + (left, right) => + string.CompareOrdinal(left.AssemblyQualifiedName, right.AssemblyQualifiedName) + ); + + List rules = new List(); + for (int index = 0; index < candidates.Count; index++) + { + Type candidate = candidates[index]; + try + { + if (Activator.CreateInstance(candidate) is IValidationRule rule) + { + rules.Add(rule); + } + } + catch (Exception exception) + { + // Reported rather than thrown: one rule without a parameterless constructor + // would otherwise hide every other rule's findings, and a silent skip would + // report a clean project that nobody had actually checked. + problems?.Add( + candidate.FullName + " could not be constructed: " + exception.Message + ); + } + } + + return rules; + } + + /// + /// Reports the ways a finished run proves nothing. + /// + /// How many rules the run was given. + /// How many assets it considered. + /// The folders it was restricted to, if any. + /// One line per reason; empty when the run actually measured something. + /// + /// A run that walked nothing, or that had nothing to walk with, is the absence of a + /// measurement rather than a pass -- and it exits 0 unless something says so. Both shapes + /// are reachable without anything looking wrong: a folder argument naming a renamed + /// directory yields no targets and is skipped silently by + /// , and a project that has not written a rule yet + /// yields no rules. Either way the build would report validation passing having checked + /// nothing, which is the shape #556 exists to refuse. + /// + /// Separated from so it can be asserted without an asset database. + /// + internal static List CoverageProblems( + int ruleCount, + int targetCount, + IReadOnlyList folders + ) + { + List problems = new List(); + if (ruleCount <= 0) + { + problems.Add( + "no IValidationRule implementation was found, so this run checked nothing. " + + "Write a rule, or drop this step until there is one." + ); + } + + if (targetCount <= 0) + { + problems.Add( + folders != null && 0 < folders.Count + ? "no assets were found under " + + string.Join(", ", folders) + + ", so this run checked nothing. Check the " + + FolderArgument + + " paths -- a folder that does not exist is skipped silently." + : "no assets were found in the project, so this run checked nothing." + ); + } + + return problems; + } + + private static ValidationSuppressions ReadSuppressions(string path, List problems) + { + if (string.IsNullOrEmpty(path)) + { + return ValidationSuppressions.Empty; + } + + try + { + return ValidationSuppressions.Parse(File.ReadAllText(path)); + } + catch (Exception exception) + { + // A suppression file that was named and could not be read is not the same as none: + // continuing with an empty set would report every already-accepted finding as new, + // and continuing silently would hide that the file was never applied. + problems?.Add(path + " could not be read: " + exception.Message); + return ValidationSuppressions.Empty; + } + } + + private static bool TryWrite(string path, string contents, out string failure) + { + try + { + string directory = Path.GetDirectoryName(Path.GetFullPath(path)); + if (!string.IsNullOrEmpty(directory)) + { + Directory.CreateDirectory(directory); + } + + File.WriteAllText(path, contents); + failure = null; + return true; + } + catch (Exception exception) + { + failure = path + " could not be written: " + exception.Message; + return false; + } + } + + /// + /// Reads a severity name, accepting any casing. + /// + /// What the command line said, or null. + /// What to use when it said nothing usable. + /// The severity. + /// + /// An unrecognized name falls back rather than failing. The fallback is the strictest + /// useful threshold, so a typo cannot quietly turn the gate off. + /// + internal static ValidationSeverity ParseSeverity(string value, ValidationSeverity fallback) + { + if (string.IsNullOrEmpty(value)) + { + return fallback; + } + + foreach ( + ValidationSeverity candidate in new[] + { + ValidationSeverity.Info, + ValidationSeverity.Warning, + ValidationSeverity.Error, + } + ) + { + if (string.Equals(value, candidate.ToString(), StringComparison.OrdinalIgnoreCase)) + { + return candidate; + } + } + + return fallback; + } + + /// Reads the value following a named argument, or null. + /// The process arguments. + /// The argument to look for. + /// The first value given for it. + internal static string ValueOf(string[] commandLine, string name) + { + List values = ValuesOf(commandLine, name); + return values.Count == 0 ? null : values[0]; + } + + /// Reads every value given for a named argument, in order. + /// The process arguments. + /// The argument to look for. + /// The values; empty when the argument was not given. + internal static List ValuesOf(string[] commandLine, string name) + { + List values = new List(); + if (commandLine == null) + { + return values; + } + + for (int index = 0; index + 1 < commandLine.Length; index++) + { + if (string.Equals(commandLine[index], name, StringComparison.Ordinal)) + { + values.Add(commandLine[index + 1]); + } + } + + return values; + } + + /// What a headless validation run decided. + public sealed class Result + { + /// + /// Initializes a new instance of the class. + /// + /// The finished run. + /// What the project silences. + /// The rendered report. + /// Anything that went wrong outside a rule. + /// Whether the caller should exit non-zero. + public Result( + ValidationRun run, + ValidationSuppressions suppressions, + string json, + IReadOnlyList problems, + bool failed + ) + { + Run = run; + Suppressions = suppressions; + Json = json; + Problems = problems ?? Array.Empty(); + Failed = failed; + } + + /// The finished run. + public ValidationRun Run { get; } + + /// What the project silences. + public ValidationSuppressions Suppressions { get; } + + /// The rendered JSON report. + public string Json { get; } + + /// Anything that went wrong outside a rule: an unreadable file, an unbuildable rule. + public IReadOnlyList Problems { get; } + + /// Whether the caller should exit non-zero. + public bool Failed { get; } + + /// The exit code the caller should use. + public int ExitCode => Failed ? 1 : 0; + + /// A one-paragraph account of the run, for the console. + public string Summary + { + get + { + int findings = Run == null ? 0 : Run.Findings.Count; + int failures = Run == null ? 0 : Run.Failures.Count; + int considered = Run == null ? 0 : Run.TotalCount; + int unused = + Suppressions == null || Run == null + ? 0 + : Suppressions.UnusedIn(Run.Findings).Count; + + string text = + "Validation: " + + considered + + " asset(s), " + + findings + + " finding(s), " + + failures + + " failure(s), " + + unused + + " unused suppression(s)."; + for (int index = 0; index < Problems.Count; index++) + { + text += "\n " + Problems[index]; + } + + return text; + } + } + } + } +#endif +} diff --git a/Editor/Validation/Continuous/ValidationBatch.cs.meta b/Editor/Validation/Continuous/ValidationBatch.cs.meta new file mode 100644 index 00000000..d794d896 --- /dev/null +++ b/Editor/Validation/Continuous/ValidationBatch.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 2e5d4735ab259e1b84bcbe86cd991c57 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Editor/Validation/Continuous/ValidationReport.cs b/Editor/Validation/Continuous/ValidationReport.cs new file mode 100644 index 00000000..ec1b1f4e --- /dev/null +++ b/Editor/Validation/Continuous/ValidationReport.cs @@ -0,0 +1,238 @@ +// MIT License - Copyright (c) 2026 wallstop +// Full license text: https://github.com/wallstop/unity-helpers/blob/main/LICENSE + +namespace WallstopStudios.UnityHelpers.Editor.Validation.Continuous +{ +#if UNITY_EDITOR + using System; + using System.Collections.Generic; + using UnityEngine; + + /// + /// A finished rendered as JSON, for a build that has to decide + /// something about it. + /// + /// + /// + /// The shape is flat and every field is a string or a number, because the consumer is a CI step + /// or another tool rather than this assembly. is written into the + /// document so a consumer can tell an older report from a newer one instead of guessing from + /// which fields happen to be present. + /// + /// + /// Suppressed findings are written with suppressed set rather than dropped. A report + /// that silently omitted them would make a suppression file indistinguishable from a project + /// that had no findings, which is the difference somebody reviewing the file needs to see. + /// + /// + /// Rendered through JsonUtility, the same way every other editor tool here writes JSON, + /// so escaping is Unity's problem rather than a hand-rolled writer's. + /// + /// + public static class ValidationReport + { + /// The schema version written into every document this produces. + public const int SchemaVersion = 1; + + /// + /// Renders a run. + /// + /// The run to render; null yields an empty report. + /// + /// What the project has decided not to be told about; null suppresses nothing. + /// + /// Whether to indent the document. + /// The JSON document; never null. + public static string ToJson( + ValidationRun run, + ValidationSuppressions suppressions, + bool prettyPrint = true + ) + { + ValidationSuppressions effective = suppressions ?? ValidationSuppressions.Empty; + Document document = new Document + { + schemaVersion = SchemaVersion, + assetsConsidered = run == null ? 0 : run.TotalCount, + assetsProcessed = run == null ? 0 : run.ProcessedCount, + complete = run != null && run.IsComplete && !run.IsCancelled, + cancelled = run != null && run.IsCancelled, + }; + + IReadOnlyList findings = + run == null ? Array.Empty() : run.Findings; + for (int index = 0; index < findings.Count; index++) + { + ValidationFinding finding = findings[index]; + bool suppressed = effective.IsSuppressed(finding); + document.findings.Add( + new FindingRecord + { + id = finding.Id, + ruleId = finding.RuleId, + severity = finding.Severity.ToString(), + assetGuid = finding.AssetGuid, + assetPath = finding.AssetPath, + discriminator = finding.Discriminator, + message = finding.Message, + suppressed = suppressed, + } + ); + + if (!suppressed) + { + document.unsuppressedCount++; + } + } + + IReadOnlyList failures = + run == null ? Array.Empty() : run.Failures; + for (int index = 0; index < failures.Count; index++) + { + ValidationRuleFailure failure = failures[index]; + document.failures.Add( + new FailureRecord + { + // A load failure has no rule to blame, and the empty string a JSON reader + // sees for a null is indistinguishable from an unnamed rule -- so the + // report states which it was rather than leaving it to be inferred. + ruleId = failure.RuleId, + loadFailure = failure.IsLoadFailure, + assetPath = failure.AssetPath, + exception = + failure.Exception == null ? string.Empty : failure.Exception.ToString(), + } + ); + } + + IReadOnlyList unused = effective.UnusedIn(findings); + for (int index = 0; index < unused.Count; index++) + { + document.unusedSuppressions.Add(unused[index]); + } + + return JsonUtility.ToJson(document, prettyPrint); + } + + /// + /// Reports whether a run produced anything at or above a severity that is not suppressed. + /// + /// The run to inspect; null counts as nothing found. + /// What to ignore; null suppresses nothing. + /// The lowest severity that counts. + /// true when at least one finding at or above stands. + /// + /// A rule that threw counts, whatever the threshold. It produced no answer for that asset, + /// which is not the same as answering "nothing wrong", so a build that passed on it would + /// be reporting coverage it does not have. + /// + public static bool HasBlockingResults( + ValidationRun run, + ValidationSuppressions suppressions, + ValidationSeverity threshold + ) + { + if (run == null) + { + return false; + } + + if (0 < run.Failures.Count) + { + return true; + } + + ValidationSuppressions effective = suppressions ?? ValidationSuppressions.Empty; + IReadOnlyList findings = run.Findings; + for (int index = 0; index < findings.Count; index++) + { + ValidationFinding finding = findings[index]; + if (threshold <= finding.Severity && !effective.IsSuppressed(finding)) + { + return true; + } + } + + return false; + } + + /// One finding, as the report writes it. + [Serializable] + public sealed class FindingRecord + { + /// The finding's identity across runs. + public string id; + + /// The reporting rule's stable identifier. + public string ruleId; + + /// The severity's name, so the document reads without a lookup table. + public string severity; + + /// The GUID of the asset the finding belongs to. + public string assetGuid; + + /// The asset's project-relative path as of this run. + public string assetPath; + + /// What tells this finding apart from the rule's others on the same asset. + public string discriminator; + + /// The human-readable description. + public string message; + + /// Whether the project's suppression file silences this finding. + public bool suppressed; + } + + /// One rule or loader that threw, as the report writes it. + [Serializable] + public sealed class FailureRecord + { + /// The rule that threw, empty when the asset itself failed to load. + public string ruleId; + + /// Whether loading the asset threw, rather than a rule. + public bool loadFailure; + + /// The asset it was validating. + public string assetPath; + + /// What it threw. + public string exception; + } + + /// The whole document. + [Serializable] + public sealed class Document + { + /// The schema this document follows. + public int schemaVersion; + + /// How many assets the run considered. + public int assetsConsidered; + + /// How many it got through. + public int assetsProcessed; + + /// Whether it reached the end without being cancelled. + public bool complete; + + /// Whether it was cancelled before finishing. + public bool cancelled; + + /// How many findings the suppression file does not silence. + public int unsuppressedCount; + + /// Every finding, suppressed ones included and marked. + public List findings = new List(); + + /// Every rule or loader that threw. + public List failures = new List(); + + /// Suppression entries that silenced nothing in this run. + public List unusedSuppressions = new List(); + } + } +#endif +} diff --git a/Editor/Validation/Continuous/ValidationReport.cs.meta b/Editor/Validation/Continuous/ValidationReport.cs.meta new file mode 100644 index 00000000..5ee336ed --- /dev/null +++ b/Editor/Validation/Continuous/ValidationReport.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 0c0669692a748dd39f30b7e68d505637 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Editor/Validation/Continuous/ValidationSuppressions.cs b/Editor/Validation/Continuous/ValidationSuppressions.cs new file mode 100644 index 00000000..897c9e2a --- /dev/null +++ b/Editor/Validation/Continuous/ValidationSuppressions.cs @@ -0,0 +1,207 @@ +// MIT License - Copyright (c) 2026 wallstop +// Full license text: https://github.com/wallstop/unity-helpers/blob/main/LICENSE + +namespace WallstopStudios.UnityHelpers.Editor.Validation.Continuous +{ +#if UNITY_EDITOR + using System; + using System.Collections.Generic; + using System.Text; + + /// + /// The findings a project has decided not to be told about again, read from a committed file. + /// + /// + /// + /// One per line, so the file is a review artifact rather + /// than an opaque blob: a diff shows exactly which check somebody switched off. Blank lines and + /// # comments are ignored, and writes the asset path and message + /// above each entry as a comment, because a rule name and a GUID tell a reviewer nothing. + /// + /// + /// Matching is on the finding's identity -- rule, asset GUID, discriminator -- and never on the + /// path or the message, so moving the asset or rewording the rule does not silently un-suppress + /// it. That is the same identity the finding already documents, and the reason it excludes + /// those two fields. + /// + /// + /// exists because a suppression that outlives the finding it silenced is + /// the same defect class as a linter that cannot report: it reads as a considered decision and + /// is really a stale line nobody has looked at. A headless run reports them rather than letting + /// the file accumulate. + /// + /// + public sealed class ValidationSuppressions + { + private static readonly string[] NoIds = Array.Empty(); + + private static readonly ValidationSuppressions EmptySuppressions = + new ValidationSuppressions( + new List(), + new HashSet(StringComparer.Ordinal) + ); + + private readonly List _ordered; + private readonly HashSet _ids; + + private ValidationSuppressions(List ordered, HashSet ids) + { + _ordered = ordered; + _ids = ids; + } + + /// A set that suppresses nothing. + public static ValidationSuppressions Empty => EmptySuppressions; + + /// How many distinct findings this set suppresses. + public int Count => _ordered.Count; + + /// The suppressed identities, in the order the file listed them. + public IReadOnlyList Ids => _ordered; + + /// + /// Reads a suppression file. + /// + /// The file's contents; null or blank yields . + /// The set; never null. + /// + /// Nothing here throws or reports. A malformed line is a line that suppresses nothing, + /// which the run then reports through along with every other entry + /// that matched nothing -- one mechanism for "this line does not do what you think" rather + /// than a parse error for some shapes and silence for the rest. + /// + public static ValidationSuppressions Parse(string text) + { + if (string.IsNullOrEmpty(text)) + { + return EmptySuppressions; + } + + List ordered = new List(); + HashSet ids = new HashSet(StringComparer.Ordinal); + foreach (string line in text.Replace("\r\n", "\n").Split('\n')) + { + string trimmed = line.Trim(); + if (trimmed.Length == 0 || trimmed[0] == '#') + { + continue; + } + + if (ids.Add(trimmed)) + { + ordered.Add(trimmed); + } + } + + return ordered.Count == 0 + ? EmptySuppressions + : new ValidationSuppressions(ordered, ids); + } + + /// + /// Renders findings as a suppression file, newest decision first in file order. + /// + /// What to suppress; null entries are skipped. + /// The complete file text, with a trailing newline. + /// + /// The comment above each entry is what makes the file reviewable. It is regenerated from + /// the finding rather than preserved from an earlier file, so it cannot drift into + /// describing something the identity no longer points at. + /// + public static string Render(IReadOnlyList findings) + { + StringBuilder builder = new StringBuilder(); + builder.Append("# Validation suppressions.\n"); + builder.Append("# One finding identity per line: rule|assetGuid|discriminator.\n"); + builder.Append("# Delete a line to be told about that finding again. A line that\n"); + builder.Append("# matches nothing is reported by the headless run rather than kept.\n"); + + HashSet written = new HashSet(StringComparer.Ordinal); + for (int index = 0; index < Safe(findings).Count; index++) + { + ValidationFinding finding = findings[index]; + if (!written.Add(finding.Id)) + { + continue; + } + + builder.Append('\n'); + builder.Append("# "); + builder.Append( + string.IsNullOrEmpty(finding.AssetPath) ? "(no path)" : finding.AssetPath + ); + builder.Append(" -- "); + builder.Append(Single(finding.Message)); + builder.Append('\n'); + builder.Append(finding.Id); + builder.Append('\n'); + } + + return builder.ToString(); + } + + /// Reports whether this set silences a finding. + /// The finding to test. + /// true when the file lists the finding's identity. + public bool IsSuppressed(in ValidationFinding finding) + { + return _ids.Contains(finding.Id); + } + + /// + /// The entries that silenced nothing in a run. + /// + /// Every finding the run produced, suppressed ones included. + /// The unmatched identities, in file order; empty when every entry earned its place. + /// + /// Only meaningful for a run that covered the whole project. A run scoped to one folder + /// will not have seen the assets most entries name, so treating its answer as stale + /// suppressions would delete decisions about assets nobody looked at. + /// + public IReadOnlyList UnusedIn(IReadOnlyList findings) + { + if (_ordered.Count == 0) + { + return NoIds; + } + + HashSet seen = new HashSet(StringComparer.Ordinal); + for (int index = 0; index < Safe(findings).Count; index++) + { + seen.Add(findings[index].Id); + } + + List unused = new List(); + for (int index = 0; index < _ordered.Count; index++) + { + if (!seen.Contains(_ordered[index])) + { + unused.Add(_ordered[index]); + } + } + + return unused.Count == 0 ? NoIds : unused; + } + + private static IReadOnlyList Safe(IReadOnlyList values) + { + return values ?? (IReadOnlyList)Array.Empty(); + } + + /// + /// Flattens a message onto one line, so it cannot become an entry of its own. + /// + /// The finding's message. + /// The message with newlines replaced by spaces. + private static string Single(string message) + { + if (string.IsNullOrEmpty(message)) + { + return "(no message)"; + } + + return message.Replace("\r\n", " ").Replace('\n', ' ').Replace('\r', ' '); + } + } +#endif +} diff --git a/Editor/Validation/Continuous/ValidationSuppressions.cs.meta b/Editor/Validation/Continuous/ValidationSuppressions.cs.meta new file mode 100644 index 00000000..96d694c1 --- /dev/null +++ b/Editor/Validation/Continuous/ValidationSuppressions.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: c5bf92a149b4b359abe9f244f4249554 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/DiagnosticTests.cs b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/DiagnosticTests.cs index 55537e59..ab0a1939 100644 --- a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/DiagnosticTests.cs +++ b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/DiagnosticTests.cs @@ -513,6 +513,78 @@ public void ASubtypeCannotDeclareItselfAgainstABaseInAnotherAssembly() Assert.IsTrue(match.GetMessage().Contains("ConsumerAssembly"), match.GetMessage()); } + /// + /// The refusal explains the mechanism and names a fix that works. + /// + /// + /// A developer whose build just failed needs two things: why, and what to write instead. + /// The "why" is a fact about per-assembly generation -- the base's chain was emitted when + /// the base's assembly compiled -- and NOT a claim that the feature can never exist: + /// emitting the chain in the extending assembly is a different mechanism entirely, and is + /// tracked on + /// #612. + /// The runtime registry refused on + /// #603 + /// is the thing that stays refused. + /// + [Test] + public void TheCrossAssemblyRefusalExplainsTheMechanismAndNamesAWorkingAlternative() + { + MetadataReference upstream = CompileReference( + "UpstreamAssembly", + @"namespace Upstream { using WallstopStudios.UnityHelpers.Core.Serialization.WallstopProto; + [WProtoContract] public partial class Base { [WProtoMember(1)] public int A; } }" + ); + + string message = Run( + @"[WProtoContract] [WProtoSubtype(typeof(Upstream.Base), 100)] public partial class Sub : Upstream.Base { [WProtoMember(1)] public int B; }", + upstream + ) + .Single(diagnostic => diagnostic.Id == "WPROTO040") + .GetMessage(); + + // The mechanism, so the reader can tell this from a number they merely chose badly. + StringAssert.Contains("generated when its own assembly is compiled", message); + + // And the shape that does work, because a diagnostic naming no fix is half a report. + StringAssert.Contains("[WProtoMember]", message); + + // It must not promise a release either. The refusal is real today whatever #612 does. + foreach (string promise in new[] { "not yet", "for now", "in a future", "will be" }) + { + StringAssert.DoesNotContain(promise, message); + } + } + + /// + /// The alternative the refusal recommends compiles and generates, in the consumer assembly. + /// + /// + /// A diagnostic that names a fix has to name one that works, or the developer spends the + /// refusal twice. Composition is what a per-assembly generator CAN honour: the member's + /// declared type resolves through the upstream assembly's own formatter, which carries the + /// upstream subtypes in the chain that was emitted with it. + /// + [Test] + public void TheAlternativeTheCrossAssemblyRefusalRecommendsGenerates() + { + MetadataReference upstream = CompileReference( + "UpstreamAssembly", + @"namespace Upstream { using WallstopStudios.UnityHelpers.Core.Serialization.WallstopProto; + [WProtoContract] [WProtoInclude(100, typeof(UpstreamSub))] public partial class Base { [WProtoMember(1)] public int A; } + [WProtoContract] public partial class UpstreamSub : Base { [WProtoMember(1)] public int C; } }" + ); + + CollectionAssert.IsEmpty( + Run( + @"[WProtoContract] public partial class Holder { [WProtoMember(1)] public Upstream.Base Wrapped; [WProtoMember(2)] public int B; }", + upstream + ) + .Select(diagnostic => diagnostic.Id + " " + diagnostic.GetMessage()) + .ToArray() + ); + } + /// /// The two declaration forms emit the same formatter, character for character. /// @@ -965,6 +1037,268 @@ public void TwoMembersClaimingOneFieldNumberIsAnError() ); } + /// + /// A member cannot take a field number the contract reserved for a removed one. + /// + /// + /// #608. + /// WPROTO002 fires on two members that exist at once and so cannot see a number a deletion + /// freed: every payload written before the removal still carries that field, and giving it + /// to another member reads those saves back as the wrong thing. + /// + [Test] + public void AMemberCannotTakeAReservedFieldNumber() + { + AssertDiagnostic( + "WPROTO043", + "field number 3", + @"[WProtoContract] [WProtoReserved(3)] public sealed partial class Save + { + [WProtoMember(3)] public string Name; + }" + ); + } + + [Test] + public void AMemberCannotTakeAReservedName() + { + // protobuf reserves names as well as numbers, and for the same reason: a re-added + // Health at a DIFFERENT number still breaks anything matching by name -- a JSON + // projection, a generated .proto consumer, a schema registry. + AssertDiagnostic( + "WPROTO043", + "the name 'Health'", + @"[WProtoContract] [WProtoReserved(""Health"")] public sealed partial class Save + { + [WProtoMember(9)] public int Health; + }" + ); + } + + /// + /// A reservation is a record, and a record may not touch the wire. + /// + /// + /// Stronger than comparing bytes for a handful of values: if the emitted code is the same + /// code, there is no payload the two could disagree about. A reservation that changed the + /// formatter would be a wire break introduced by documenting a wire contract, which is the + /// one outcome this feature must not have. + /// + [Test] + public void AReservationDoesNotChangeTheEmittedFormatter() + { + const string Members = + @" public sealed partial class Save + { + [WProtoMember(1)] public int Kept; + [WProtoMember(4)] public string Name; + }"; + + Assert.AreEqual( + GeneratedFormatterFor("Consumer.Save", "[WProtoContract]" + Members), + GeneratedFormatterFor( + "Consumer.Save", + @"[WProtoContract] [WProtoReserved(2, 3)] [WProtoReserved(""Health"")]" + + Members + ) + ); + } + + [Test] + public void AMemberCannotRenameItselfOntoAReservedName() + { + // [WProtoMember(Name = ...)] is what a generated schema, a payload dump and anything + // matching by name actually see, so a rule reading only the C# name is one an author + // steps around by renaming. + AssertDiagnostic( + "WPROTO043", + "the name 'Health'", + @"[WProtoContract] [WProtoReserved(""Health"")] public sealed partial class Save + { + [WProtoMember(9, Name = ""Health"")] public int Hp; + }" + ); + } + + [Test] + public void RenamingAwayFromAReservedNameIsAllowed() + { + // The identifier here IS the reserved word and the schema name is not, which is the + // only arrangement that can tell the two identities apart -- the first draft named the + // member Vitality as well, so it passed whichever name the rule happened to read. + // Reported by Cursor Bugbot. + CollectionAssert.IsEmpty( + Run( + @"[WProtoContract] [WProtoReserved(""Health"")] public sealed partial class Save + { + [WProtoMember(9, Name = ""Vitality"")] public int Health; + }" + ) + .Select(diagnostic => diagnostic.Id + " " + diagnostic.GetMessage()) + .ToArray() + ); + } + + [Test] + public void AMemberTakingBothAReservedNumberAndNameIsNamedForBoth() + { + AssertDiagnostic( + "WPROTO043", + "field number 3 and the name 'Health'", + @"[WProtoContract] [WProtoReserved(3)] [WProtoReserved(""Health"")] public sealed partial class Save + { + [WProtoMember(3)] public int Health; + }" + ); + } + + [Test] + public void OneDeclarationCanReserveSeveralNumbers() + { + AssertDiagnostic( + "WPROTO043", + "field number 9", + @"[WProtoContract] [WProtoReserved(3, 7, 9)] public sealed partial class Save + { + [WProtoMember(9)] public int Later; + }" + ); + } + + [Test] + public void AReservationDoesNotRefuseTheNumbersAroundIt() + { + // The refusal has to be exactly the reserved set. One that swallowed the numbers beside it + // would push every later member up the number line for no reason, and the numbers it + // skipped would be lost as surely as the reserved one. + CollectionAssert.IsEmpty( + Run( + @"[WProtoContract] [WProtoReserved(3)] [WProtoReserved(""Health"")] public sealed partial class Save + { + [WProtoMember(2)] public int Before; + [WProtoMember(4)] public int After; + [WProtoMember(5)] public int Healthy; + }" + ) + .Select(diagnostic => diagnostic.Id + " " + diagnostic.GetMessage()) + .ToArray() + ); + } + + [Test] + public void AReservationOnOneContractDoesNotBindAnother() + { + // Field numbers live in one type's space. A reservation inherited from a base -- or + // leaking to a sibling -- would refuse a member for a collision that cannot happen. + CollectionAssert.IsEmpty( + Run( + @"[WProtoContract] [WProtoReserved(3)] public partial class Base { [WProtoMember(1)] public int A; } + [WProtoContract] [WProtoSubtype(typeof(Base), 100)] public partial class Sub : Base { [WProtoMember(3)] public int B; } + [WProtoContract] public sealed partial class Unrelated { [WProtoMember(3)] public int C; }" + ) + .Select(diagnostic => diagnostic.Id + " " + diagnostic.GetMessage()) + .ToArray() + ); + } + + [Test] + public void ARemovedMemberComingBackUnchangedIsAllowedOnceItsReservationGoes() + { + // The escape the message names, asserted so it is real: a reservation is a record, not + // a permanent ban on a type ever holding that field again. + CollectionAssert.IsEmpty( + Run( + @"[WProtoContract] public sealed partial class Save + { + [WProtoMember(3)] public int Health; + }" + ) + .Select(diagnostic => diagnostic.Id + " " + diagnostic.GetMessage()) + .ToArray() + ); + } + + /// + /// A reservation binds subtype discriminators, not only members. + /// + /// + /// Reported by Cursor Bugbot against the first draft, which checked only + /// [WProtoMember]. A base's includes are numbered against its members -- one space -- + /// so a rule binding one half is one an author steps around by writing the number on the + /// other. + /// + [Test] + public void AnIncludeCannotTakeAReservedFieldNumber() + { + AssertDiagnostic( + "WPROTO013", + "is reserved on 'Base'", + @"[WProtoContract] [WProtoReserved(100)] [WProtoInclude(100, typeof(Sub))] public partial class Base { [WProtoMember(1)] public int A; } + [WProtoContract] public partial class Sub : Base { [WProtoMember(1)] public int B; }" + ); + } + + [Test] + public void ASubtypeDeclarationCannotTakeAReservedFieldNumber() + { + AssertDiagnostic( + "WPROTO040", + "is reserved on 'Base'", + @"[WProtoContract] [WProtoReserved(100)] public partial class Base { [WProtoMember(1)] public int A; } + [WProtoContract] [WProtoSubtype(typeof(Base), 100)] public partial class Sub : Base { [WProtoMember(1)] public int B; }" + ); + } + + [Test] + public void AReservationOnABaseDoesNotRefuseAnUnreservedDiscriminator() + { + // The refusal is the reserved set exactly. One that swallowed the numbers beside it + // would push every later subtype up the number line for no reason. + CollectionAssert.IsEmpty( + Run( + @"[WProtoContract] [WProtoReserved(100)] [WProtoInclude(101, typeof(Sub))] public partial class Base { [WProtoMember(1)] public int A; } + [WProtoContract] public partial class Sub : Base { [WProtoMember(1)] public int B; }" + ) + .Select(diagnostic => diagnostic.Id + " " + diagnostic.GetMessage()) + .ToArray() + ); + } + + [Test] + public void ReservationsDoNotChangeWhatTwoLiveMembersOnOneNumberReport() + { + // The acceptance criterion that the existing duplicate rule is untouched. A contract + // that reserves something unrelated still gets WPROTO002 for its live collision. + AssertDiagnostic( + "WPROTO002", + "Second", + @"[WProtoContract] [WProtoReserved(42)] public sealed partial class Clash + { + [WProtoMember(1)] public int First; + [WProtoMember(1)] public int Second; + }" + ); + } + + [Test] + public void EveryMemberOnAReservedNumberIsToldWhyRatherThanOneBeingCalledADuplicate() + { + // Both are wrong for the same reason, and neither may keep the number, so "you are a + // duplicate of the one above" would send the second author to the wrong fix. + CollectionAssert.AreEqual( + new[] { "WPROTO043", "WPROTO043" }, + Run( + @"[WProtoContract] [WProtoReserved(1)] public sealed partial class Clash + { + [WProtoMember(1)] public int First; + [WProtoMember(1)] public int Second; + }" + ) + .Select(diagnostic => diagnostic.Id) + .ToArray() + ); + } + // Every one of these is a shape a developer would reasonably expect to work, which is why it // has to fail the build with a message rather than silently get no formatter. // diff --git a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/SubtypeTagManifestTests.cs b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/SubtypeTagManifestTests.cs index 19d843f9..9542edf1 100644 --- a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/SubtypeTagManifestTests.cs +++ b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator.Tests/SubtypeTagManifestTests.cs @@ -471,6 +471,33 @@ public void AFreshNumberAvoidsTheBasesOwnMembersAndItsIncludes() NoEntries ); + CollectionAssert.AreEqual( + new[] { "N.Pinned=3", "N.Sub=4" }, + Describe(plan.Assigned), + "N.Sub avoids 1, 2 and 3; N.Pinned is recorded at the number it wrote itself" + ); + } + + [Test] + public void AFreshNumberAvoidsWhatTheBaseReservedWithWProtoReserved() + { + // The other half of Bugbot's second finding. A reserved number is spent as surely as a + // live one -- the generator refuses a discriminator that takes it -- so assigning + // around only the live numbers hands out a number the next compile rejects, which is + // the deadlock this tool exists to remove. The assigner feeds [WProtoReserved] numbers + // in through `reserved`, exactly as it does members and includes. + WProtoSubtypeTagPlan plan = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base") }, + new[] + { + Entry("Id", "N.Base", 1), + Entry("[WProtoReserved]", "N.Base", 2), + Entry("[WProtoReserved]", "N.Base", 3), + }, + NoEntries, + NoEntries + ); + CollectionAssert.AreEqual(new[] { "N.Sub=4" }, Describe(plan.Assigned)); } @@ -579,7 +606,9 @@ public void TheRenderedManifestIsWhatTheGeneratorReadsBack() public void APromotedSubtypeKeepsItsNumberWithoutRetiringIt() { // Moving a number out of the manifest and into the attribute changes nothing on the - // wire, so retiring it would forbid the very declaration now holding it. + // wire, so retiring it would forbid the very declaration now holding it. The entry + // stays as the record of a number that has been spent (#606) rather than being dropped + // as redundant -- dropping it was what let the next deletion free the number silently. WProtoSubtypeTagPlan plan = WProtoSubtypeTagPlan.Create( new[] { Declare("N.Sub", "N.Base", 4) }, NoEntries, @@ -587,8 +616,299 @@ public void APromotedSubtypeKeepsItsNumberWithoutRetiringIt() NoEntries ); - Assert.IsEmpty(plan.Assigned); + CollectionAssert.AreEqual(new[] { "N.Sub=4" }, Describe(plan.Assigned)); Assert.IsEmpty(plan.Retired); + Assert.IsEmpty(plan.FreshlyAssigned); + } + + [Test] + public void AnExplicitlyNumberedSubtypeIsRecordedSoItsNumberCanBeRetired() + { + // #606. A number written by hand is as durable a wire contract as one the tool + // assigned, and until this recorded it the only trace that 1 had ever been spent was + // the declaration itself -- which is deleted along with the type it sits on. + WProtoSubtypeTagPlan plan = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Melee", "N.Base", 1) }, + NoEntries, + NoEntries, + NoEntries + ); + + CollectionAssert.AreEqual(new[] { "N.Melee=1" }, Describe(plan.Assigned)); + Assert.IsEmpty( + plan.FreshlyAssigned, + "the number was written by the developer, so nothing was invented and the " + + "automatic pass has no reason to run" + ); + } + + [Test] + public void DeletingAnExplicitlyNumberedSubtypeRetiresItsNumber() + { + // The half #606 is named for: WPROTO039 has no memory, so a number freed by a deletion + // is indistinguishable from one never used unless the deletion leaves a record. + WProtoSubtypeTagPlan recorded = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Melee", "N.Base", 1) }, + NoEntries, + NoEntries, + NoEntries + ); + + WProtoSubtypeTagPlan afterDeletion = WProtoSubtypeTagPlan.Create( + new WProtoSubtypeTagPlan.Declaration[0], + NoEntries, + recorded.Assigned, + recorded.Retired + ); + + CollectionAssert.AreEqual(new[] { "N.Melee=1" }, Describe(afterDeletion.Retired)); + Assert.IsEmpty(afterDeletion.Assigned); + } + + [Test] + public void ANumberFreedByDeletingAnExplicitlyNumberedSubtypeIsNeverHandedOut() + { + // The consequence, end to end. Without the record the next subtype is handed 1 -- the + // smallest free number -- and every payload written by an older build reads that field + // back as the wrong type, with no diagnostic anywhere. + WProtoSubtypeTagPlan recorded = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Melee", "N.Base", 1) }, + NoEntries, + NoEntries, + NoEntries + ); + WProtoSubtypeTagPlan afterDeletion = WProtoSubtypeTagPlan.Create( + new WProtoSubtypeTagPlan.Declaration[0], + NoEntries, + recorded.Assigned, + recorded.Retired + ); + + WProtoSubtypeTagPlan withSuccessor = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Later", "N.Base") }, + NoEntries, + afterDeletion.Assigned, + afterDeletion.Retired + ); + + CollectionAssert.AreEqual(new[] { "N.Later=2" }, Describe(withSuccessor.Assigned)); + CollectionAssert.AreEqual(new[] { "N.Melee=1" }, Describe(withSuccessor.Retired)); + } + + [Test] + public void ReAddingAnExplicitlyNumberedSubtypeTakesBackTheNumberItHeld() + { + // Remove-then-re-add has to keep working for the explicit form too: the type comes back + // with the number it always had, and the retirement it left behind is lifted rather + // than left to forbid the very declaration now holding it. + WProtoSubtypeTagPlan recorded = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Melee", "N.Base", 1) }, + NoEntries, + NoEntries, + NoEntries + ); + WProtoSubtypeTagPlan afterDeletion = WProtoSubtypeTagPlan.Create( + new WProtoSubtypeTagPlan.Declaration[0], + NoEntries, + recorded.Assigned, + recorded.Retired + ); + + WProtoSubtypeTagPlan restored = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Melee", "N.Base", 1) }, + NoEntries, + afterDeletion.Assigned, + afterDeletion.Retired + ); + + CollectionAssert.AreEqual(new[] { "N.Melee=1" }, Describe(restored.Assigned)); + Assert.IsEmpty(restored.Retired, "the number is in use again by the type that held it"); + } + + [Test] + public void ReAddingATypeLiftsOnlyTheRetirementItReclaims() + { + // Reported by Cursor Bugbot against the first draft, which keyed the lift by + // subtype/base pair. A pair can hold MORE than one retirement -- a hand-edited number + // leaves one and a later deletion leaves another -- and re-adding the type under the + // first freed the second, which is the exact reuse the record exists to forbid. + WProtoSubtypeTagPlan plan = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base", 5) }, + NoEntries, + NoEntries, + new[] { Entry("N.Sub", "N.Base", 5), Entry("N.Sub", "N.Base", 7) } + ); + + CollectionAssert.AreEqual(new[] { "N.Sub=5" }, Describe(plan.Assigned)); + CollectionAssert.AreEqual( + new[] { "N.Sub=7" }, + Describe(plan.Retired), + "7 belonged to an earlier version of this type and is still spent" + ); + } + + [Test] + public void ANumberAPairRetiredTwiceOverIsNeverHandedToTheNextSubtype() + { + // The consequence, driven one step further: with the retirement dropped, the next + // tag-less subtype was handed the freed number. + WProtoSubtypeTagPlan plan = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base", 1), Declare("N.Later", "N.Base") }, + NoEntries, + NoEntries, + new[] { Entry("N.Sub", "N.Base", 1), Entry("N.Sub", "N.Base", 2) } + ); + + CollectionAssert.DoesNotContain( + plan.Assigned.Select(entry => entry.Tag).ToArray(), + 2, + "2 is retired and may never be handed out again" + ); + CollectionAssert.Contains(Describe(plan.Retired), "N.Sub=2"); + } + + [Test] + public void ATaglessReAddLiftsOnlyTheRetirementItReclaims() + { + // Same rule through the tag-less path, which restores from the manifest rather than + // from the attribute. + WProtoSubtypeTagPlan plan = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base") }, + NoEntries, + NoEntries, + new[] { Entry("N.Sub", "N.Base", 3), Entry("N.Sub", "N.Base", 8) } + ); + + CollectionAssert.AreEqual(new[] { "N.Sub=3" }, Describe(plan.Assigned)); + CollectionAssert.AreEqual(new[] { "N.Sub=8" }, Describe(plan.Retired)); + } + + [Test] + public void DemotingASubtypeToTheManifestKeepsTheNumberItWroteByHand() + { + // The other direction of the promotion case above, and the same defect: with nothing + // recording that the attribute said 1, deleting the number from the source made the + // pair look brand new and it was handed the smallest free number instead. + WProtoSubtypeTagPlan recorded = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base", 40) }, + NoEntries, + NoEntries, + NoEntries + ); + + WProtoSubtypeTagPlan demoted = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base") }, + NoEntries, + recorded.Assigned, + recorded.Retired + ); + + CollectionAssert.AreEqual(new[] { "N.Sub=40" }, Describe(demoted.Assigned)); + Assert.IsEmpty(demoted.Retired); + Assert.IsEmpty( + demoted.FreshlyAssigned, + "40 came from the record, so nothing was invented" + ); + } + + [Test] + public void RenumberingAnExplicitDeclarationRetiresTheNumberItLeft() + { + // Editing a shipped number in place is the thing the guidance forbids, and it used to + // be invisible. The new number is recorded and the old one is retired, so a later + // subtype cannot be given the number old payloads still mean this type by. + WProtoSubtypeTagPlan recorded = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base", 5) }, + NoEntries, + NoEntries, + NoEntries + ); + + WProtoSubtypeTagPlan renumbered = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Sub", "N.Base", 6) }, + NoEntries, + recorded.Assigned, + recorded.Retired + ); + + CollectionAssert.AreEqual(new[] { "N.Sub=6" }, Describe(renumbered.Assigned)); + CollectionAssert.AreEqual(new[] { "N.Sub=5" }, Describe(renumbered.Retired)); + } + + [Test] + public void AnExplicitDeclarationIsNotRetiredByAnUnattendedPassThatCannotSeeIt() + { + // Recording the explicit form must not weaken the Partial guard: a subtype behind + // #if !UNITY_EDITOR is absent from TypeCache and present in the player, so an + // unattended pass keeps its number claimed rather than retiring it. + WProtoSubtypeTagPlan recorded = WProtoSubtypeTagPlan.Create( + new[] { Declare("N.Hidden", "N.Base", 1) }, + NoEntries, + NoEntries, + NoEntries + ); + + WProtoSubtypeTagPlan unattended = WProtoSubtypeTagPlan.Create( + new WProtoSubtypeTagPlan.Declaration[0], + NoEntries, + recorded.Assigned, + recorded.Retired, + WProtoSubtypeTagDiscovery.Partial + ); + + CollectionAssert.AreEqual(new[] { "N.Hidden=1" }, Describe(unattended.Assigned)); + Assert.IsEmpty(unattended.Retired); + } + + [Test] + public void TheGeneratorRefusesAnExplicitSubtypeClaimingARetiredNumber() + { + // The enforcement half. The record is only worth what refuses to spend it again, and + // WPROTO039 cannot: it fires on two LIVE claims, and a retired number has none. + Diagnostic match = Run( + Fixture( + "[assembly: WProtoRetiredSubtypeTag(\"Consumer.Deleted\", typeof(Consumer.Base), 7)]", + "[WProtoSubtype(typeof(Base), 7)]" + ) + ) + .Single(diagnostic => diagnostic.Id == "WPROTO040"); + + StringAssert.Contains("Consumer.Deleted", match.GetMessage()); + StringAssert.Contains("retired", match.GetMessage()); + } + + [Test] + public void TheGeneratorRefusesAnIncludeClaimingARetiredNumber() + { + // [WProtoInclude] on the base and [WProtoSubtype] on the subtype are the same + // declaration written two ways and share one field-number space, so a rule that + // covered only one of them is a rule an author steps around by accident. + Diagnostic match = Run( + "[assembly: WProtoRetiredSubtypeTag(\"Consumer.Deleted\", typeof(Consumer.Base), 7)]" + + "\n[WProtoContract] [WProtoInclude(7, typeof(Sub))] public partial class Base { [WProtoMember(1)] public int A; }" + + "\n[WProtoContract] public partial class Sub : Base { [WProtoMember(1)] public int B; }" + ) + .Single(diagnostic => diagnostic.Id == "WPROTO013"); + + StringAssert.Contains("Consumer.Deleted", match.GetMessage()); + StringAssert.Contains("retired", match.GetMessage()); + } + + [Test] + public void TheGeneratorLetsARetiredTypeReclaimItsOwnNumber() + { + // Re-adding the type the number belonged to is the case retirement exists to serve, so + // the refusal is about the NAME, not about the number alone. + Assert.IsEmpty( + Describe( + Run( + Fixture( + "[assembly: WProtoRetiredSubtypeTag(\"Consumer.Sub\", typeof(Consumer.Base), 7)]", + "[WProtoSubtype(typeof(Base), 7)]" + ) + ) + ) + ); } [Test] @@ -1124,8 +1444,15 @@ public void ASecondPassOverAnAlreadyWrittenManifestWritesNothing() } [Test] - public void AnAssemblyWhoseSubtypesAllWriteTheirOwnNumbersGetsNoManifestAtAll() - { + public void AdoptingThePackageWritesNoManifestIntoAProjectThatInventsNoNumbers() + { + // The guard that "adopting the package must not put a file into a project" rests on, + // now that a hand-written number is recorded rather than dropped (#606). It was + // plan.IsEmpty, which said the same thing only for as long as such a plan had nothing + // in it; the real gate is the one the unattended pass reads -- + // WProtoSubtypeTagAssigner skips the write when FreshlyAssigned is empty, so a project + // that numbers its own subtypes gets a file only from a deliberate menu run whose diff + // a human reads. WProtoSubtypeTagPlan plan = WProtoSubtypeTagPlan.Create( new[] { Declare("N.Sub", "N.Base", 4) }, NoEntries, @@ -1133,10 +1460,14 @@ public void AnAssemblyWhoseSubtypesAllWriteTheirOwnNumbersGetsNoManifestAtAll() NoEntries ); - Assert.IsTrue(plan.IsEmpty); - Assert.IsFalse( - WProtoSubtypeTagManifestFile.NeedsWrite(null, plan.Render("A"), plan.IsEmpty), - "adopting the package must not put a file into a project that never uses the form" + Assert.IsEmpty( + plan.FreshlyAssigned, + "nothing was invented, so the automatic pass has no reason to write" + ); + CollectionAssert.AreEqual( + new[] { "N.Sub=4" }, + Describe(plan.Assigned), + "and an explicit run records the number, which is what makes a later deletion visible" ); } diff --git a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/ReservedMap.cs b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/ReservedMap.cs new file mode 100644 index 00000000..03beaf5d --- /dev/null +++ b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/ReservedMap.cs @@ -0,0 +1,184 @@ +// MIT License - Copyright (c) 2026 wallstop +// Full license text: https://github.com/wallstop/unity-helpers/blob/main/LICENSE + +namespace WallstopStudios.UnityHelpers.Proto.Generator +{ + using System; + using System.Collections.Generic; + using Microsoft.CodeAnalysis; + + /// + /// The field numbers and member names one contract's [WProtoReserved] declarations hold + /// against every member of that contract. + /// + /// + /// + /// Read from the contract itself and from nothing else. A reservation is a statement about one + /// type's own field-number space, so inheriting one from a base -- whose numbers live in a + /// different space entirely -- would refuse a member for a collision that cannot happen. + /// + /// + /// The record exists because the declaration that spends a number is deleted along with the + /// member it sits on, so WPROTO002 -- which fires on two LIVE claims -- cannot see a + /// number a deletion freed + /// (#608). + /// + /// + internal sealed class ReservedMap + { + internal const string ReservedAttribute = + "WallstopStudios.UnityHelpers.Core.Serialization.WallstopProto.WProtoReservedAttribute"; + + private static readonly ReservedMap EmptyMap = new ReservedMap( + new HashSet(), + new HashSet(StringComparer.Ordinal) + ); + + private readonly HashSet _numbers; + private readonly HashSet _names; + + private ReservedMap(HashSet numbers, HashSet names) + { + _numbers = numbers; + _names = names; + } + + /// A map for a contract that reserves nothing. + internal static ReservedMap Empty => EmptyMap; + + /// Whether this contract reserves anything at all. + internal bool IsEmpty => _numbers.Count == 0 && _names.Count == 0; + + /// The reserved field numbers, ascending. + internal IEnumerable Numbers + { + get + { + List ordered = new List(_numbers); + ordered.Sort(); + return ordered; + } + } + + /// The reserved member names, in ordinal order. + internal IEnumerable Names + { + get + { + List ordered = new List(_names); + ordered.Sort(StringComparer.Ordinal); + return ordered; + } + } + + /// + /// Indexes one contract's reservations. + /// + /// The contract to read. + /// The map; empty when the contract reserves nothing. + internal static ReservedMap Build(INamedTypeSymbol contract) + { + if (contract == null) + { + return EmptyMap; + } + + HashSet numbers = new HashSet(); + HashSet names = new HashSet(StringComparer.Ordinal); + foreach (AttributeData attribute in contract.GetAttributes()) + { + if ( + attribute.AttributeClass == null + || attribute.AttributeClass.ToDisplayString() != ReservedAttribute + ) + { + continue; + } + + foreach (TypedConstant value in Arguments(attribute)) + { + if (value.Value is int number) + { + numbers.Add(number); + } + else if (value.Value is string name && !string.IsNullOrEmpty(name)) + { + names.Add(name); + } + } + } + + return numbers.Count == 0 && names.Count == 0 + ? EmptyMap + : new ReservedMap(numbers, names); + } + + /// Whether a field number may not be used. + /// The number a member is claiming. + /// true when the contract reserves it. + internal bool ReservesNumber(int fieldNumber) + { + return _numbers.Contains(fieldNumber); + } + + /// + /// Explains why a reserved field number cannot be taken by a subtype declaration. + /// + /// The number being claimed. + /// The contract that reserves it. + /// The clause an include or subtype diagnostic appends. + /// + /// Members and subtype discriminators share ONE field-number space -- a base's includes are + /// numbered against its members -- so a rule that bound only [WProtoMember] would be + /// one an author steps around by writing the number on an include instead. + /// + internal static string ReservedProblem(int fieldNumber, string contractName) + { + return "field number " + + fieldNumber + + " is reserved on '" + + contractName + + "' with [WProtoReserved]. A subtype's number and a member's number are the same " + + "space, so a reservation binds both. Every payload written before the removal " + + "still carries that field, and a discriminator sharing it reads those saves back " + + "as the wrong type. Use a free number, or delete the matching [WProtoReserved] if " + + "this really is the removed declaration coming back"; + } + + /// Whether a member name may not be used. + /// The name a member is declared under. + /// true when the contract reserves it. + internal bool ReservesName(string memberName) + { + return !string.IsNullOrEmpty(memberName) && _names.Contains(memberName); + } + + /// + /// Flattens one declaration's arguments, whichever overload wrote them. + /// + /// The reservation. + /// Every value it names, arrays expanded. + /// + /// Both constructors are (first, params rest[]), so a declaration arrives as a scalar + /// followed by an array. Written to expand any array it finds rather than to assume that + /// shape, so a later overload cannot silently drop its values. + /// + private static IEnumerable Arguments(AttributeData attribute) + { + foreach (TypedConstant argument in attribute.ConstructorArguments) + { + if (argument.Kind == TypedConstantKind.Array) + { + foreach (TypedConstant element in argument.Values) + { + yield return element; + } + + continue; + } + + yield return argument; + } + } + } +} diff --git a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeMap.cs b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeMap.cs index 1522eaf7..7376fa08 100644 --- a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeMap.cs +++ b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeMap.cs @@ -383,6 +383,19 @@ out string problem ) ) { + // A per-assembly generator emits the base's dispatch chain when the base's assembly + // compiles, so a subtype declared afterwards in a referencing assembly is not late + // to a list -- it is outside the compilation that built the list. That is a fact + // about THIS mechanism, and the message says only that. + // + // A runtime registry would close the gap and is refused: unordered registrars, two + // packages claiming one tag, and a lookup stripping under IL2CPP are all silent + // data corruption rather than build errors + // (https://github.com/Ambiguous-Interactive/unity-helpers/issues/603). Emitting the + // base's chain in the EXTENDING assembly instead is neither a registry nor + // refused, and is tracked on + // https://github.com/Ambiguous-Interactive/unity-helpers/issues/612 -- so the + // message must not tell a developer the feature can never exist. problem = "'" + baseType.Name @@ -392,13 +405,20 @@ out string problem + subType.Name + "' into '" + (subType.ContainingAssembly == null ? "?" : subType.ContainingAssembly.Name) - + "'. The base's dispatch chain was generated when its own assembly was compiled " - + "and nothing added later can appear in it, so this subtype would compile and " - + "then throw on the first save. Move '" + + "'. The base's dispatch chain is generated when its own assembly is compiled, " + + "so a subtype declared afterwards in an assembly that references it cannot " + + "appear in that chain, and accepting the declaration would compile and then " + + "throw on the first save. Either move '" + subType.Name + "' into '" + (baseType.ContainingAssembly == null ? "?" : baseType.ContainingAssembly.Name) - + "', or hold it behind a contract of its own rather than as its base"; + + "', or give '" + + subType.Name + + "' a [WProtoContract] of its own and hold a '" + + baseType.Name + + "' in it as a [WProtoMember] instead of deriving from it -- a member of a " + + "type from another assembly is generated normally, and the base writes its " + + "own subtypes through its own chain"; return true; } @@ -422,11 +442,48 @@ out string problem "field number " + tag + " is outside 1-536870911 or inside the reserved 19000-19999 range"; + return true; + } + + // A hand-written number is checked against the retirement record, and a manifest one is + // not: an entry that collides with a retirement is WPROTO042 at the manifest line that + // holds it, and reporting the same collision twice sends the developer to the + // declaration rather than to the file the number actually lives in. The name is what + // decides, not the number -- re-adding the type the number belonged to is the case + // retirement exists to serve (#606). + if ( + !tagless + && manifest.TryRetired(baseType, tag, out string retiredBy) + && retiredBy != subType.ToDisplayString() + ) + { + problem = RetiredProblem(tag, baseType, retiredBy); } return true; } + /// + /// Explains why a retired field number cannot be handed to another subtype. + /// + /// The field number being claimed. + /// The base it lives on. + /// The fully qualified name of the type that held it. + /// The clause a subtype or include diagnostic appends. + internal static string RetiredProblem(int tag, INamedTypeSymbol baseType, string retiredBy) + { + return "field number " + + tag + + " on '" + + (baseType == null ? "?" : baseType.Name) + + "' is retired, having belonged to '" + + retiredBy + + "'. Payloads written before that type was removed still carry it under this " + + "number, so handing it to another type reads those saves back as the wrong " + + "type. Give this one a free number, or restore the deleted type under its own " + + "name"; + } + /// /// Reports whether or anything enclosing it takes type arguments. /// diff --git a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeTagManifest.cs b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeTagManifest.cs index d0333ad7..5e948f4f 100644 --- a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeTagManifest.cs +++ b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/SubtypeTagManifest.cs @@ -44,14 +44,20 @@ internal sealed class SubtypeTagManifest "WallstopStudios.UnityHelpers.Core.Serialization.WallstopProto.WProtoRetiredSubtypeTagAttribute"; private static readonly SubtypeTagManifest EmptyManifest = new SubtypeTagManifest( - new Dictionary(StringComparer.Ordinal) + new Dictionary(StringComparer.Ordinal), + new Dictionary(StringComparer.Ordinal) ); private readonly Dictionary _assigned; + private readonly Dictionary _retired; - private SubtypeTagManifest(Dictionary assigned) + private SubtypeTagManifest( + Dictionary assigned, + Dictionary retired + ) { _assigned = assigned; + _retired = retired; } /// A manifest with no entries, for a compilation that declares none. @@ -70,12 +76,35 @@ private SubtypeTagManifest(Dictionary assigned) internal static SubtypeTagManifest Build(Compilation compilation) { Dictionary assigned = new Dictionary(StringComparer.Ordinal); + Dictionary retired = new Dictionary( + StringComparer.Ordinal + ); foreach (AttributeData attribute in compilation.Assembly.GetAttributes()) { + if ( + TryReadEntry( + attribute, + RetiredAttribute, + out string retiredName, + out INamedTypeSymbol retiredBase, + out int retiredTag + ) + ) + { + string retiredKey = TagKeyOf(retiredBase, retiredTag); + if (!retired.ContainsKey(retiredKey)) + { + retired[retiredKey] = retiredName; + } + + continue; + } + if ( !TryReadEntry( attribute, + TagAttribute, out string subTypeName, out INamedTypeSymbol baseType, out int tag @@ -92,7 +121,34 @@ out int tag } } - return assigned.Count == 0 ? EmptyManifest : new SubtypeTagManifest(assigned); + return assigned.Count == 0 && retired.Count == 0 + ? EmptyManifest + : new SubtypeTagManifest(assigned, retired); + } + + /// + /// Looks up the subtype a retired field number used to belong to. + /// + /// The base the number lives on. + /// The field number being claimed. + /// The fully qualified name of the type that held it. + /// false when the number is not retired on that base. + /// + /// The enforcement half of the retirement record. WPROTO039 fires when two types + /// claim one number at the same TIME and so has no memory: a number freed by a deletion is + /// indistinguishable from one never used, and handing it to a later subtype reads every + /// payload written by an older build back as the wrong type + /// (#606). + /// + internal bool TryRetired(INamedTypeSymbol baseType, int tag, out string retiredBy) + { + if (baseType == null) + { + retiredBy = null; + return false; + } + + return _retired.TryGetValue(TagKeyOf(baseType, tag), out retiredBy); } /// @@ -289,6 +345,7 @@ internal static void Validate(Compilation compilation, Action report private static bool TryReadEntry( AttributeData attribute, + string attributeName, out string subTypeName, out INamedTypeSymbol baseType, out int tag @@ -296,7 +353,7 @@ out int tag { if ( attribute.AttributeClass == null - || attribute.AttributeClass.ToDisplayString() != TagAttribute + || attribute.AttributeClass.ToDisplayString() != attributeName || attribute.ConstructorArguments.Length < 3 ) { diff --git a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoDiagnostics.cs b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoDiagnostics.cs index ec2f00e3..318c2afb 100644 --- a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoDiagnostics.cs +++ b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoDiagnostics.cs @@ -455,6 +455,33 @@ internal static class WProtoDiagnostics isEnabledByDefault: true ); + /// + /// A member claiming a field number or a name the contract has reserved. + /// + /// + /// + /// Its own code rather than part of WPROTO002, whose subject is two members that + /// exist at once. This one is about a member that no longer exists: the declaration that + /// spent the number was deleted with it, so nothing but the reservation records that the + /// number was ever used + /// (#608). + /// + /// + /// It is also the answer to a reservation that contradicts a live member, because that is + /// the same state seen from the other side. Which of the two is wrong cannot be decided + /// here -- the member may be the removed one coming back unchanged -- so the message offers + /// both fixes rather than a second diagnostic that could never fire alongside this one. + /// + /// + internal static readonly DiagnosticDescriptor ReservedTag = new DiagnosticDescriptor( + "WPROTO043", + "WallstopProto member takes something the contract reserved", + "'{0}.{1}' claims {2}, which '{0}' reserves with [WProtoReserved]. A reservation records what a removed member held, because the declaration that spent it was deleted along with it -- so every payload written before the removal still carries that field, and giving it to another member reads those saves back as the wrong thing. Use a free field number and an unreserved name, or, if this really is the removed member coming back unchanged, delete the matching [WProtoReserved] in the same commit.", + "WallstopProto", + DiagnosticSeverity.Error, + isEnabledByDefault: true + ); + internal static readonly DiagnosticDescriptor HookSignature = new DiagnosticDescriptor( "WPROTO008", "WallstopProto lifecycle hook has the wrong signature", diff --git a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoGenerator.cs b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoGenerator.cs index db74592b..3e4f2e31 100644 --- a/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoGenerator.cs +++ b/Generator~/WallstopStudios.UnityHelpers.Proto.Generator/WProtoGenerator.cs @@ -2903,6 +2903,7 @@ SubtypeMap subtypes { List includes = new List(); HashSet claimed = new HashSet(); + ReservedMap reserved = ReservedMap.Build(contract); foreach (Member member in members) { claimed.Add(member.Tag); @@ -2956,6 +2957,22 @@ SubtypeMap subtypes + tag + " is outside 1-536870911 or inside the reserved 19000-19999 range"; } + else if ( + subtypes.Manifest.TryRetired(contract, tag, out string retiredBy) + && retiredBy != subType.ToDisplayString() + ) + { + // The two declaration forms share one field-number space, so a rule that + // covered only [WProtoSubtype] would be one an author steps around by + // accident (#606). + problem = SubtypeMap.RetiredProblem(tag, contract, retiredBy); + } + else if (reserved.ReservesNumber(tag)) + { + // Checked before claimed.Add so a refused include does not spend the number it + // was refused for. + problem = ReservedMap.ReservedProblem(tag, contract.Name); + } else if (!claimed.Add(tag)) { problem = @@ -3000,6 +3017,21 @@ SubtypeMap subtypes continue; } + if (reserved.ReservesNumber(declared.Tag)) + { + context.ReportDiagnostic( + Diagnostic.Create( + WProtoDiagnostics.BadSubtype, + declared.SubType.Locations.FirstOrDefault(), + declared.SubType.Name, + SubtypeMap.Written(contract, declared.Tag, declared.TagFromManifest), + ReservedMap.ReservedProblem(declared.Tag, contract.Name) + ) + ); + failed = true; + continue; + } + if (!claimed.Add(declared.Tag)) { context.ReportDiagnostic( @@ -3123,6 +3155,7 @@ NestedCollections nested { List members = new List(); Dictionary claimed = new Dictionary(); + ReservedMap reserved = ReservedMap.Build(contract); bool failed = false; foreach (ISymbol symbol in contract.GetMembers()) @@ -3183,6 +3216,35 @@ NestedCollections nested continue; } + // Checked after the duplicate, because a member colliding with a LIVE sibling has a + // fix the author can see in front of them; a collision with something deleted needs + // the reservation explained. + // + // The name a CONSUMER sees, and only that. A generated schema, a payload dump and + // anything matching by name all read [WProtoMember(Name = ...)] where it is set and + // the member's own name where it is not, so that is the identity a reservation + // protects. Reading the C# identifier as well would refuse a member presenting a + // free name, which is the decoupling Name exists for. + string schemaName = SchemaNameOf(attribute) ?? symbol.Name; + bool reservedName = reserved.ReservesName(schemaName); + if (reserved.ReservesNumber(tag) || reservedName) + { + Report( + context, + WProtoDiagnostics.ReservedTag, + symbol, + contract.Name, + symbol.Name, + reserved.ReservesNumber(tag) + ? reservedName + ? "field number " + tag + " and the name '" + schemaName + "'" + : "field number " + tag + : "the name '" + schemaName + "'" + ); + failed = true; + continue; + } + bool zigZag = AsksForZigZag(attribute); if (zigZag && !Shape.SupportsZigZag(type)) { @@ -3335,6 +3397,29 @@ params object[] arguments ); } + /// + /// The schema name a member declared for itself, or null when it declared none. + /// + /// The member's [WProtoMember]. + /// The declared name, or null. + /// + /// Never written to the wire -- protobuf identifies fields by number -- but it is what a + /// generated schema, a payload dump and anything matching by name see, which is exactly + /// what a reserved name protects. + /// + private static string SchemaNameOf(AttributeData attribute) + { + foreach (KeyValuePair argument in attribute.NamedArguments) + { + if (argument.Key == "Name" && argument.Value.Value is string declared) + { + return string.IsNullOrEmpty(declared) ? null : declared; + } + } + + return null; + } + /// /// Reports whether the attribute asks for DataFormat = ZigZag. /// diff --git a/Runtime/Analyzers/WallstopStudios.UnityHelpers.Proto.Generator.dll b/Runtime/Analyzers/WallstopStudios.UnityHelpers.Proto.Generator.dll index 9965697b..914fb2c3 100644 Binary files a/Runtime/Analyzers/WallstopStudios.UnityHelpers.Proto.Generator.dll and b/Runtime/Analyzers/WallstopStudios.UnityHelpers.Proto.Generator.dll differ diff --git a/Runtime/Core/Serialization/WallstopProto/WProtoReservedAttribute.cs b/Runtime/Core/Serialization/WallstopProto/WProtoReservedAttribute.cs new file mode 100644 index 00000000..73b910f9 --- /dev/null +++ b/Runtime/Core/Serialization/WallstopProto/WProtoReservedAttribute.cs @@ -0,0 +1,101 @@ +// MIT License - Copyright (c) 2026 wallstop +// Full license text: https://github.com/wallstop/unity-helpers/blob/main/LICENSE + +namespace WallstopStudios.UnityHelpers.Core.Serialization.WallstopProto +{ + using System; + using System.Collections.Generic; + using UnityEngine.Scripting; + + /// + /// Field numbers, or member names, that a removed [WProtoMember] used to hold and that + /// nothing on this contract may take again. + /// + /// + /// + /// A field number is a durable wire contract, and the declaration that spends one is deleted + /// along with the member it sits on. WPROTO002 refuses two members claiming one number at + /// the same TIME and so has no memory: a number freed by a deletion is indistinguishable from + /// one never used, and handing it to a later member reads every payload written by an older + /// build as the wrong type. This is the record that makes the deletion visible, and it is the + /// same mechanism proto3 spells reserved. + /// + /// + /// [WProtoContract] + /// [WProtoReserved(3)] // Health, removed in 4.0 + /// [WProtoReserved(7, 9)] // several at once + /// [WProtoReserved("Health")] // and the name it went by + /// public partial class Player { } + /// + /// + /// Names are reserved as well as numbers, for the reason protobuf reserves both: a re-added + /// Health at a different number still breaks anything that matches by name -- a JSON + /// projection, a generated .proto consumer, a schema registry -- while carrying data that + /// means something else. + /// + /// + /// The record is an attribute rather than a generated manifest because a member number is always + /// written by hand. Nothing assigns one, so the record belongs beside the contract where the + /// next author is already reading, and a reservation that contradicts a live member is refused + /// rather than silently outranking it. + /// + /// + [Preserve] + [AttributeUsage( + AttributeTargets.Class | AttributeTargets.Struct, + AllowMultiple = true, + Inherited = false + )] + public sealed class WProtoReservedAttribute : Attribute + { + private static readonly int[] NoNumbers = Array.Empty(); + private static readonly string[] NoNames = Array.Empty(); + + /// + /// Reserves one or more field numbers. + /// + /// A field number no member may take again. + /// Any further numbers to reserve in the same declaration. + /// + /// The first number is separate from the rest so that [WProtoReserved()] cannot + /// compile. An empty reservation reads as a considered decision and records nothing, which + /// is the state this attribute exists to prevent. + /// + public WProtoReservedAttribute(int fieldNumber, params int[] alsoReserved) + { + int[] numbers = new int[1 + (alsoReserved == null ? 0 : alsoReserved.Length)]; + numbers[0] = fieldNumber; + for (int index = 1; index < numbers.Length; index++) + { + numbers[index] = alsoReserved[index - 1]; + } + + FieldNumbers = numbers; + MemberNames = NoNames; + } + + /// + /// Reserves one or more member names. + /// + /// A member name no member may take again. + /// Any further names to reserve in the same declaration. + public WProtoReservedAttribute(string memberName, params string[] alsoReserved) + { + string[] names = new string[1 + (alsoReserved == null ? 0 : alsoReserved.Length)]; + names[0] = memberName; + for (int index = 1; index < names.Length; index++) + { + names[index] = alsoReserved[index - 1]; + } + + MemberNames = names; + FieldNumbers = NoNumbers; + } + + /// The field numbers this declaration holds; empty when it reserves names. + public IReadOnlyList FieldNumbers { get; } + + /// The member names this declaration holds; empty when it reserves numbers. + public IReadOnlyList MemberNames { get; } + } +} diff --git a/Runtime/Core/Serialization/WallstopProto/WProtoReservedAttribute.cs.meta b/Runtime/Core/Serialization/WallstopProto/WProtoReservedAttribute.cs.meta new file mode 100644 index 00000000..ad126e8f --- /dev/null +++ b/Runtime/Core/Serialization/WallstopProto/WProtoReservedAttribute.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b2fa806d44b37035c53cac6b42f49ea3 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Runtime/Core/Serialization/WallstopProto/WProtoSchemaText.cs b/Runtime/Core/Serialization/WallstopProto/WProtoSchemaText.cs index 092d1e82..6deb7a6f 100644 --- a/Runtime/Core/Serialization/WallstopProto/WProtoSchemaText.cs +++ b/Runtime/Core/Serialization/WallstopProto/WProtoSchemaText.cs @@ -393,6 +393,7 @@ public string TryAddContract(Type contractType) List members = CollectMembers(contractType, messageName); StringBuilder body = new StringBuilder(); body.Append("message ").Append(messageName).Append(" {").Append("\n"); + AppendReserved(contractType, messageName, body); foreach (MemberEntry member in members) { string line = RenderField(messageName, member); @@ -425,6 +426,111 @@ public string TryAddContract(Type contractType) return messageName; } + /// + /// Writes the contract's [WProtoReserved] declarations as proto3 reservations. + /// + /// The contract being rendered. + /// Its schema name, for diagnostics. + /// The message body being built. + /// + /// Without these the exported schema permits, in the consumer's own toolchain, exactly + /// the reuse the generator refuses here -- so a removed member's number would come back + /// meaning something else one build system over. + /// + private void AppendReserved(Type contractType, string messageName, StringBuilder body) + { + object[] markers = contractType.GetCustomAttributes( + typeof(WProtoReservedAttribute), + false + ); + if (markers.Length == 0) + { + return; + } + + SortedSet numbers = new SortedSet(); + SortedSet names = new SortedSet(StringComparer.Ordinal); + foreach (object marker in markers) + { + WProtoReservedAttribute reserved = marker as WProtoReservedAttribute; + if (reserved == null) + { + continue; + } + + foreach (int number in reserved.FieldNumbers) + { + // A number proto3 could not have used is not a number this schema can + // reserve: protoc rejects both ends of the range and owns 19000-19999 + // itself, so emitting one would make the whole file impossible to parse. + if ( + 1 <= number + && number <= 536870911 + && (number < 19000 || 19999 < number) + ) + { + numbers.Add(number); + } + else + { + _diagnostics.Add( + $"{messageName}: reserved field number {number.ToString(CultureInfo.InvariantCulture)} is outside the range proto3 allows; omitted." + ); + } + } + + foreach (string name in reserved.MemberNames) + { + if (!string.IsNullOrEmpty(name) && IsValidProtoIdentifier(name)) + { + names.Add(name); + } + else + { + _diagnostics.Add( + $"{messageName}: reserved name '{name}' is not a valid proto3 identifier; omitted." + ); + } + } + } + + if (0 < numbers.Count) + { + body.Append(" reserved "); + bool first = true; + foreach (int number in numbers) + { + if (!first) + { + body.Append(", "); + } + + body.Append(number.ToString(CultureInfo.InvariantCulture)); + first = false; + } + + body.Append(";").Append("\n"); + } + + if (0 < names.Count) + { + body.Append(" reserved "); + bool first = true; + foreach (string name in names) + { + if (!first) + { + body.Append(", "); + } + + body.Append('"').Append(name).Append('"'); + first = false; + } + + body.Append(";").Append("\n"); + } + } + private string RenderField(string ownerName, MemberEntry member) { Type memberType = member.MemberType; diff --git a/Tests/Editor/Validation/ValidationReportingTests.cs b/Tests/Editor/Validation/ValidationReportingTests.cs new file mode 100644 index 00000000..1226a329 --- /dev/null +++ b/Tests/Editor/Validation/ValidationReportingTests.cs @@ -0,0 +1,535 @@ +// MIT License - Copyright (c) 2026 wallstop +// Full license text: https://github.com/wallstop/unity-helpers/blob/main/LICENSE + +namespace WallstopStudios.UnityHelpers.Tests.Editor.Validation +{ + using System; + using System.Collections.Generic; + using NUnit.Framework; + using UnityEngine; + using WallstopStudios.UnityHelpers.Editor.Validation.Continuous; + using WallstopStudios.UnityHelpers.Tests.Core; + using Object = UnityEngine.Object; + + /// + /// Pins the headless half of the validation engine: what a suppression file means, what the + /// JSON report says, and what makes a batch run exit non-zero. + /// + /// + /// Everything here is driven from constructed findings and an injected loader rather than from + /// the asset database, so the assertions are about the reporting contract rather than about + /// whatever assets the test project happens to hold. + /// + [TestFixture] + public sealed class ValidationReportingTests : CommonTestBase + { + private const string FirstGuid = "00000000000000000000000000000001"; + private const string SecondGuid = "00000000000000000000000000000002"; + + [Test] + public void ParsingIgnoresBlankLinesCommentsAndDuplicates() + { + ValidationSuppressions suppressions = ValidationSuppressions.Parse( + "# a comment\n\n Rule|" + + FirstGuid + + "|\n" + + " # an indented comment\n" + + "Rule|" + + FirstGuid + + "|\n" + ); + + CollectionAssert.AreEqual(new[] { "Rule|" + FirstGuid + "|" }, suppressions.Ids); + Assert.AreEqual(1, suppressions.Count); + } + + [TestCase(null)] + [TestCase("")] + [TestCase("# nothing but a comment\n")] + public void AFileWithNoEntriesSuppressesNothing(string text) + { + ValidationSuppressions suppressions = ValidationSuppressions.Parse(text); + + Assert.AreEqual(0, suppressions.Count); + Assert.IsFalse(suppressions.IsSuppressed(Finding("Rule", FirstGuid, null))); + } + + [Test] + public void SuppressionSurvivesAMoveAndAReword() + { + // The identity excludes the path and the message precisely so this holds. A suppression + // that came back the moment somebody moved an asset would be worse than none, because + // the reader would believe the decision had been made. + ValidationSuppressions suppressions = ValidationSuppressions.Parse( + ValidationSuppressions.Render( + new List + { + Finding("Rule", FirstGuid, null, "Assets/Old.asset", "the old wording"), + } + ) + ); + + Assert.IsTrue( + suppressions.IsSuppressed( + Finding("Rule", FirstGuid, null, "Assets/New/Moved.asset", "reworded entirely") + ) + ); + } + + [Test] + public void SuppressionDoesNotCrossRulesAssetsOrDiscriminators() + { + ValidationSuppressions suppressions = ValidationSuppressions.Parse( + ValidationSuppressions.Render( + new List { Finding("Rule", FirstGuid, "field") } + ) + ); + + Assert.IsTrue(suppressions.IsSuppressed(Finding("Rule", FirstGuid, "field"))); + Assert.IsFalse(suppressions.IsSuppressed(Finding("Other", FirstGuid, "field"))); + Assert.IsFalse(suppressions.IsSuppressed(Finding("Rule", SecondGuid, "field"))); + Assert.IsFalse(suppressions.IsSuppressed(Finding("Rule", FirstGuid, "otherField"))); + } + + [Test] + public void ARenderedFileNamesTheAssetAndMessageForAReviewer() + { + // A rule name and a GUID tell a reviewer nothing about what is being switched off, and + // the file exists to be reviewed. The comment is not decoration. + string rendered = ValidationSuppressions.Render( + new List + { + Finding("Rule", FirstGuid, null, "Assets/Audio/Theme.wav", "not streaming"), + } + ); + + StringAssert.Contains("Assets/Audio/Theme.wav", rendered); + StringAssert.Contains("not streaming", rendered); + Assert.AreEqual(1, ValidationSuppressions.Parse(rendered).Count); + } + + [Test] + public void ARenderedFileFlattensAMultiLineMessageOntoItsComment() + { + // A message carrying a newline would otherwise put its own second line into the file as + // an entry, which then suppresses nothing and reads as a decision somebody made. + string rendered = ValidationSuppressions.Render( + new List + { + Finding("Rule", FirstGuid, null, "Assets/A.asset", "first\nsecond"), + } + ); + + CollectionAssert.AreEqual( + new[] { "Rule|" + FirstGuid + "|" }, + ValidationSuppressions.Parse(rendered).Ids + ); + } + + [Test] + public void AnEntryThatMatchesNothingIsReported() + { + // A suppression that outlives its finding reads as a considered decision and is really + // a line nobody has looked at, so the run says so rather than letting the file grow. + ValidationSuppressions suppressions = ValidationSuppressions.Parse( + "Rule|" + FirstGuid + "|\nGone|" + SecondGuid + "|\nnot even an id\n" + ); + + CollectionAssert.AreEqual( + new[] { "Gone|" + SecondGuid + "|", "not even an id" }, + suppressions.UnusedIn( + new List { Finding("Rule", FirstGuid, null) } + ) + ); + } + + [Test] + public void TheReportKeepsASuppressedFindingAndMarksIt() + { + // Dropping it would make a project with a suppression file indistinguishable from one + // with nothing wrong, which is the difference a reviewer needs to see. + ValidationRun run = RunOver( + Finding("Rule", FirstGuid, null, "Assets/A.asset", "silenced"), + Finding("Rule", SecondGuid, null, "Assets/B.asset", "loud") + ); + ValidationSuppressions suppressions = ValidationSuppressions.Parse( + "Rule|" + FirstGuid + "|" + ); + + ValidationReport.Document document = Read(ValidationReport.ToJson(run, suppressions)); + + Assert.AreEqual(ValidationReport.SchemaVersion, document.schemaVersion); + Assert.AreEqual(1, document.unsuppressedCount); + Assert.AreEqual(2, document.findings.Count); + Assert.IsTrue( + document.findings.Exists(record => + record.suppressed && record.message == "silenced" + ) + ); + Assert.IsTrue( + document.findings.Exists(record => !record.suppressed && record.message == "loud") + ); + } + + [Test] + public void TheReportSurvivesANullRunAndNullSuppressions() + { + // The batch path renders whatever it got. A report generator that threw on an empty + // project would fail the build for the one state that is unambiguously fine. + ValidationReport.Document document = Read(ValidationReport.ToJson(null, null)); + + Assert.AreEqual(0, document.assetsConsidered); + Assert.AreEqual(0, document.unsuppressedCount); + Assert.IsEmpty(document.findings); + Assert.IsEmpty(document.failures); + } + + [Test] + public void TheReportEscapesAMessageThatWouldBreakTheDocument() + { + // Rendered through JsonUtility precisely so this is Unity's problem rather than a + // hand-rolled writer's, and asserted so a later "simplification" cannot take it away. + ValidationRun run = RunOver( + Finding("Rule", FirstGuid, null, "Assets/A.asset", "he said \"stop\"\nthen \\left") + ); + + // Round-tripped rather than pattern-matched: a document that reads back with the exact + // message is the property, and a check for a backslash would pass on a document no + // reader could parse. + ValidationReport.Document document = Read( + ValidationReport.ToJson(run, ValidationSuppressions.Empty) + ); + + Assert.AreEqual(1, document.findings.Count); + Assert.AreEqual("he said \"stop\"\nthen \\left", document.findings[0].message); + } + + [TestCase(ValidationSeverity.Info, ValidationSeverity.Warning, false)] + [TestCase(ValidationSeverity.Warning, ValidationSeverity.Warning, true)] + [TestCase(ValidationSeverity.Error, ValidationSeverity.Warning, true)] + [TestCase(ValidationSeverity.Error, ValidationSeverity.Error, true)] + [TestCase(ValidationSeverity.Warning, ValidationSeverity.Error, false)] + public void OnlyFindingsAtOrAboveTheThresholdBlock( + ValidationSeverity found, + ValidationSeverity threshold, + bool expected + ) + { + ValidationRun run = RunOver( + Finding("Rule", FirstGuid, null, "Assets/A.asset", "message", found) + ); + + Assert.AreEqual( + expected, + ValidationReport.HasBlockingResults(run, ValidationSuppressions.Empty, threshold) + ); + } + + [Test] + public void ASuppressedFindingDoesNotBlock() + { + ValidationRun run = RunOver( + Finding("Rule", FirstGuid, null, "Assets/A.asset", "message") + ); + + Assert.IsFalse( + ValidationReport.HasBlockingResults( + run, + ValidationSuppressions.Parse("Rule|" + FirstGuid + "|"), + ValidationSeverity.Info + ) + ); + } + + [Test] + public void ARuleThatThrewBlocksWhateverTheThresholdIs() + { + // It produced no answer for that asset, which is not the same as answering "nothing + // wrong". A build that passed on it would be reporting coverage the run does not have. + ValidationRun run = new ValidationRun( + new List { new ThrowingRule() }, + new List + { + new ValidationTarget(FirstGuid, "Assets/A.asset", typeof(ScriptableObject)), + }, + Never + ); + while (!run.Step(double.MaxValue)) { } + + Assert.IsEmpty(run.Findings); + Assert.AreEqual(1, run.Failures.Count); + Assert.IsTrue( + ValidationReport.HasBlockingResults( + run, + ValidationSuppressions.Empty, + ValidationSeverity.Error + ) + ); + ValidationReport.Document document = Read(ValidationReport.ToJson(run, null)); + Assert.AreEqual(1, document.failures.Count); + Assert.IsFalse(document.failures[0].loadFailure, "a rule threw, not the loader"); + Assert.AreEqual("Tests.Throwing", document.failures[0].ruleId); + } + + [TestCase(null, ValidationSeverity.Error)] + [TestCase("", ValidationSeverity.Error)] + [TestCase("nonsense", ValidationSeverity.Error)] + [TestCase("warning", ValidationSeverity.Warning)] + [TestCase("WARNING", ValidationSeverity.Warning)] + [TestCase("Info", ValidationSeverity.Info)] + public void AnUnrecognizedThresholdFallsBackToTheStrictOne( + string written, + ValidationSeverity expected + ) + { + // A typo must not quietly turn the gate off, so the fallback is the strict end. + Assert.AreEqual( + expected, + ValidationBatch.ParseSeverity(written, ValidationSeverity.Error) + ); + } + + [Test] + public void CommandLineValuesAreReadInOrderAndTolerateATrailingFlag() + { + string[] commandLine = + { + "Unity", + ValidationBatch.FolderArgument, + "Assets/A", + ValidationBatch.FolderArgument, + "Assets/B", + ValidationBatch.OutputArgument, + "out.json", + ValidationBatch.SuppressionsArgument, + }; + + CollectionAssert.AreEqual( + new[] { "Assets/A", "Assets/B" }, + ValidationBatch.ValuesOf(commandLine, ValidationBatch.FolderArgument) + ); + Assert.AreEqual( + "out.json", + ValidationBatch.ValueOf(commandLine, ValidationBatch.OutputArgument) + ); + Assert.IsTrue( + ValidationBatch.ValueOf(commandLine, ValidationBatch.SuppressionsArgument) == null, + "a flag with no value after it must not read past the end of the array" + ); + Assert.IsTrue(ValidationBatch.ValueOf(null, ValidationBatch.OutputArgument) == null); + } + + [Test] + public void ARunThatWalkedNothingIsNotAPass() + { + // The same shape this repository refuses everywhere else: a gate that checked nothing + // exits 0 unless something says so. A -validationFolder naming a renamed directory is + // skipped silently by ValidationTargets.Enumerate, so this is reachable with nothing + // looking wrong at the call site. + CollectionAssert.IsEmpty( + ValidationBatch.CoverageProblems(2, 17, null), + "a run with rules and assets measured something" + ); + + Assert.AreEqual( + 1, + ValidationBatch.CoverageProblems(2, 0, null).Count, + "no assets is a run that proved nothing" + ); + Assert.AreEqual( + 1, + ValidationBatch.CoverageProblems(0, 17, null).Count, + "no rules is a run that proved nothing" + ); + Assert.AreEqual( + 2, + ValidationBatch.CoverageProblems(0, 0, null).Count, + "and both are reported, so fixing one does not hide the other" + ); + } + + [Test] + public void AnEmptyRunNamesTheFoldersItWasGiven() + { + // Without the folders in the message the reader cannot tell "the project is empty" + // from "I typed the path wrong", which is the only actionable difference. + string problem = ValidationBatch + .CoverageProblems(1, 0, new List { "Assets/Typo", "Assets/Audio" }) + .Find(entry => entry.Contains("no assets")); + + StringAssert.Contains("Assets/Typo", problem); + StringAssert.Contains("Assets/Audio", problem); + StringAssert.Contains(ValidationBatch.FolderArgument, problem); + } + + [Test] + public void EveryConstructibleRuleIsFoundInAStableOrder() + { + List problems = new List(); + + List first = ValidationBatch.DiscoverRules(problems); + List second = ValidationBatch.DiscoverRules(null); + + CollectionAssert.AreEqual( + Names(first), + Names(second), + "TypeCache's order is not a property of the project, so discovery has to impose one" + ); + Assert.IsTrue( + first.Exists(rule => + string.Equals(rule.RuleId, "Tests.Throwing", StringComparison.Ordinal) + ), + "this fixture's constructible rule has to be found, or the assertion is vacuous" + ); + } + + [Test] + public void ARuleWithNoParameterlessConstructorIsReportedRatherThanEndingTheRun() + { + // One rule that cannot be built must not hide every other rule's findings, and a silent + // skip would report a clean project nobody had actually checked. ScriptedRule below + // takes its findings as a constructor argument, so it is exactly that shape. + List problems = new List(); + + List rules = ValidationBatch.DiscoverRules(problems); + + Assert.IsTrue( + problems.Exists(problem => problem.Contains(nameof(ScriptedRule))), + "expected the unconstructible rule to be reported, got: " + + string.Join(" | ", problems) + ); + Assert.IsTrue( + rules.Exists(rule => + string.Equals(rule.RuleId, "Tests.Throwing", StringComparison.Ordinal) + ), + "and its neighbours still have to be constructed" + ); + } + + /// + /// Reads a rendered report back, which is what makes the assertions about content rather + /// than about how Unity happens to indent. + /// + /// The rendered document. + /// The parsed document; never null. + private static ValidationReport.Document Read(string json) + { + ValidationReport.Document document = JsonUtility.FromJson( + json + ); + Assert.IsTrue( + document != null, + "the report has to be a document a reader can parse: " + json + ); + return document; + } + + private static string[] Names(List rules) + { + List names = new List(); + for (int index = 0; index < rules.Count; index++) + { + names.Add(rules[index].GetType().FullName); + } + + return names.ToArray(); + } + + private static ValidationFinding Finding( + string ruleId, + string guid, + string discriminator, + string path = "Assets/Asset.asset", + string message = "message", + ValidationSeverity severity = ValidationSeverity.Error + ) + { + return new ValidationFinding( + ruleId, + severity, + null, + guid, + path, + discriminator, + message + ); + } + + /// + /// A finished run whose findings are exactly those given. + /// + /// What the run should report. + /// The completed run. + private static ValidationRun RunOver(params ValidationFinding[] findings) + { + List targets = new List + { + new ValidationTarget(FirstGuid, "Assets/Only.asset", typeof(ScriptableObject)), + }; + ValidationRun run = new ValidationRun( + new List { new ScriptedRule(findings) }, + targets, + Never + ); + while (!run.Step(double.MaxValue)) { } + + return run; + } + + private static Object Never(ValidationTarget target) + { + return null; + } + + /// A rule that reports whatever the fixture handed it, once. + private sealed class ScriptedRule : IValidationRule + { + private readonly ValidationFinding[] _findings; + + internal ScriptedRule(ValidationFinding[] findings) + { + _findings = findings; + } + + public string RuleId => "Tests.Scripted"; + + public string DisplayName => "Scripted"; + + public bool AppliesTo(in ValidationTarget target) + { + return true; + } + + public void Validate( + in ValidationTarget target, + Object asset, + List findings + ) + { + findings.AddRange(_findings); + } + } + + /// A rule that throws, so a failure can be asserted without an asset. + private sealed class ThrowingRule : IValidationRule + { + public string RuleId => "Tests.Throwing"; + + public string DisplayName => "Throwing"; + + public bool AppliesTo(in ValidationTarget target) + { + return true; + } + + public void Validate( + in ValidationTarget target, + Object asset, + List findings + ) + { + throw new InvalidOperationException("rule failed"); + } + } + } +} diff --git a/Tests/Editor/Validation/ValidationReportingTests.cs.meta b/Tests/Editor/Validation/ValidationReportingTests.cs.meta new file mode 100644 index 00000000..142c8ba6 --- /dev/null +++ b/Tests/Editor/Validation/ValidationReportingTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: e8747f3b4d49ab987ae2164fcce6d4c1 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Tests/Runtime/Serialization/WProtoSchemaTextTests.cs b/Tests/Runtime/Serialization/WProtoSchemaTextTests.cs index 2c4cd7ca..bc66a5c5 100644 --- a/Tests/Runtime/Serialization/WProtoSchemaTextTests.cs +++ b/Tests/Runtime/Serialization/WProtoSchemaTextTests.cs @@ -49,6 +49,65 @@ out IReadOnlyList diagnostics Assert.IsEmpty(diagnostics, "A fully supported contract must not report anything."); } + [Test] + public void ReservedNumbersAndNamesReachTheSchema() + { + // Without these the exported schema permits, in the consumer's own toolchain, exactly + // the reuse the generator refuses here (#608) -- so a removed member's number would come + // back meaning something else one build system over. + bool rendered = WProtoSchemaText.TryWriteSchema( + new[] { typeof(SchemaReserved) }, + "test.pkg", + null, + out string schema, + out IReadOnlyList diagnostics + ); + + Assert.IsTrue(rendered); + Assert.AreEqual( + GeneratedHeader + + "package test.pkg;\n" + + "\n" + + "message SchemaReserved {\n" + + " reserved 2, 7;\n" + + " reserved \"Armour\", \"Health\";\n" + + " int32 Kept = 1;\n" + + "}\n" + + "\n", + schema, + "The schema is generated output; its exact text is the contract." + ); + Assert.IsEmpty(diagnostics); + } + + [Test] + public void AReservedNumberProtocCouldNotParseIsOmittedAndReported() + { + // protoc rejects both ends of the field-number range and owns 19000-19999 itself, so + // emitting one would make the whole file impossible to parse -- a schema nobody can read is + // worse than one missing a reservation, and the diagnostic says which was dropped. + bool rendered = WProtoSchemaText.TryWriteSchema( + new[] { typeof(SchemaReservedOutOfRange) }, + "test.pkg", + null, + out string schema, + out IReadOnlyList diagnostics + ); + + Assert.IsTrue(rendered); + StringAssert.DoesNotContain("reserved", schema); + CollectionAssert.AreEqual( + new[] + { + "SchemaReservedOutOfRange: reserved field number 0 is outside the range proto3 allows; omitted.", + "SchemaReservedOutOfRange: reserved field number 19500 is outside the range proto3 allows; omitted.", + "SchemaReservedOutOfRange: reserved field number 536870912 is outside the range proto3 allows; omitted.", + }, + diagnostics, + "each dropped number has to be named, or the author cannot tell which record was lost" + ); + } + [Test] public void ScalarMembersMapToTheirWireTypes() { @@ -870,4 +929,21 @@ public sealed partial class SchemaEnumHost [WProtoMember(1)] public SchemaAliased Value; } + + [WProtoContract] + [WProtoReserved(2, 7)] + [WProtoReserved("Health", "Armour")] + public sealed partial class SchemaReserved + { + [WProtoMember(1)] + public int Kept; + } + + [WProtoContract] + [WProtoReserved(0, 19500, 536870912)] + public sealed partial class SchemaReservedOutOfRange + { + [WProtoMember(1)] + public int Kept; + } } diff --git a/Tests/Runtime/WProtoSubtypeTags.cs b/Tests/Runtime/WProtoSubtypeTags.cs index a6176e22..bee52bdd 100644 --- a/Tests/Runtime/WProtoSubtypeTags.cs +++ b/Tests/Runtime/WProtoSubtypeTags.cs @@ -4,10 +4,12 @@ // WallstopProto subtype tag manifest for WallstopStudios.UnityHelpers.Tests.Runtime. // Written by Tools > Wallstop Studios > Unity Helpers > Assign WallstopProto // Subtype Tags. Commit it: these numbers are the wire contract for every -// [WProtoSubtype] declared without one, so a payload saved today is read back by -// this file. Do not renumber an entry, and do not delete a retired one -- a -// retired number is held so a later subtype cannot be given a number old saves -// already mean something else by. +// [WProtoSubtype] in this assembly, so a payload saved today is read back by +// this file. A subtype that wrote its own number is recorded here too, because +// deleting the type deletes the only other record that the number was spent. +// Do not renumber an entry, and do not delete a retired one -- a retired number +// is held so a later subtype cannot be given a number old saves already mean +// something else by. // // The editor rewrites this file automatically after an assembly reload that finds a // [WProtoSubtype] with no number and no entry here, so adding a subtype is one diff --git a/docs/features/editor-tools/asset-validation.md b/docs/features/editor-tools/asset-validation.md index 3b784479..6e3913af 100644 --- a/docs/features/editor-tools/asset-validation.md +++ b/docs/features/editor-tools/asset-validation.md @@ -135,10 +135,68 @@ if (ValidationSeverity.Warning <= finding.Severity) } ``` +## Run it in CI + +Continuous checks only become a guarantee when something other than a person runs them. One +`-executeMethod` runs every rule in the project and exits non-zero when anything stands: + +```bash +Unity -batchmode -quit -projectPath "$PWD" \ + -executeMethod WallstopStudios.UnityHelpers.Editor.Validation.Continuous.ValidationBatch.ValidateFromCommandLine \ + -validationOutput validation.json \ + -validationSuppressions ValidationSuppressions.txt \ + -validationFailOn Warning +``` + +| Argument | Effect | +| ------------------------- | --------------------------------------------------------------- | +| `-validationOutput` | Where to write the JSON report. Omit it and nothing is written. | +| `-validationSuppressions` | The suppression file to apply. Omit it and nothing is silenced. | +| `-validationFailOn` | Lowest severity that fails the run. Defaults to `Error`. | +| `-validationFolder` | Restrict the run to a folder. Repeat it for several. | + +Rules are found through `TypeCache` and built with their parameterless constructor, in a stable +order so two machines produce the same report. A rule that cannot be constructed is reported and +skipped — one broken rule must not hide every other rule's findings — and the run still fails. + +**A rule that threw fails the run whatever the threshold.** It produced no answer for that asset, +which is not the same as answering "nothing wrong", so passing on it would report coverage the run +does not have. + +**A run that checked nothing fails too**, and says which half was empty. No rules, or no assets, is +the absence of a measurement rather than a pass -- and a `-validationFolder` naming a renamed +directory is skipped silently, so a green run over nothing is reachable with nothing looking wrong +at the call site. + +The report carries a `schemaVersion`, the counts, every finding (suppressed ones included and +marked), every failure, and any suppression entry that matched nothing. + +## Suppressions + +A suppression file is one finding identity per line, so a diff shows exactly which check somebody +switched off: + +```text +# Assets/Audio/Theme.wav -- 42.0s clip is not streaming. +MyGame.ClipsMustStream|8f3a5c1d9e2b4a7f8c3d6e1a0b5f4c2d| +``` + +`ValidationSuppressions.Render(findings)` writes one, comments and all. `#` lines and blanks are +ignored, so the comment above each entry is regenerated from the finding rather than parsed. + +Matching is on the finding's identity — rule, asset GUID, discriminator — never the path and never +the message. **Moving the asset or rewording the rule does not un-suppress it.** That is the same +identity findings already have, and the reason it excludes those two fields. + +A run reports entries that matched nothing, in the report's `unusedSuppressions` and in the console +summary. A suppression that outlives the finding it silenced reads as a considered decision and is +really a line nobody has looked at. Only trust that list from a run that covered the whole project: +a run scoped to one folder never saw the assets the other entries name. + ## Not yet -This is the engine, not the whole feature. There is no results window, no suppression that survives -a domain reload, no automatic re-run when an asset changes, and no Test Runner adapter — all tracked -on [issue #288](https://github.com/Ambiguous-Interactive/unity-helpers/issues/288). Scenes and -prefab contents are out of scope for now: a run walks assets, and opening a scene to validate it -needs dirty/open/save semantics that are not settled. +This is the engine and its headless reporting, not the whole feature. There is no results window and +no automatic re-run when an asset changes — both tracked on +[issue #288](https://github.com/Ambiguous-Interactive/unity-helpers/issues/288). Scenes and prefab +contents are out of scope for now: a run walks assets, and opening a scene to validate it needs +dirty/open/save semantics that are not settled. diff --git a/docs/features/serialization/serialization.md b/docs/features/serialization/serialization.md index 47add8ad..eea33927 100644 --- a/docs/features/serialization/serialization.md +++ b/docs/features/serialization/serialization.md @@ -459,7 +459,7 @@ In your `Assets` folder (or any subfolder), create `link.xml` to preserve your P - + @@ -1098,6 +1098,9 @@ everywhere. `AbstractRandom` is the worked example: the after-deserialization wo needs is declared on `AbstractRandom` and dispatched through `OnAfterDeserialization`. Suppress `WPROTO034` at the declaration when the hook only repeats work every other path already does. +`WPROTO043` fires when a member takes a field number, or a name, that the contract reserved. See +[Retiring a member](#retiring-a-member). + `WPROTO039`, `WPROTO040`, `WPROTO041` and `WPROTO042` are specific to declaring a subtype **from the subtype** with `[WProtoSubtype]`. `WPROTO039` fires when two subtypes of one base claim the same field number, whichever end each was declared from, and names both types and the number. @@ -1552,6 +1555,12 @@ the tool enforces all three: take that number, so a payload saved before the deletion cannot come back as some later type. - **Re-adding the type restores its own number.** The retired entry is matched by name and turned back into an assignment. +- **A number you wrote by hand is recorded too.** `[WProtoSubtype(typeof(Weapon), 3)]` gets an + entry beside the assigned ones, because the declaration is otherwise the only record that 3 + was ever spent -- and it is deleted along with the type that carries it. With the entry, the + deletion is seen and 3 is retired; without it, the next subtype added is handed 3 and every + payload written by an older build reads that field back as the wrong type. `[WProtoInclude]` + is the same declaration written on the base and is covered the same way. **The subtype half of an entry is a string, not a `typeof`, and that is what makes retirement possible.** A `typeof` stops compiling the moment the subtype is deleted, and the only cheap repair @@ -1601,15 +1610,102 @@ A `[WProtoSubtype]` must name the annotated type's **immediate** base, which mus `[WProtoContract]` **in the same assembly**, with a field number that is free. Neither type may be generic: one formatter serves every closure of a generic definition, and one field number cannot identify a type that is really as many types as it has closures. Anything else is a build error -(`WPROTO040`) naming the type, the base and what is wrong. The same-assembly rule is -where this feature stops: the base's dispatch chain is generated when the base's own assembly is -compiled, and a declaration made afterwards, in a package that references it, could never appear -there. **The manifest does not change this.** A number is only half the problem: two packages that -never see each other cannot coordinate one, Unity's registrars run unordered so a serialize before -every registrar has run would write under the wrong number or none, and a registry lookup has to -stay IL2CPP-safe. That is a different mechanism with a different failure mode, and it is tracked -separately. Until then, keep a hierarchy inside one assembly, or hold the foreign type behind a -contract of its own rather than as its base. +(`WPROTO040`) naming the type, the base and what is wrong. + +##### Why a hierarchy cannot cross an assembly boundary + +The same-assembly rule is where this feature stops today. The base's dispatch chain is generated when +the base's own assembly is compiled, so a subtype declared afterwards, in an assembly that references +it, is not late to a list -- it is outside the compilation that built the list. **The manifest does +not change this**: a number was never the obstacle, and writing one by hand does not help. + +One way of closing the gap is refused outright. A **runtime registry** has failure modes that are all +silent data corruption rather than build errors: Unity's registrars run unordered, so a serialize +before every registrar has run writes under the wrong number or none; two unrelated packages picking +the same number on a shared base is undetectable at build time and type-confusing at read time; and +the lookup has to stay IL2CPP-safe through managed stripping. A build error you can see is a better +trade than a player that writes an unreadable save. + +A second way is **not** refused, and is tracked on +[issue 612](https://github.com/Ambiguous-Interactive/unity-helpers/issues/612): the extending +assembly emits the base's whole dispatch chain itself, package subtypes included, and registers it in +place of the shipped one. Its compilation can already read every field number the base spends, so a +collision is a build error rather than a runtime surprise, and the dispatch stays the same static +code. Until that exists, `WPROTO040` refuses the declaration. + +Two shapes work instead. Keep the hierarchy inside one assembly -- or, when the base belongs to +somebody else, **compose rather than derive**: + +```csharp +// Refused: Sub is in your assembly, Weapon is in the package's. +[WProtoContract] +[WProtoSubtype(typeof(Weapon), 100)] +public partial class PlasmaCutter : Weapon { } + +// Supported: your type is its own contract and holds a Weapon. +[WProtoContract] +public partial class PlasmaCutter +{ + [WProtoMember(1)] + public Weapon Base; + + [WProtoMember(2)] + public float ChargeSeconds; +} +``` + +A member whose type comes from another assembly is generated normally, and `Weapon` still writes its +own subtypes through the chain that was emitted with it -- so a `Weapon` field holding a package +subtype round-trips as that subtype. What you give up is being _dispatched as_ a `Weapon`: a +collection declared `List` cannot hold a `PlasmaCutter`. Declare the collection as your own +type instead. + +#### Retiring a member + +A field number is a durable wire contract, and the declaration that spends one is deleted along with +the member it sits on. `WPROTO002` refuses two members claiming one number at the same **time**, so +it cannot see a number a deletion freed: delete `Health`, add something else at 3, and every payload +written by an older build reads that field back as the wrong thing, with no diagnostic anywhere. + +Record the removal where the next author is already reading: + +```csharp +[WProtoContract] +[WProtoReserved(3)] // Health, removed in 4.0 +[WProtoReserved(7, 9)] // several at once +[WProtoReserved("Health")] // and the name it went by +public partial class Player +{ + [WProtoMember(1)] + public string Name; +} +``` + +A member that takes a reserved number or a reserved name is `WPROTO043`. **Names are reserved as +well as numbers**, for the reason protobuf reserves both: a re-added `Health` at a _different_ number +still breaks anything matching by name -- a JSON projection, a generated `.proto` consumer, a schema +registry -- while carrying data that means something else. The check reads the name a consumer +actually sees, so `[WProtoMember(9, Name = "Health")]` is refused whatever the C# member is +called, and a C# `Health` presenting itself as something else is not. + +A reservation is a record, not a permanent ban. If the removed member really is coming back +unchanged, delete the matching `[WProtoReserved]` in the same commit; `WPROTO043`'s message says so, +because from the compiler's side "a new member took a dead number" and "a reservation contradicts a +live member" are the same state and nothing there can tell them apart. + +A reservation binds **subtype discriminators too**, not only members. A base's `[WProtoInclude]` and +`[WProtoSubtype]` numbers share one space with its members, so a rule covering half of it would be +one you step around by writing the number on the other half. **Assign WallstopProto Subtype Tags** +knows this as well, and assigns around reserved numbers rather than handing out one the next compile +would reject. + +Reservations are per contract. A base's reservation does not bind its subtypes' OWN members: those +numbers live in a different space, so inheriting one would refuse a member for a collision that +cannot happen. + +The [schema exporter](#exporting-a-proto3-schema) writes them out as proto3 `reserved` lines. Without +that, the exported schema would permit, in a consumer's own toolchain, exactly the reuse this +refuses. #### Surrogates diff --git a/docs/features/utilities/math-and-extensions.md b/docs/features/utilities/math-and-extensions.md index a2f9bda7..412b5733 100644 --- a/docs/features/utilities/math-and-extensions.md +++ b/docs/features/utilities/math-and-extensions.md @@ -1075,7 +1075,7 @@ gets a dictionary. Negative members count normally toward that span, so **The problem:** Enum values often need different names in UI than in code. ```csharp -using WallstopStudios.UnityHelpers.Core.Attribute; +using WallstopStudios.UnityHelpers.Core.Attributes; public enum Difficulty { diff --git a/package.json b/package.json index dd363bb9..61162de2 100644 --- a/package.json +++ b/package.json @@ -133,6 +133,7 @@ "lint:docs": "node ./scripts/run-doc-link-lint.js", "lint:doc-links": "node ./scripts/run-doc-link-lint.js --verbose", "lint:code-samples": "node ./scripts/extract-code-samples.js --extract-only", + "lint:doc-identifiers": "node ./scripts/lint-doc-identifiers.js", "lint:code-samples:verbose": "node ./scripts/extract-code-samples.js --verbose --extract-only", "lint:spelling": "node ./scripts/run-node-bin.js cspell --no-progress --show-suggestions", "lint:spelling:verbose": "node ./scripts/run-node-bin.js cspell --show-suggestions", @@ -202,6 +203,8 @@ "test:validate-hook-permissions": "bash scripts/tests/test-validate-hook-permissions.sh", "test:validate-hook-sync-calls": "pwsh -NoProfile -File scripts/tests/test-validate-hook-sync-calls.ps1", "test:validate-github-pages-css": "bash scripts/tests/test-validate-github-pages-css.sh", + "test:check-code-fence-syntax": "bash scripts/tests/test-check-code-fence-syntax.sh", + "test:lint-doc-identifiers": "node scripts/tests/test-lint-doc-identifiers.js", "test:lint-dependabot": "pwsh -NoProfile -File scripts/tests/test-lint-dependabot.ps1 -VerboseOutput", "test:lint-duplicate-usings": "pwsh -NoProfile -File scripts/tests/test-lint-duplicate-usings.ps1 -VerboseOutput", "test:lint-preserve-attributes": "pwsh -NoProfile -File scripts/tests/test-lint-preserve-attributes.ps1 -VerboseOutput", diff --git a/scripts/check-code-fence-syntax.sh b/scripts/check-code-fence-syntax.sh index 2d78aec1..c3d036d2 100755 --- a/scripts/check-code-fence-syntax.sh +++ b/scripts/check-code-fence-syntax.sh @@ -49,6 +49,10 @@ echo "" # Array to store issues for summary declare -a ISSUE_LIST +# A scan that matched nothing is the absence of a measurement, not a pass (#556): docs/ renamed, +# a corpus moved, or a find that stopped matching all report "no issues found" otherwise. +SCANNED=0 + # Find all markdown files and check for invalid code fence syntax # Pattern matches code fences with language followed by comma and attributes # Examples of invalid patterns: @@ -56,6 +60,7 @@ declare -a ISSUE_LIST # ```rust,no_run # ```python,something while IFS= read -r -d '' mdfile; do + SCANNED=$((SCANNED + 1)) line_num=0 while IFS= read -r line || [[ -n "$line" ]]; do @@ -80,12 +85,25 @@ while IFS= read -r -d '' mdfile; do ISSUES=$((ISSUES + 1)) fi done < "$mdfile" -done < <(find "$DOCS_DIR" -name "*.md" -type f -print0 2>/dev/null) +done < <(find "$DOCS_DIR" -name "*.md" -type f -print0) echo "----------------------------------------" echo "Summary" echo "----------------------------------------" +if [ "$SCANNED" -eq 0 ]; then + printf "${RED}ERROR: No markdown files found under %s${NC}\n" "$DOCS_DIR" + echo "" + echo "The corpus is empty, so this run checked nothing. Point the validator at a" + echo "directory that contains markdown, or fix the path if the docs tree moved." + echo "" + printf "${RED}VALIDATION FAILED${NC}\n" + exit 1 +fi + +printf "Markdown files scanned: ${BLUE}%d${NC}\n" "$SCANNED" +echo "" + if [ "$ISSUES" -eq 0 ]; then printf "${GREEN}No code fence syntax issues found.${NC}\n" echo "" diff --git a/scripts/lint-comparison-direction.js b/scripts/lint-comparison-direction.js index 5c9c9b38..1a12029c 100644 --- a/scripts/lint-comparison-direction.js +++ b/scripts/lint-comparison-direction.js @@ -835,6 +835,17 @@ function main(argv) { } return 1; } + if (files.length === 0) { + // A walk that matched nothing is the absence of a measurement rather than a pass: a renamed + // source root, a moved tree, or a walk that stopped descending all reach this line otherwise + // (#556). Reported whatever the verbosity, because a silent zero is the whole defect. + console.error( + `[comparison-direction] no C# files were found under ${SCAN_ROOTS.join(", ")}, so this run ` + + `checked nothing.` + ); + return 1; + } + if (verbose) { console.log(`[comparison-direction] ${files.length} file(s) clean.`); } diff --git a/scripts/lint-doc-identifiers.js b/scripts/lint-doc-identifiers.js new file mode 100644 index 00000000..d82e455b --- /dev/null +++ b/scripts/lint-doc-identifiers.js @@ -0,0 +1,229 @@ +#!/usr/bin/env node +/** + * Documentation may not name a namespace or an assembly this repository does not have. + * + * `lint:code-samples` extracts 3,061 C# blocks out of `docs/` and validates none of them, so an + * example that names something moved or renamed reads as correct forever. The damage is silent + * twice over: the reader copies it, and the person who moved the namespace gets no signal at all. + * + * TWO RULES, both chosen because they have no false positives. A general "does this API exist" + * check needs a real parser -- a first attempt keyed on `Type.Member` reported + * `Serializer.ProtoDeserialize` as missing, because a regex over declarations cannot see a generic + * method -- and a gate that cries wolf is one people stop reading: + * + * 1. Every `using WallstopStudios.UnityHelpers...;` in a Markdown file must name a namespace some + * `.cs` file under the governed source roots declares. A `using` is unambiguous: it is a + * namespace, spelled in full, or it does not compile. + * 2. Every `` in a `link.xml` example must name an + * assembly some `.asmdef` declares. A wrong name here is worse than a compile error, because + * the linker silently preserves nothing and the failure arrives as a stripped player. + * + * Both found a real defect on the run that introduced them: `Core.Attribute` for `Core.Attributes` + * in the enum display-name example, and `WallstopStudios.UnityHelpers.Runtime` for the runtime + * assembly, which is named `WallstopStudios.UnityHelpers` + * ([#441](https://github.com/Ambiguous-Interactive/unity-helpers/issues/441)). + * + * Exit codes: 0 = every name resolves, 1 = at least one does not. + */ + +"use strict"; + +const fs = require("fs"); +const path = require("path"); + +const REPO_ROOT = path.resolve(__dirname, ".."); +// Overridable so the self-test can point the scan at a fixture tree. Nothing in CI sets it, so the +// default is the only path that ships. +const SCAN_ROOT = process.env.DOC_IDENTIFIER_ROOT + ? path.resolve(process.env.DOC_IDENTIFIER_ROOT) + : REPO_ROOT; + +/** Where a namespace or an assembly may be declared. */ +const SOURCE_ROOTS = ["Runtime", "Editor", "Tests", "Generator~", "Samples~"]; + +/** Where documentation is read from. */ +const DOC_ROOTS = ["docs"]; + +const SKIPPED_DIRECTORIES = new Set(["obj", "bin", "node_modules", "Library", "artifacts"]); + +const USING_PATTERN = /^\s*using\s+(WallstopStudios(?:\.[A-Za-z0-9_]+)*)\s*;/; +const ASSEMBLY_PATTERN = / entry.name.endsWith(extension))) { + found.push(path.join(current, entry.name)); + } + } + } + + return found; +} + +/** + * The namespaces and assembly names this repository actually declares. + * + * @param {string} root Repository root to read. + * @returns {{namespaces: Set, assemblies: Set}} What exists. + */ +function declared(root) { + const namespaces = new Set(); + const assemblies = new Set(); + + for (const sourceRoot of SOURCE_ROOTS) { + for (const file of filesUnder(path.join(root, sourceRoot), [".cs"])) { + const text = fs.readFileSync(file, "utf8"); + NAMESPACE_PATTERN.lastIndex = 0; + let match; + while ((match = NAMESPACE_PATTERN.exec(text)) !== null) { + // Every ancestor too: `using A.B;` is legal wherever `A.B.C` is declared. + const parts = match[1].split("."); + for (let length = 1; length <= parts.length; length++) { + namespaces.add(parts.slice(0, length).join(".")); + } + } + } + + for (const file of filesUnder(path.join(root, sourceRoot), [".asmdef"])) { + try { + const name = JSON.parse(fs.readFileSync(file, "utf8")).name; + if (typeof name === "string" && 0 < name.length) { + assemblies.add(name); + } + } catch { + // An unreadable asmdef is lint-asmdef's subject, not this one's. Skipping it here can only + // produce a false positive further down, which the report names precisely enough to see. + } + } + } + + return { namespaces, assemblies }; +} + +/** + * Checks every documentation file against what the sources declare. + * + * @param {string} root Repository root to scan. + * @returns {{violations: string[], usings: number, assemblies: number}} What was checked and what failed. + */ +function analyze(root) { + const { namespaces, assemblies } = declared(root); + const violations = []; + let usingCount = 0; + let assemblyCount = 0; + let documentCount = 0; + + for (const docRoot of DOC_ROOTS) { + for (const file of filesUnder(path.join(root, docRoot), [".md"])) { + documentCount++; + const relative = path.relative(root, file).split(path.sep).join("/"); + const lines = fs.readFileSync(file, "utf8").split("\n"); + lines.forEach((line, index) => { + const usingMatch = line.match(USING_PATTERN); + if (usingMatch !== null) { + usingCount++; + if (!namespaces.has(usingMatch[1])) { + violations.push( + `${relative}:${index + 1}: 'using ${usingMatch[1]};' names a namespace nothing declares. ` + + `A reader copying this example gets a compile error.` + ); + } + } + + ASSEMBLY_PATTERN.lastIndex = 0; + let assemblyMatch; + while ((assemblyMatch = ASSEMBLY_PATTERN.exec(line)) !== null) { + assemblyCount++; + if (!assemblies.has(assemblyMatch[1])) { + violations.push( + `${relative}:${index + 1}: names an assembly ` + + `no .asmdef declares. The linker preserves nothing under a wrong name and reports ` + + `nothing, so this surfaces as a stripped player rather than as an error.` + ); + } + } + }); + } + } + + return { + violations, + usings: usingCount, + assemblies: assemblyCount, + namespaces: namespaces.size, + documents: documentCount + }; +} + +function main() { + const { violations, usings, assemblies, namespaces, documents } = analyze(SCAN_ROOT); + + // A walk that matched nothing is the absence of a measurement rather than a pass: docs/ renamed, + // a source root moved, or a walk that stopped descending all reach the success line otherwise + // (#556). Both halves are checked, because either one going empty makes every answer vacuous -- + // no documents means nothing was read, and no namespaces means everything would resolve to + // nothing and be reported, or, as here, nothing would be reported at all. + const empty = []; + if (documents === 0) { + empty.push(`no Markdown files under ${DOC_ROOTS.join(", ")}`); + } + + if (namespaces === 0) { + empty.push(`no namespaces declared under ${SOURCE_ROOTS.join(", ")}`); + } + + if (0 < empty.length) { + console.error(`[lint-doc-identifiers] ${empty.join(" and ")}, so this run checked nothing.`); + process.exitCode = 1; + return; + } + + if (0 < violations.length) { + console.error( + `[lint-doc-identifiers] ${violations.length} documentation reference(s) do not resolve:` + ); + for (const violation of violations) { + console.error(` ${violation}`); + } + + process.exitCode = 1; + return; + } + + // The counts are the gate's own red half at a glance: a scan that checked nothing would say so + // here rather than printing the same success line a clean corpus does. + console.log( + `[lint-doc-identifiers] ${usings} package using directive(s) and ${assemblies} assembly ` + + `reference(s) across ${documents} document(s) all resolve.` + ); +} + +if (require.main === module) { + main(); +} + +module.exports = { analyze, declared, filesUnder, SOURCE_ROOTS, DOC_ROOTS }; diff --git a/scripts/lint-doc-identifiers.js.meta b/scripts/lint-doc-identifiers.js.meta new file mode 100644 index 00000000..04ccaed0 --- /dev/null +++ b/scripts/lint-doc-identifiers.js.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: b2e9994f3221ab919a3f1d524733e7a4 +TextScriptImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/scripts/lint-xml-doc-summaries.js b/scripts/lint-xml-doc-summaries.js index ff4105ed..aa9b77fe 100644 --- a/scripts/lint-xml-doc-summaries.js +++ b/scripts/lint-xml-doc-summaries.js @@ -289,6 +289,18 @@ function main() { return; } + if (files.length === 0) { + // A walk that matched nothing is the absence of a measurement rather than a pass: a renamed + // source root, a moved tree, or a walk that stopped descending all reach this line otherwise + // (#556). Reported whatever the verbosity, because a silent zero is the whole defect. + console.error( + `[xml-doc-summaries] no C# files were found under ${SCAN_ROOTS.join(", ")}, so this run ` + + `checked nothing.` + ); + process.exitCode = 1; + return; + } + if (verbose) { console.log(`[xml-doc-summaries] ${files.length} file(s) clean.`); } diff --git a/scripts/run-contract-tests.js b/scripts/run-contract-tests.js index 3744b9a7..b18d6b8e 100644 --- a/scripts/run-contract-tests.js +++ b/scripts/run-contract-tests.js @@ -106,6 +106,16 @@ const CHECKS = [ name: "GitHub Pages CSS validator self-test", run: "npm run test:validate-github-pages-css" }, + { + id: "check-code-fence-syntax", + name: "Code fence syntax gate self-test", + run: "npm run test:check-code-fence-syntax" + }, + { + id: "lint-doc-identifiers", + name: "Documentation identifier linter self-test", + run: "npm run test:lint-doc-identifiers" + }, { id: "lint-dependabot", name: "Dependabot linter self-test", diff --git a/scripts/run-repo-lint.js b/scripts/run-repo-lint.js index bdba90ed..57b8bcce 100644 --- a/scripts/run-repo-lint.js +++ b/scripts/run-repo-lint.js @@ -215,6 +215,11 @@ const CHECKS = [ name: "Documentation code samples", run: "npm run lint:code-samples" }, + { + id: "doc-identifiers", + name: "Documentation namespace and assembly names", + run: "npm run lint:doc-identifiers" + }, // Gaps found while consolidating: these four are in `validate:content` / `validate:local` and so // ran on a developer's machine, but no workflow invoked any of them. They pass today; the point // is that nothing would have said so if they stopped. diff --git a/scripts/tests/test-check-code-fence-syntax.sh b/scripts/tests/test-check-code-fence-syntax.sh new file mode 100755 index 00000000..9853aca9 --- /dev/null +++ b/scripts/tests/test-check-code-fence-syntax.sh @@ -0,0 +1,239 @@ +#!/usr/bin/env bash +# ============================================================================= +# Self-test for scripts/check-code-fence-syntax.sh +# ============================================================================= +# The gate scans a markdown corpus for code fences carrying comma-separated +# attributes (```csharp,ignore), which MkDocs renders as an unknown language +# rather than as C#. Run against the repository's own docs/ it prints +# "VALIDATION PASSED", which is evidence about docs/ and no evidence at all +# that the gate still reports (#556, #604). +# +# It already takes the corpus directory as $1, so no production change was +# needed to make it testable. +# +# Green half: +# - the repository's real docs/ passes +# - fences that are legal stay legal: no language, a language alone, and +# space-separated attributes, which MkDocs does support +# - a comma inside prose or inside a fenced body is not a fence attribute +# +# Red halves, one per way the gate must report, each asserted on the message +# for that specific reason so a fixture tripping a neighbouring rule cannot +# read as covering the one it is named for: +# - a corpus directory that does not exist +# - a corpus that exists but holds no markdown -- an empty scan is the +# absence of a measurement, not a pass +# - a backtick fence with a comma attribute +# - a tilde fence with a comma attribute +# - an indented fence +# - a fence longer than three backticks +# - a file in a subdirectory, so a lost recursion goes red +# - two offences in one corpus are both counted +# - the report names the offending file, line and replacement +# ============================================================================= + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" +GATE="$REPO_ROOT/scripts/check-code-fence-syntax.sh" +REAL_DOCS="$REPO_ROOT/docs" + +WORKSPACE="$(mktemp -d)" +trap 'rm -rf "$WORKSPACE"' EXIT + +PASSED=0 +FAILED=0 +FAILED_NAMES=() + +pass() { + echo " [PASS] $1" + PASSED=$((PASSED + 1)) +} + +fail() { + echo " [FAIL] $1" + echo " $2" + FAILED=$((FAILED + 1)) + FAILED_NAMES+=("$1") +} + +# Each corpus is a fresh directory holding one markdown file, so a red half +# names exactly one offence and cannot borrow a neighbour's. +make_corpus() { + local name="$1" + local relative="$2" + local corpus="$WORKSPACE/$name" + mkdir -p "$corpus/$(dirname "$relative")" + cat > "$corpus/$relative" + printf '%s' "$corpus" +} + +run_gate() { + GATE_OUTPUT="$(bash "$GATE" "$1" 2>&1)" + GATE_EXIT=$? + return 0 +} + +expect_pass() { + local name="$1" + local corpus="$2" + run_gate "$corpus" + if [ "$GATE_EXIT" -ne 0 ]; then + fail "$name" "gate rejected a corpus it must accept (exit $GATE_EXIT): $GATE_OUTPUT" + return + fi + pass "$name" +} + +expect_fail() { + local name="$1" + local corpus="$2" + local expected="$3" + run_gate "$corpus" + if [ "$GATE_EXIT" -eq 0 ]; then + fail "$name" "gate accepted a corpus it must reject: $GATE_OUTPUT" + return + fi + if ! printf '%s' "$GATE_OUTPUT" | grep -qF -- "$expected"; then + fail "$name" "rejected, but not for the reason under test. Expected to contain '$expected'. Got: $GATE_OUTPUT" + return + fi + pass "$name" +} + +echo "" +echo "Running check-code-fence-syntax.sh self-tests" +echo "" + +# -- Green half -------------------------------------------------------------- +expect_pass "the repository docs/ passes" "$REAL_DOCS" + +# The count is the empty-corpus guard's green half: a run that reports a number +# is a run that walked a corpus, which "no issues found" alone cannot show. +run_gate "$REAL_DOCS" +if printf '%s' "$GATE_OUTPUT" | grep -qE 'Markdown files scanned: .*[1-9]'; then + pass "a passing run reports how many files it scanned" +else + fail "a passing run reports how many files it scanned" "expected a non-zero scan count, got: $GATE_OUTPUT" +fi + +LEGAL="$(make_corpus legal ok.md <<'MARKDOWN' +# Legal fences + +```csharp +int value = 1; +``` + +``` +no language at all +``` + +```csharp title="Example.cs" hl_lines="1 2" +int spaced = 2; +``` + +~~~python +value = 3 +~~~ + +Prose may say ```csharp,ignore``` is wrong without being wrong itself, and a +fenced body may contain commas: + +```text +one, two, three +``` +MARKDOWN +)" +expect_pass "legal fences, spaced attributes and prose commas pass" "$LEGAL" + +# -- Red halves -------------------------------------------------------------- +expect_fail "a corpus directory that does not exist is rejected" \ + "$WORKSPACE/absent" "Docs directory not found" + +mkdir -p "$WORKSPACE/no-markdown/nested" +printf 'not markdown\n' > "$WORKSPACE/no-markdown/nested/readme.txt" +expect_fail "a corpus with no markdown is rejected" \ + "$WORKSPACE/no-markdown" "No markdown files found" + +BACKTICK="$(make_corpus backtick guide.md <<'MARKDOWN' +# Guide + +```csharp,ignore +int value = 1; +``` +MARKDOWN +)" +expect_fail "a backtick fence with a comma attribute is rejected" "$BACKTICK" "VALIDATION FAILED" + +run_gate "$BACKTICK" +for fragment in "guide.md:3" '```csharp,ignore' "(remove ',ignore')"; do + if printf '%s' "$GATE_OUTPUT" | grep -qF -- "$fragment"; then + pass "the report names $fragment" + else + fail "the report names $fragment" "not present in: $GATE_OUTPUT" + fi +done + +TILDE="$(make_corpus tilde guide.md <<'MARKDOWN' +~~~python,no_run +value = 1 +~~~ +MARKDOWN +)" +expect_fail "a tilde fence with a comma attribute is rejected" "$TILDE" '```python,no_run' + +INDENTED="$(make_corpus indented guide.md <<'MARKDOWN' +1. A step: + + ```rust,no_run + let value = 1; + ``` +MARKDOWN +)" +expect_fail "an indented fence is rejected" "$INDENTED" '```rust,no_run' + +LONG_FENCE="$(make_corpus long-fence guide.md <<'MARKDOWN' +````markdown,linenums +```csharp +int value = 1; +``` +```` +MARKDOWN +)" +expect_fail "a fence longer than three backticks is rejected" "$LONG_FENCE" '```markdown,linenums' + +NESTED="$(make_corpus nested features/deep/guide.md <<'MARKDOWN' +```csharp,ignore +int value = 1; +``` +MARKDOWN +)" +expect_fail "a file in a subdirectory is scanned" "$NESTED" "features/deep/guide.md:1" + +MULTIPLE="$(make_corpus multiple guide.md <<'MARKDOWN' +```csharp,ignore +int value = 1; +``` + +```rust,no_run +let value = 1; +``` +MARKDOWN +)" +expect_fail "two offences in one corpus are both counted" "$MULTIPLE" "Found 2 code fence syntax issue(s)" + +echo "" +echo "Passed: $PASSED" +echo "Failed: $FAILED" + +if [ "$FAILED" -gt 0 ]; then + echo "" + echo "Failed tests:" + for name in "${FAILED_NAMES[@]}"; do + echo " - $name" + done + exit 1 +fi + +exit 0 diff --git a/scripts/tests/test-check-code-fence-syntax.sh.meta b/scripts/tests/test-check-code-fence-syntax.sh.meta new file mode 100644 index 00000000..d2077104 --- /dev/null +++ b/scripts/tests/test-check-code-fence-syntax.sh.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: 37ee99d6eb496972d86f1d1126b78795 +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/scripts/tests/test-lint-comparison-direction.js b/scripts/tests/test-lint-comparison-direction.js index a63c3cc0..84fc9433 100644 --- a/scripts/tests/test-lint-comparison-direction.js +++ b/scripts/tests/test-lint-comparison-direction.js @@ -150,6 +150,26 @@ runTest("fix: refuses a comparison that spans more than one line", () => { assert.strictEqual(fixed(source), source); }); +runTest("a corpus with no C# files is rejected rather than reported clean", () => { + // A walk that matched nothing is the absence of a measurement, not a pass (#556). Reachable + // with nothing looking wrong: a renamed source root, a moved tree, a walk that stops descending. + const directory = fs.mkdtempSync(path.join(os.tmpdir(), "comparison-direction-empty-")); + try { + const result = spawnSync(process.execPath, [linterPath], { + env: { ...process.env, COMPARISON_DIRECTION_ROOTS: directory }, + encoding: "utf8" + }); + + assert.strictEqual(result.status, 1, "an empty walk must not report a clean run"); + assert.ok( + result.stderr.includes("checked nothing"), + `the report must say what was empty, got: ${result.stderr}` + ); + } finally { + fs.rmSync(directory, { recursive: true, force: true }); + } +}); + runTest("linter exits non-zero on a violation and zero once fixed", () => { const directory = fs.mkdtempSync(path.join(os.tmpdir(), "comparison-direction-")); try { diff --git a/scripts/tests/test-lint-doc-identifiers.js b/scripts/tests/test-lint-doc-identifiers.js new file mode 100644 index 00000000..dd9d04bb --- /dev/null +++ b/scripts/tests/test-lint-doc-identifiers.js @@ -0,0 +1,242 @@ +#!/usr/bin/env node +/** + * Self-test for scripts/lint-doc-identifiers.js. + * + * Run against the repository the linter prints a success line, which is evidence about `docs/` and + * no evidence that it still reports (#556). Every case below drives it over a fixture tree through + * `DOC_IDENTIFIER_ROOT`, so each rule has a red half that fails the suite if the rule stops firing. + * + * Green halves also matter here more than usual, because the whole design claim is "no false + * positives": a `using` of a parent namespace, of a namespace declared only under `Generator~`, and + * a non-package `using` all have to pass. + */ + +"use strict"; + +const assert = require("assert"); +const fs = require("fs"); +const os = require("os"); +const path = require("path"); +const { spawnSync } = require("child_process"); + +const REPO_ROOT = path.resolve(__dirname, "..", ".."); +const LINTER = path.join(REPO_ROOT, "scripts", "lint-doc-identifiers.js"); + +let passed = 0; +let failed = 0; +const failedTests = []; + +function test(name, body) { + try { + body(); + console.log(` [PASS] ${name}`); + passed++; + } catch (error) { + console.log(` [FAIL] ${name}`); + console.log(` ${error.message}`); + failed++; + failedTests.push(name); + } +} + +/** + * Builds a scratch repository and runs the linter over it. + * + * @param {{sources: Record, docs: Record}} tree What to write. + * @returns {{status: number, output: string}} The linter's exit code and combined output. + */ +function run(tree) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "lint-doc-identifiers-")); + try { + for (const [relative, contents] of Object.entries(tree.sources || {})) { + const full = path.join(root, relative); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, contents); + } + + for (const [relative, contents] of Object.entries(tree.docs || {})) { + const full = path.join(root, "docs", relative); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, contents); + } + + const result = spawnSync(process.execPath, [LINTER], { + encoding: "utf8", + env: { ...process.env, DOC_IDENTIFIER_ROOT: root } + }); + + return { + status: result.status, + output: `${result.stdout || ""}${result.stderr || ""}` + }; + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +const NAMESPACE_SOURCE = { + "Runtime/Core/Attributes/Thing.cs": + "namespace WallstopStudios.UnityHelpers.Core.Attributes\n{\n public sealed class Thing { }\n}\n", + "Runtime/WallstopStudios.UnityHelpers.asmdef": '{\n "name": "WallstopStudios.UnityHelpers"\n}\n' +}; + +console.log("\nTesting scripts/lint-doc-identifiers.js...\n"); + +// -- Green half --------------------------------------------------------------- +test("a using that names a declared namespace passes", () => { + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { + "guide.md": "```csharp\nusing WallstopStudios.UnityHelpers.Core.Attributes;\n```\n" + } + }); + + assert.strictEqual(status, 0, output); + assert.ok(/1 package using directive/.test(output), output); +}); + +test("a using of a PARENT namespace passes", () => { + // `using A.B;` is legal wherever `A.B.C` is declared, so the index has to carry the ancestors. + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { "guide.md": "```csharp\nusing WallstopStudios.UnityHelpers.Core;\n```\n" } + }); + + assert.strictEqual(status, 0, output); +}); + +test("a namespace declared only under Generator~ passes", () => { + const { status, output } = run({ + sources: { + ...NAMESPACE_SOURCE, + "Generator~/Gen/Emitter.cs": + "namespace WallstopStudios.UnityHelpers.Proto.Generator\n{\n internal sealed class Emitter { }\n}\n" + }, + docs: { + "guide.md": "```csharp\nusing WallstopStudios.UnityHelpers.Proto.Generator;\n```\n" + } + }); + + assert.strictEqual(status, 0, output); +}); + +test("a using from another vendor is not this linter's business", () => { + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { "guide.md": "```csharp\nusing UnityEngine.Rendering.Universal;\n```\n" } + }); + + assert.strictEqual(status, 0, output); + assert.ok(/0 package using directive/.test(output), output); +}); + +test("an assembly reference that names a real asmdef passes", () => { + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { + "guide.md": '```xml\n\n```\n' + } + }); + + assert.strictEqual(status, 0, output); + assert.ok(/1 assembly reference/.test(output), output); +}); + +// -- Red halves --------------------------------------------------------------- +test("a using that names no declared namespace is reported", () => { + // The real defect this was written for: Core.Attribute for Core.Attributes. + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { "guide.md": "```csharp\nusing WallstopStudios.UnityHelpers.Core.Attribute;\n```\n" } + }); + + assert.strictEqual(status, 1, output); + assert.ok(/Core\.Attribute;/.test(output), output); + assert.ok(/guide\.md:2/.test(output), output); +}); + +test("an assembly reference that names no asmdef is reported", () => { + // The other real defect: the runtime assembly is WallstopStudios.UnityHelpers, and a link.xml + // naming .Runtime preserves nothing while reporting nothing. + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { + "guide.md": '```xml\n\n```\n' + } + }); + + assert.strictEqual(status, 1, output); + assert.ok(/WallstopStudios\.UnityHelpers\.Runtime/.test(output), output); +}); + +test("a nested documentation file is scanned", () => { + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { + "features/deep/guide.md": "```csharp\nusing WallstopStudios.UnityHelpers.Nowhere;\n```\n" + } + }); + + assert.strictEqual(status, 1, output); + assert.ok(/features\/deep\/guide\.md/.test(output), output); +}); + +test("every violation is reported, not just the first", () => { + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { + "a.md": "```csharp\nusing WallstopStudios.UnityHelpers.Nowhere;\n```\n", + "b.md": "```csharp\nusing WallstopStudios.UnityHelpers.AlsoNowhere;\n```\n" + } + }); + + assert.strictEqual(status, 1, output); + assert.ok(/2 documentation reference\(s\)/.test(output), output); +}); + +test("a corpus with no documents is rejected", () => { + // A walk that matched nothing is the absence of a measurement, not a pass (#556). Reachable with + // nothing looking wrong: docs/ renamed, a tree moved, a walk that stopped descending. + const { status, output } = run({ sources: NAMESPACE_SOURCE, docs: {} }); + + assert.strictEqual(status, 1, output); + assert.ok(/no Markdown files/.test(output), output); +}); + +test("a corpus with no declared namespaces is rejected", () => { + // The other half. With nothing indexed every using would resolve to nothing -- so the run would + // either report all of them or, as it did, report none and exit 0. + const { status, output } = run({ + sources: {}, + docs: { "guide.md": "```csharp\nusing WallstopStudios.UnityHelpers.Core.Attributes;\n```\n" } + }); + + assert.strictEqual(status, 1, output); + assert.ok(/no namespaces declared/.test(output), output); +}); + +test("a passing run reports how many documents it read", () => { + const { status, output } = run({ + sources: NAMESPACE_SOURCE, + docs: { + "a.md": "```csharp\nusing WallstopStudios.UnityHelpers.Core.Attributes;\n```\n", + "b.md": "# no code here\n" + } + }); + + assert.strictEqual(status, 0, output); + assert.ok(/across 2 document\(s\)/.test(output), output); +}); + +test("the repository itself passes", () => { + const result = spawnSync(process.execPath, [LINTER], { encoding: "utf8" }); + assert.strictEqual(result.status, 0, `${result.stdout || ""}${result.stderr || ""}`); +}); + +console.log(`\n${passed} passed, ${failed} failed`); +if (0 < failed) { + console.log(`Failed: ${failedTests.join(", ")}`); + process.exit(1); +} + +process.exit(0); diff --git a/scripts/tests/test-lint-doc-identifiers.js.meta b/scripts/tests/test-lint-doc-identifiers.js.meta new file mode 100644 index 00000000..3e30bac6 --- /dev/null +++ b/scripts/tests/test-lint-doc-identifiers.js.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: 3376611d4cd081722e8d7786d62b99e4 +TextScriptImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/scripts/tests/test-lint-xml-doc-summaries.js b/scripts/tests/test-lint-xml-doc-summaries.js index b6eb81e8..63cf2628 100644 --- a/scripts/tests/test-lint-xml-doc-summaries.js +++ b/scripts/tests/test-lint-xml-doc-summaries.js @@ -209,6 +209,26 @@ runTest("CR-only source is analyzed the same as LF", () => { assert.strictEqual(analyzeFile(orphanedBlock.replace(/\n/g, "\r")).length, 1); }); +runTest("a corpus with no C# files is rejected rather than reported clean", () => { + // A walk that matched nothing is the absence of a measurement, not a pass (#556). Reachable + // with nothing looking wrong: a renamed source root, a moved tree, a walk that stops descending. + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), "xml-doc-summaries-empty-")); + try { + const result = spawnSync(process.execPath, [linterPath, "--verbose"], { + encoding: "utf8", + env: { ...process.env, XML_DOC_SUMMARY_ROOTS: scratch } + }); + + assert.strictEqual(result.status, 1, "an empty walk must not report a clean run"); + assert.ok( + result.stderr.includes("checked nothing"), + `the report must say what was empty, got: ${result.stderr}` + ); + } finally { + fs.rmSync(scratch, { recursive: true, force: true }); + } +}); + runTest("the linter exits non-zero on a fixture tree that violates the rule", () => { const scratch = fs.mkdtempSync(path.join(os.tmpdir(), "xml-doc-summaries-")); try { diff --git a/scripts/tests/test-run-repo-lint.js b/scripts/tests/test-run-repo-lint.js index 14462413..6bd4102d 100644 --- a/scripts/tests/test-run-repo-lint.js +++ b/scripts/tests/test-run-repo-lint.js @@ -545,13 +545,11 @@ runTest("no linter in scripts/ has been left unfalsifiable", () => { // map stays, because the two assertions below it are the mechanism that keeps it a work list -- // an entry may not outlive its file, and may not outlive its coverage. // - // Refilled by widening the family below to `check-*`: the rule had never been applied to a - // `check-` gate at all, and one of them has no self-test. This entry is parked on #600, the issue - // whose review surfaced it, and wants an issue of its own -- closing it means adding - // scripts/tests/test-check-code-fence-syntax.sh with a malformed-fence fixture the checker must - // report, and registering it in scripts/run-contract-tests.js so the reachability half below is - // satisfied. - const missingRedHalf = new Map([["scripts/check-code-fence-syntax.sh", "#600"]]); + // Refilled by widening the family below to `check-*`, then emptied again in session 237: + // check-code-fence-syntax.sh was the one `check-` gate with no self-test, and #604 gave it + // scripts/tests/test-check-code-fence-syntax.sh -- which also found the gate reporting a clean + // pass over a corpus holding no markdown at all. + const missingRedHalf = new Map(); // This file and its sibling are REGISTRIES: they name linters in allowlists rather than run // them, so scanning them for a mention counts an excuse as coverage. The first draft did, and