diff --git a/.agents/skills/uloop-hot-reload/references/scope-and-limits.md b/.agents/skills/uloop-hot-reload/references/scope-and-limits.md index 7efb07f29e..aa15b91200 100644 --- a/.agents/skills/uloop-hot-reload/references/scope-and-limits.md +++ b/.agents/skills/uloop-hot-reload/references/scope-and-limits.md @@ -324,6 +324,7 @@ source on disk. When a run skips a method it had patched before, `Warnings` name | Private/internal access inside an async/iterator/closure body has no accessor-delegate shape | Conditional access (`?.`), `??=`, indexers, static field writes, initializer member assignments, compound writes whose receiver could be evaluated twice, assignments whose value is consumed, and calls with `ref`/`out`/`in`, named, optional, or `params` arguments (or to extension/generic/by-ref-returning methods) cannot be rewritten to accessor delegates. Neither can ref-returning properties. These limits apply to compiled members; a `ref`/`out` method added in the same reload is reached directly. A compiled private/internal static property can be read, assigned, and compound-assigned | | An async/iterator/closure body references a private/internal type | Accessor delegates rescue member access, not type references; the body still cannot JIT-compile from the shim assembly | | A declared return or parameter type cannot be resolved (a new type this reload could not introduce, a missing using, or a typo) | Skipped; a supported new type declared in an edited file of the same assembly is introduced by this reload, so check `Warnings` for the refusal reason (`introduced-types.md`); otherwise add the type or the `using`, or fix the typo, then run `uloop compile` | +| An added member's body cannot be fully bound in the hot-reload compilation | Hot reload cannot verify a member it cannot bind. A common cause: another file of the same reload, passed or pulled back in because it holds active patches, declares a compiled type from source, while a compiled API the body calls still names the compiled copy (for example, a lambda handed to a compiled `Register(Action)`). The reason then names both types and the file declaring the compiled API; pass that file to the same reload as well so both bind to the same type, or run `uloop compile` | | Edited setter, init, or indexer accessor of a *compiled* property | Accessor patching covers getters only; `uloop compile` applies these edits. Accessors of a property added in this edit are emitted instead | | Constructor (instance or static), operator, conversion operator, or explicit event accessor (add/remove) | Skipped; `uloop compile` applies these edits | | Method raises or reads a field-like event that has no reachable backing field | Custom `add`/`remove` accessors, an `abstract`/`extern`/interface event, a delegate type that is not visible outside the assembly, or an event added in this edit leave nothing for the shim's Harmony accessor to bind | diff --git a/.claude/skills/uloop-hot-reload/references/scope-and-limits.md b/.claude/skills/uloop-hot-reload/references/scope-and-limits.md index 7efb07f29e..aa15b91200 100644 --- a/.claude/skills/uloop-hot-reload/references/scope-and-limits.md +++ b/.claude/skills/uloop-hot-reload/references/scope-and-limits.md @@ -324,6 +324,7 @@ source on disk. When a run skips a method it had patched before, `Warnings` name | Private/internal access inside an async/iterator/closure body has no accessor-delegate shape | Conditional access (`?.`), `??=`, indexers, static field writes, initializer member assignments, compound writes whose receiver could be evaluated twice, assignments whose value is consumed, and calls with `ref`/`out`/`in`, named, optional, or `params` arguments (or to extension/generic/by-ref-returning methods) cannot be rewritten to accessor delegates. Neither can ref-returning properties. These limits apply to compiled members; a `ref`/`out` method added in the same reload is reached directly. A compiled private/internal static property can be read, assigned, and compound-assigned | | An async/iterator/closure body references a private/internal type | Accessor delegates rescue member access, not type references; the body still cannot JIT-compile from the shim assembly | | A declared return or parameter type cannot be resolved (a new type this reload could not introduce, a missing using, or a typo) | Skipped; a supported new type declared in an edited file of the same assembly is introduced by this reload, so check `Warnings` for the refusal reason (`introduced-types.md`); otherwise add the type or the `using`, or fix the typo, then run `uloop compile` | +| An added member's body cannot be fully bound in the hot-reload compilation | Hot reload cannot verify a member it cannot bind. A common cause: another file of the same reload, passed or pulled back in because it holds active patches, declares a compiled type from source, while a compiled API the body calls still names the compiled copy (for example, a lambda handed to a compiled `Register(Action)`). The reason then names both types and the file declaring the compiled API; pass that file to the same reload as well so both bind to the same type, or run `uloop compile` | | Edited setter, init, or indexer accessor of a *compiled* property | Accessor patching covers getters only; `uloop compile` applies these edits. Accessors of a property added in this edit are emitted instead | | Constructor (instance or static), operator, conversion operator, or explicit event accessor (add/remove) | Skipped; `uloop compile` applies these edits | | Method raises or reads a field-like event that has no reachable backing field | Custom `add`/`remove` accessors, an `abstract`/`extern`/interface event, a delegate type that is not visible outside the assembly, or an event added in this edit leave nothing for the shim's Harmony accessor to bind | diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitE2ETests.cs b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitE2ETests.cs new file mode 100644 index 0000000000..348bfd97b8 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitE2ETests.cs @@ -0,0 +1,141 @@ +using System.Collections.Generic; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +using NUnit.Framework; + +using UnityEngine; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// End-to-end EditMode coverage for the skipped row of an added method that cannot bind + /// because a file the reload pulls in on its own declares a compiled type from source. + /// + public class HotReloadBindingSplitE2ETests + { + private const string HostFileName = "HotReloadBindingSplitHost.cs"; + private const string PayloadFileName = "HotReloadBindingSplitPayload.cs"; + private const string RegistryFileName = "HotReloadBindingSplitRegistry.cs"; + private const string HostInsertionAnchor = " public int Handled => _handled;"; + private const string WireMethod = + "\n\n public void Wire()\n {\n _registry.Register(p => Handle(p));\n }"; + private const string PayloadScaledBody = "return Value * 2;"; + + private HotReloadDomainTestScope _scope; + + [SetUp] + public void SetUp() + { + _scope = new HotReloadDomainTestScope(); + HotReloadAutoRefreshHold.SyncToActiveChanges(); + } + + [TearDown] + public void TearDown() + { + _scope.Dispose(); + HotReloadAutoRefreshHold.SyncToActiveChanges(); + VibeLogger.ClearMemoryLogs(); + } + + /// + /// What: when only the host is passed but the payload file comes back as an active sibling, + /// the added method that cannot bind is skipped with a reason naming the file declaring the + /// compiled API, because the reader never listed the payload file and has no other way to + /// learn which file to pass. + /// + [Test] + public async Task Run_PayloadReappliedAsSibling_SkippedRowNamesTheFileDeclaringTheCompiledSignature() + { + string hostPath = FixturePath(HostFileName); + string payloadPath = FixturePath(PayloadFileName); + string payloadEditPath = HotReloadTestSourceWriter.WriteEditedSource( + "BindingSplitE2EPayload.cs", + ReplaceOnce(File.ReadAllText(payloadPath), PayloadScaledBody, "return Value * 3;")); + + HotReloadOrchestratorResult first = await HotReloadCompositionRoot.Services.Orchestrator.RunAsync( + new[] { payloadPath }, + contentPathOverride: null, + CancellationToken.None, + new Dictionary { [payloadPath] = payloadEditPath }); + Assert.That( + new HotReloadBindingSplitPayload { Value = 2 }.Scaled(), + Is.EqualTo(6), + "Precondition: the payload body must be patched.\n" + FormatOutcomes(first)); + + HotReloadOrchestratorResult second = await HotReloadCompositionRoot.Services.Orchestrator.RunAsync( + new[] { hostPath }, + contentPathOverride: null, + CancellationToken.None, + new Dictionary + { + [hostPath] = HotReloadTestSourceWriter.WriteEditedSource( + "BindingSplitE2EHost.cs", + ReplaceOnce(File.ReadAllText(hostPath), HostInsertionAnchor, HostInsertionAnchor + WireMethod)), + [payloadPath] = payloadEditPath + }); + + Assert.That( + second.ReappliedSiblingPaths, + Is.EquivalentTo(new[] { ProjectRelativePath(PayloadFileName) }), + FormatOutcomes(second)); + HotReloadMethodOutcome skipped = FindSkippedWire(second); + Assert.That(skipped, Is.Not.Null, "Missing skipped row for Wire.\n" + FormatOutcomes(second)); + Assert.That( + skipped.Reason, + Does.Contain("Pass '" + ProjectRelativePath(RegistryFileName) + "'"), + skipped.Reason); + } + + private static HotReloadMethodOutcome FindSkippedWire(HotReloadOrchestratorResult result) + { + foreach (HotReloadMethodOutcome outcome in result.Methods) + { + if (outcome.Kind == HotReloadMethodOutcomeKind.Skipped + && outcome.Method != null + && outcome.Method.Contains(".Wire(")) + { + return outcome; + } + } + + return null; + } + + private static string ReplaceOnce(string source, string anchor, string replacement) + { + Assert.That(source, Does.Contain(anchor), "Precondition: anchor must exist."); + return source.Replace(anchor, replacement); + } + + private static string FixturePath(string fileName) + { + string path = Path.GetFullPath( + Path.Combine(Application.dataPath, "Tests", "Editor", "HotReload", fileName)); + Assert.That(File.Exists(path), Is.True, "Fixture missing: " + path); + return path; + } + + private static string ProjectRelativePath(string fileName) + { + return "Assets/Tests/Editor/HotReload/" + fileName; + } + + private static string FormatOutcomes(HotReloadOrchestratorResult result) + { + List lines = new List(); + foreach (HotReloadMethodOutcome outcome in result.Methods) + { + lines.Add(outcome.Kind + " " + outcome.Method + " @" + outcome.FilePath + " :: " + outcome.Reason); + } + + lines.AddRange(result.Warnings ?? new List()); + return string.Join("\n", lines); + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitE2ETests.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitE2ETests.cs.meta new file mode 100644 index 0000000000..3cff6711a7 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitE2ETests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: ee5196bb3499241519638842d7d4aa91 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitNestedRegistry.cs b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitNestedRegistry.cs new file mode 100644 index 0000000000..9180210280 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitNestedRegistry.cs @@ -0,0 +1,29 @@ +using System; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// Holds a compiled API as a nested type, so a skipped row that names the type declaring a + /// compiled signature has to find the file of a nested type. + /// + public static class HotReloadBindingSplitNestedRegistry + { + /// + /// A compiled API that takes a handler of the payload type, like the top-level registry. + /// + public sealed class Inner + { + private Action _handler; + + public void Register(Action handler) + { + _handler = handler; + } + + public void Raise(int value) + { + _handler?.Invoke(new HotReloadBindingSplitPayload { Value = value }); + } + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitNestedRegistry.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitNestedRegistry.cs.meta new file mode 100644 index 0000000000..e98a47a244 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitNestedRegistry.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: e44c93bdb4216417d836a1a586f52e1f +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayload.cs b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayload.cs index 0e406e4a72..34364753a5 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayload.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayload.cs @@ -7,5 +7,12 @@ namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload public sealed class HotReloadBindingSplitPayload { public int Value; + + // A body a reload can patch, so this file stays active and comes back into a later reload + // of the assembly as a sibling. + public int Scaled() + { + return Value * 2; + } } } diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayloadExtensions.cs b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayloadExtensions.cs new file mode 100644 index 0000000000..030c052cca --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayloadExtensions.cs @@ -0,0 +1,14 @@ +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// A compiled extension on the payload type, so a split can hide in the receiver of a reduced + /// extension call, which is not one of the call's parameters. + /// + public static class HotReloadBindingSplitPayloadExtensions + { + public static int Doubled(this HotReloadBindingSplitPayload payload) + { + return payload.Value * 2; + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayloadExtensions.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayloadExtensions.cs.meta new file mode 100644 index 0000000000..f6fe570a27 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitPayloadExtensions.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: bb37aed72872b4c8ba67ac3d4f3f46cd +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitRegistry.cs b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitRegistry.cs index de1c7e03e2..f3acd75617 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadBindingSplitRegistry.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadBindingSplitRegistry.cs @@ -15,6 +15,8 @@ public void Register(Action handler) _handler = handler; } + public HotReloadBindingSplitPayload this[int value] => new HotReloadBindingSplitPayload { Value = value }; + public void Raise(int value) { _handler?.Invoke(new HotReloadBindingSplitPayload { Value = value }); diff --git a/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs b/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs index 0f45a51302..7079a75e49 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs @@ -275,6 +275,14 @@ private static IEnumerable RenderCases() "The added member's body could not be fully bound in the hot-reload compilation " + "(CS1503: Argument 1: cannot convert); hot reload cannot verify a member it cannot bind, " + "so it is skipped. Run 'uloop compile'."); + yield return Case( + HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature, + new[] { "CS1503: Argument 1: cannot convert", "'Example.Payload'", "'Example.Registry'", "'Assets/Registry.cs'" }, + "The added member's body could not be fully bound in the hot-reload compilation " + + "(CS1503: Argument 1: cannot convert): this reload declares 'Example.Payload' from source, " + + "while the compiled signatures of 'Example.Registry' still name the compiled 'Example.Payload', " + + "so it is skipped. Pass 'Assets/Registry.cs' to this reload as well so both bind to the same type. " + + "Otherwise run 'uloop compile'."); yield return Case( HotReloadWorkerReasonCode.AddedFieldStructHost, NoArgs, diff --git a/Assets/Tests/Editor/HotReload/TransformWorkerBindingSplitTests.cs b/Assets/Tests/Editor/HotReload/TransformWorkerBindingSplitTests.cs index e31595c8d4..d7b14c73d2 100644 --- a/Assets/Tests/Editor/HotReload/TransformWorkerBindingSplitTests.cs +++ b/Assets/Tests/Editor/HotReload/TransformWorkerBindingSplitTests.cs @@ -24,14 +24,36 @@ public class TransformWorkerBindingSplitTests private const string TestAssemblyName = "UnityCLILoop.Tests.Editor.HotReload"; private const string HostFileName = "HotReloadBindingSplitHost.cs"; private const string PayloadFileName = "HotReloadBindingSplitPayload.cs"; + private const string RegistryFileName = "HotReloadBindingSplitRegistry.cs"; private const string HostTypeMetadataName = "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadBindingSplitHost"; + private const string RegistryTypeMetadataName = + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadBindingSplitRegistry"; + private const string PayloadTypeMetadataName = + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadBindingSplitPayload"; + private const string NestedRegistryFileName = "HotReloadBindingSplitNestedRegistry.cs"; + // The metadata form, which is how the worker reports a nested type. + private const string NestedRegistryTypeMetadataName = + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadBindingSplitNestedRegistry/Inner"; + private const string ExtensionsFileName = "HotReloadBindingSplitPayloadExtensions.cs"; private const string InsertionAnchor = " public int Handled => _handled;"; private const string WireMethod = "\n\n public void Wire()\n {\n _registry.Register(p => Handle(p));\n }"; private const string WireMethodThatAlsoCounts = "\n\n public void Wire()\n {\n _registry.Register(p =>\n {\n" + " _handled++;\n Handle(p);\n });\n }"; + private const string CallExtensionMethod = + "\n\n public int Twice(HotReloadBindingSplitPayload payload)\n {\n" + + " return payload.Doubled();\n }"; + private const string PeekThroughIndexerMethod = + "\n\n public int Peek()\n {\n" + + " HotReloadBindingSplitPayload payload = _registry[3];\n return payload.Value;\n }"; + private const string WireWithNullAndTypoMethod = + "\n\n public void Wire()\n {\n _registry.Register(null);\n" + + " UndeclaredName();\n }"; + private const string WireThroughNestedRegistryMethod = + "\n\n public void Wire()\n {\n" + + " new HotReloadBindingSplitNestedRegistry.Inner().Register(p => Handle(p));\n }"; /// /// What: with the host alone in the run, the lambda binds against the compiled payload and @@ -68,7 +90,7 @@ public async Task Run_HostWithThePayloadFile_SkipsTheMethodItCannotBind() Assert.That(FindEntry(result, "Wire"), Is.Null, "Wire must not be applied."); TransformWorkerSkippedDto skipped = FindSkipped(result, "Wire"); Assert.That(skipped, Is.Not.Null, "Missing skipped row for Wire.\n" + FormatSkipped(result)); - Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyUnbound)); + Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature)); } /// @@ -87,7 +109,137 @@ public async Task Run_HostWithThePayloadFile_SkipsTheMethodEvenWhenItsLambdaAlso Assert.That(FindEntry(result, "Wire"), Is.Null, "Wire must not be applied."); TransformWorkerSkippedDto skipped = FindSkipped(result, "Wire"); Assert.That(skipped, Is.Not.Null, "Missing skipped row for Wire.\n" + FormatSkipped(result)); - Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyUnbound)); + Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature)); + } + + /// + /// What: when the added method cannot bind because this run declares the payload from source + /// while a compiled API still takes the compiled payload, the skipped row names the compiled + /// type whose signature holds the old payload and the file that declares it, so the reader + /// knows which file to pass as well instead of only being told to compile. + /// + [Test] + public async Task Run_HostWithThePayloadFile_NamesTheFileDeclaringTheCompiledSignature() + { + TransformWorkerClientResult result = await RunAsync( + new[] { HostFileName, PayloadFileName }, + new[] { WithMethod(ReadOnDisk(HostFileName), WireMethod), ReadOnDisk(PayloadFileName) }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + TransformWorkerSkippedDto skipped = FindSkipped(result, "Wire"); + Assert.That(skipped, Is.Not.Null, "Missing skipped row for Wire.\n" + FormatSkipped(result)); + Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature)); + string text = HotReloadWorkerReasonText.Render(skipped.reason); + Assert.That(text, Does.Contain("'" + PayloadTypeMetadataName + "'"), text); + Assert.That(text, Does.Contain("'" + RegistryTypeMetadataName + "'"), text); + Assert.That(text, Does.Contain("Pass 'Assets/Tests/Editor/HotReload/" + RegistryFileName + "'"), text); + Assert.That(text, Does.Contain("uloop compile"), text); + } + + /// + /// What: passing the file that declares the compiled API as well lets the added method bind + /// against the payload this run declares, so the method is applied, which is the recovery + /// the skipped row recommends. + /// + [Test] + public async Task Run_HostWithThePayloadAndTheRegistryFiles_AddsTheMethod() + { + TransformWorkerClientResult result = await RunAsync( + new[] { HostFileName, PayloadFileName, RegistryFileName }, + new[] + { + WithMethod(ReadOnDisk(HostFileName), WireMethod), + ReadOnDisk(PayloadFileName), + ReadOnDisk(RegistryFileName) + }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + AssertNoSkippedMethodNamed(result, "Wire"); + TransformWorkerEntryDto entry = FindEntry(result, "Wire"); + Assert.That(entry, Is.Not.Null, "Missing entry for Wire.\n" + FormatSkipped(result)); + Assert.That(entry.patchKind, Is.EqualTo(HotReloadConstants.PatchKindAddedMethod)); + } + + /// + /// What: when the compiled API that still takes the compiled payload is a nested type, the + /// skipped row names it in its metadata form and still finds the file declaring it, because + /// the name the worker sends and the name the compiled assembly is read by must agree. + /// + [Test] + public async Task Run_HostWithThePayloadFile_NamesTheFileDeclaringANestedCompiledSignature() + { + TransformWorkerClientResult result = await RunAsync( + new[] { HostFileName, PayloadFileName }, + new[] { WithMethod(ReadOnDisk(HostFileName), WireThroughNestedRegistryMethod), ReadOnDisk(PayloadFileName) }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + TransformWorkerSkippedDto skipped = FindSkipped(result, "Wire"); + Assert.That(skipped, Is.Not.Null, "Missing skipped row for Wire.\n" + FormatSkipped(result)); + Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature)); + string text = HotReloadWorkerReasonText.Render(skipped.reason); + Assert.That(text, Does.Contain("'" + NestedRegistryTypeMetadataName + "'"), text); + Assert.That(text, Does.Contain("Pass 'Assets/Tests/Editor/HotReload/" + NestedRegistryFileName + "'"), text); + } + + /// + /// What: when the compiled type is the receiver of a compiled extension method, which the + /// reduced call does not list among its parameters, the skipped row still names the file + /// declaring the extension, so a split through the receiver is not missed. + /// + [Test] + public async Task Run_HostWithThePayloadFile_NamesTheFileDeclaringACompiledExtensionOnThePayload() + { + TransformWorkerClientResult result = await RunAsync( + new[] { HostFileName, PayloadFileName }, + new[] { WithMethod(ReadOnDisk(HostFileName), CallExtensionMethod), ReadOnDisk(PayloadFileName) }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + TransformWorkerSkippedDto skipped = FindSkipped(result, "Twice"); + Assert.That(skipped, Is.Not.Null, "Missing skipped row for Twice.\n" + FormatSkipped(result)); + string text = HotReloadWorkerReasonText.Render(skipped.reason); + Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature), text); + Assert.That(text, Does.Contain("Pass 'Assets/Tests/Editor/HotReload/" + ExtensionsFileName + "'"), text); + } + + /// + /// What: a compiled indexer that returns the compiled payload is found as the signature + /// holding the split, so the skipped row names the registry and its file. + /// + [Test] + public async Task Run_HostWithThePayloadFile_NamesTheFileDeclaringACompiledIndexer() + { + TransformWorkerClientResult result = await RunAsync( + new[] { HostFileName, PayloadFileName }, + new[] { WithMethod(ReadOnDisk(HostFileName), PeekThroughIndexerMethod), ReadOnDisk(PayloadFileName) }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + TransformWorkerSkippedDto skipped = FindSkipped(result, "Peek"); + Assert.That(skipped, Is.Not.Null, "Missing skipped row for Peek.\n" + FormatSkipped(result)); + string text = HotReloadWorkerReasonText.Render(skipped.reason); + Assert.That(skipped.reason.code, Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature), text); + Assert.That(text, Does.Contain("'" + RegistryTypeMetadataName + "'"), text); + Assert.That(text, Does.Contain("Pass 'Assets/Tests/Editor/HotReload/" + RegistryFileName + "'"), text); + } + + /// + /// What: a body that fails only on a typo keeps the plain unbound reason even though it also + /// calls a compiled API naming the payload, because that call binds and passing its file + /// would not fix the typo. + /// + [Test] + public async Task Run_HostWithThePayloadFile_KeepsThePlainReasonWhenOnlyATypoFailsToBind() + { + TransformWorkerClientResult result = await RunAsync( + new[] { HostFileName, PayloadFileName }, + new[] { WithMethod(ReadOnDisk(HostFileName), WireWithNullAndTypoMethod), ReadOnDisk(PayloadFileName) }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + TransformWorkerSkippedDto skipped = FindSkipped(result, "Wire"); + Assert.That(skipped, Is.Not.Null, "Missing skipped row for Wire.\n" + FormatSkipped(result)); + Assert.That( + skipped.reason.code, + Is.EqualTo(HotReloadWorkerReasonCode.AddedMethodBodyUnbound), + HotReloadWorkerReasonText.Render(skipped.reason)); } private static string WithMethod(string hostSource, string method) diff --git a/Assets/Tests/Editor/HotReload/TransformWorkerClientTests.cs b/Assets/Tests/Editor/HotReload/TransformWorkerClientTests.cs index d7e9e12a26..3f5deb80ec 100644 --- a/Assets/Tests/Editor/HotReload/TransformWorkerClientTests.cs +++ b/Assets/Tests/Editor/HotReload/TransformWorkerClientTests.cs @@ -3565,7 +3565,9 @@ private static TransformWorkerInputDto CreatePreparationValidationInputWithRetai private static TransformWorkerOutputInterpreter CreateOutputInterpreter() { - return new TransformWorkerOutputInterpreter(new TransformWorkerOutputValidator()); + return new TransformWorkerOutputInterpreter( + new TransformWorkerOutputValidator(), + new TransformWorkerCompiledTypeFileCompleter()); } private static string CreateMatchingPreparationOutputJson(string assemblyName, string assemblyMvid) diff --git a/Assets/Tests/Editor/HotReload/TransformWorkerCompiledTypeFileCompleterTests.cs b/Assets/Tests/Editor/HotReload/TransformWorkerCompiledTypeFileCompleterTests.cs new file mode 100644 index 0000000000..2804786067 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/TransformWorkerCompiledTypeFileCompleterTests.cs @@ -0,0 +1,117 @@ +using System.IO; + +using NUnit.Framework; + +using UnityEngine; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// EditMode coverage for completing the reasons that name compiled types with the files + /// declaring them, read from the target assembly's debug data. + /// + public class TransformWorkerCompiledTypeFileCompleterTests + { + private const string TestAssemblyName = "UnityCLILoop.Tests.Editor.HotReload"; + // Any other assembly of this project: its PDB carries a different build id. + private const string OtherAssemblyName = "Assembly-CSharp"; + private const string RegistryTypeMetadataName = + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadBindingSplitRegistry"; + private const string RegistryProjectRelativePath = + "Assets/Tests/Editor/HotReload/HotReloadBindingSplitRegistry.cs"; + + /// + /// What: a split reason carried as the detail of another reason, as when a member reads an + /// added property whose own body split, is completed with the declaring file too, so the + /// composed row renders instead of throwing on a missing value. + /// + [Test] + public void Complete_SplitReasonAsDetail_CompletesTheDetailWithTheDeclaringFile() + { + TransformWorkerReasonDto split = SplitReason(); + TransformWorkerOutputDto output = new TransformWorkerOutputDto + { + skipped = new[] + { + new TransformWorkerSkippedDto + { + method = "Example.Reader.Read()", + reason = new TransformWorkerReasonDto + { + code = HotReloadWorkerReasonCode.AddedPropertyUnavailableAddedProperty, + detail = split + } + } + } + }; + + new TransformWorkerCompiledTypeFileCompleter().Complete( + new TransformWorkerInputDto { targetTypesAssemblyPath = TargetAssemblyPath() }, + output); + + string text = HotReloadWorkerReasonText.Render(output.skipped[0].reason); + Assert.That(text, Does.Contain("Pass '" + RegistryProjectRelativePath + "'"), text); + } + + /// + /// What: when the PDB beside the target assembly belongs to another build, the reason names + /// the type alone instead of failing the reload, because the file only sharpens the hint. + /// + [Test] + public void Complete_PdbOfAnotherBuild_NamesTheTypeInsteadOfThrowing() + { + string directory = Path.Combine(Path.GetTempPath(), "CompletedTypeFileMismatch_" + System.Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(directory); + try + { + string dllPath = Path.Combine(directory, TestAssemblyName + ".dll"); + File.Copy(TargetAssemblyPath(), dllPath); + File.Copy(OtherAssemblyPdbPath(), Path.ChangeExtension(dllPath, ".pdb")); + TransformWorkerReasonDto split = SplitReason(); + TransformWorkerOutputDto output = new TransformWorkerOutputDto + { + skipped = new[] { new TransformWorkerSkippedDto { method = "Example.Host.Wire()", reason = split } } + }; + + new TransformWorkerCompiledTypeFileCompleter().Complete( + new TransformWorkerInputDto { targetTypesAssemblyPath = dllPath }, + output); + + string text = HotReloadWorkerReasonText.Render(split); + Assert.That(text, Does.Contain("Pass the file that declares '" + RegistryTypeMetadataName + "'"), text); + } + finally + { + Directory.Delete(directory, true); + } + } + + private static TransformWorkerReasonDto SplitReason() + { + return new TransformWorkerReasonDto + { + code = HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature, + args = new[] { "CS1503", "'Example.Payload'", "'" + RegistryTypeMetadataName + "'" }, + typeMetadataNames = new[] { RegistryTypeMetadataName } + }; + } + + private static string OtherAssemblyPdbPath() + { + string path = Path.GetFullPath( + Path.Combine(Application.dataPath, "..", "Library", "ScriptAssemblies", OtherAssemblyName + ".pdb")); + Assert.That(File.Exists(path), Is.True, "Other assembly pdb missing: " + path); + return path; + } + + private static string TargetAssemblyPath() + { + string path = Path.GetFullPath( + Path.Combine(Application.dataPath, "..", "Library", "ScriptAssemblies", TestAssemblyName + ".dll")); + Assert.That(File.Exists(path), Is.True, "Test assembly dll missing: " + path); + return path; + } + } +} diff --git a/Assets/Tests/Editor/HotReload/TransformWorkerCompiledTypeFileCompleterTests.cs.meta b/Assets/Tests/Editor/HotReload/TransformWorkerCompiledTypeFileCompleterTests.cs.meta new file mode 100644 index 0000000000..ee67294aee --- /dev/null +++ b/Assets/Tests/Editor/HotReload/TransformWorkerCompiledTypeFileCompleterTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: a3494848068294e9fa7d915391593ae5 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs index bbd948dee7..a63739fc87 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs @@ -31,6 +31,7 @@ internal enum HotReloadWorkerReasonCode AddedMethodInterfaceMember, AddedMethodInaccessibleAccessNoRewrite, AddedMethodBodyUnbound, + AddedMethodBodyBindsCompiledSignature, AddedFieldStructHost, AddedFieldInitializerNotLiteralOrExternalStatic, AddedFieldFieldTypeNotExternallyVisible, diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs index 95852d060a..830b4010b8 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs @@ -62,6 +62,14 @@ private static void AddAddedMemberTemplates( "The added member's body could not be fully bound in the hot-reload compilation ({0}); " + "hot reload cannot verify a member it cannot bind, so it is skipped. " + CompileCallToAction, 1)); + templates.Add( + HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature, + Plain( + "The added member's body could not be fully bound in the hot-reload compilation ({0}): " + + "this reload declares {1} from source, while the compiled signatures of {2} still name " + + "the compiled {1}, so it is skipped. Pass {3} to this reload as well so both bind to " + + "the same type. Otherwise run 'uloop compile'.", + 4)); templates.Add( HotReloadWorkerReasonCode.AddedFieldStructHost, diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerClient.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerClient.cs index 66cf8ed498..193e4018f1 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerClient.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerClient.cs @@ -154,7 +154,9 @@ private async Task RunOneShotAsync( private static TransformWorkerOutputInterpreter CreateOutputInterpreter() { - return new TransformWorkerOutputInterpreter(new TransformWorkerOutputValidator()); + return new TransformWorkerOutputInterpreter( + new TransformWorkerOutputValidator(), + new TransformWorkerCompiledTypeFileCompleter()); } } diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerCompiledTypeFileCompleter.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerCompiledTypeFileCompleter.cs new file mode 100644 index 0000000000..dce8c720ba --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerCompiledTypeFileCompleter.cs @@ -0,0 +1,159 @@ +using System; +using System.Collections.Generic; +using System.IO; + +using Mono.Cecil; +using Mono.Cecil.Cil; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Completes the skipped reasons that name compiled types with the files declaring them, read + /// from the debug data of the compiled assembly the run targets. + /// + // Why on the Editor side: the worker only binds the compiled assembly as metadata and never + // reads its PDB, while the reader needs a file it can pass to the next reload. + internal sealed class TransformWorkerCompiledTypeFileCompleter + { + internal void Complete(TransformWorkerInputDto input, TransformWorkerOutputDto output) + { + Dictionary> filesByType = null; + foreach (TransformWorkerSkippedDto skipped in output.skipped) + { + // Why the whole detail chain: a member whose own body split is recorded as + // unavailable, and a member reading it is skipped with that reason as its detail. + for (TransformWorkerReasonDto reason = skipped?.reason; reason != null; reason = reason.detail) + { + if (reason.typeMetadataNames == null) + { + continue; + } + + filesByType ??= ReadDeclaringFiles(input.targetTypesAssemblyPath); + reason.args = AppendPassTarget(reason.args, reason.typeMetadataNames, filesByType); + } + } + } + + private static string[] AppendPassTarget( + string[] args, + string[] typeMetadataNames, + Dictionary> filesByType) + { + List targets = new List(); + foreach (string typeMetadataName in typeMetadataNames) + { + if (!filesByType.TryGetValue(typeMetadataName, out List files) || files.Count == 0) + { + targets.Add("the file that declares '" + typeMetadataName + "'"); + continue; + } + + foreach (string file in files) + { + string quoted = "'" + file + "'"; + if (!targets.Contains(quoted)) + { + targets.Add(quoted); + } + } + } + + List completed = new List(args ?? Array.Empty()); + completed.Add(string.Join(" and ", targets)); + return completed.ToArray(); + } + + // Keyed by the metadata name Cecil reports, which is the form the worker sends, so a nested + // type needs no conversion here. A type is missing when the assembly or its PDB is. + private static Dictionary> ReadDeclaringFiles(string dllPath) + { + Dictionary> filesByType = new Dictionary>(StringComparer.Ordinal); + string pdbPath = string.IsNullOrEmpty(dllPath) ? null : Path.ChangeExtension(dllPath, ".pdb"); + if (pdbPath == null || !File.Exists(dllPath) || !File.Exists(pdbPath)) + { + return filesByType; + } + + // Why not let a read failure escape: the files only sharpen a hint, and a row naming the + // types alone still tells the reader what to pass, so the reload must not fail on it. + try + { + ReadDocumentsInto(dllPath, pdbPath, filesByType); + } + // InvalidOperationException covers Cecil's SymbolsNotMatchingException, thrown when the + // PDB beside the assembly belongs to another build of it. + catch (Exception exception) when (exception is IOException + || exception is BadImageFormatException + || exception is InvalidOperationException) + { + filesByType.Clear(); + } + + return filesByType; + } + + private static void ReadDocumentsInto(string dllPath, string pdbPath, Dictionary> filesByType) + { + using FileStream dllStream = File.Open(dllPath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); + using FileStream pdbStream = File.Open(pdbPath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); + ReaderParameters readerParameters = new ReaderParameters + { + InMemory = true, + ReadSymbols = true, + SymbolReaderProvider = new PortablePdbReaderProvider(), + SymbolStream = pdbStream + }; + using AssemblyDefinition assembly = AssemblyDefinition.ReadAssembly(dllStream, readerParameters); + foreach (TypeDefinition type in assembly.MainModule.GetTypes()) + { + filesByType[type.FullName] = CollectDocumentPaths(type); + } + } + + private static List CollectDocumentPaths(TypeDefinition type) + { + List paths = new List(); + foreach (MethodDefinition method in type.Methods) + { + if (!method.HasBody || method.DebugInformation == null || !method.DebugInformation.HasSequencePoints) + { + continue; + } + + foreach (SequencePoint sequencePoint in method.DebugInformation.SequencePoints) + { + if (sequencePoint.IsHidden || sequencePoint.Document == null) + { + continue; + } + + string path = ToProjectRelativePath(sequencePoint.Document.Url); + if (!paths.Contains(path)) + { + paths.Add(path); + } + } + } + + return paths; + } + + // Why the current directory: the Editor runs with the project root as its working + // directory, and a rooted document path outside it is still worth showing as it is. + private static string ToProjectRelativePath(string documentUrl) + { + string path = HotReloadSourcePathNormalizer.ToForwardSlashes(documentUrl); + if (!Path.IsPathRooted(path)) + { + return path; + } + + string root = HotReloadSourcePathNormalizer.ToForwardSlashes(Directory.GetCurrentDirectory()).TrimEnd('/') + "/"; + StringComparison comparison = Path.DirectorySeparatorChar == '\\' + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; + return path.StartsWith(root, comparison) ? path.Substring(root.Length) : path; + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerCompiledTypeFileCompleter.cs.meta b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerCompiledTypeFileCompleter.cs.meta new file mode 100644 index 0000000000..70a11879d1 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerCompiledTypeFileCompleter.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: a5cbba6c730b749ec9209b2fcd9a5f1b +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerDtos.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerDtos.cs index 759c3a5954..eaf3277083 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerDtos.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerDtos.cs @@ -366,6 +366,10 @@ internal sealed class TransformWorkerReasonDto // The fragment a composed reason ends with, such as the reason an accessor rewrite was // unavailable. Null when the code does not compose. public TransformWorkerReasonDto detail; + + // Metadata names of the compiled types the sentence names, for the Editor to resolve to the + // files that declare them before the sentence is worded. Null when the reason names none. + public string[] typeMetadataNames; } [Serializable] diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerOutputInterpreter.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerOutputInterpreter.cs index d9e204c8fb..b40f6f5af1 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerOutputInterpreter.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/TransformWorkerOutputInterpreter.cs @@ -13,10 +13,14 @@ namespace io.github.hatayama.UnityCliLoop.FirstPartyTools internal sealed class TransformWorkerOutputInterpreter { private readonly TransformWorkerOutputValidator _validator; + private readonly TransformWorkerCompiledTypeFileCompleter _compiledTypeFileCompleter; - internal TransformWorkerOutputInterpreter(TransformWorkerOutputValidator validator) + internal TransformWorkerOutputInterpreter( + TransformWorkerOutputValidator validator, + TransformWorkerCompiledTypeFileCompleter compiledTypeFileCompleter) { _validator = validator; + _compiledTypeFileCompleter = compiledTypeFileCompleter; } /// @@ -83,6 +87,9 @@ internal TransformWorkerClientResult InterpretOutput( return TransformWorkerClientResult.Failure(homeAssemblyError); } + // Why last: only a validated document reaches a reader, and every reason that names + // compiled types must carry its files before any caller words it. + _compiledTypeFileCompleter.Complete(input, output); return TransformWorkerClientResult.SuccessResult(output); } diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md index 7efb07f29e..aa15b91200 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md @@ -324,6 +324,7 @@ source on disk. When a run skips a method it had patched before, `Warnings` name | Private/internal access inside an async/iterator/closure body has no accessor-delegate shape | Conditional access (`?.`), `??=`, indexers, static field writes, initializer member assignments, compound writes whose receiver could be evaluated twice, assignments whose value is consumed, and calls with `ref`/`out`/`in`, named, optional, or `params` arguments (or to extension/generic/by-ref-returning methods) cannot be rewritten to accessor delegates. Neither can ref-returning properties. These limits apply to compiled members; a `ref`/`out` method added in the same reload is reached directly. A compiled private/internal static property can be read, assigned, and compound-assigned | | An async/iterator/closure body references a private/internal type | Accessor delegates rescue member access, not type references; the body still cannot JIT-compile from the shim assembly | | A declared return or parameter type cannot be resolved (a new type this reload could not introduce, a missing using, or a typo) | Skipped; a supported new type declared in an edited file of the same assembly is introduced by this reload, so check `Warnings` for the refusal reason (`introduced-types.md`); otherwise add the type or the `using`, or fix the typo, then run `uloop compile` | +| An added member's body cannot be fully bound in the hot-reload compilation | Hot reload cannot verify a member it cannot bind. A common cause: another file of the same reload, passed or pulled back in because it holds active patches, declares a compiled type from source, while a compiled API the body calls still names the compiled copy (for example, a lambda handed to a compiled `Register(Action)`). The reason then names both types and the file declaring the compiled API; pass that file to the same reload as well so both bind to the same type, or run `uloop compile` | | Edited setter, init, or indexer accessor of a *compiled* property | Accessor patching covers getters only; `uloop compile` applies these edits. Accessors of a property added in this edit are emitted instead | | Constructor (instance or static), operator, conversion operator, or explicit event accessor (add/remove) | Skipped; `uloop compile` applies these edits | | Method raises or reads a field-like event that has no reachable backing field | Custom `add`/`remove` accessors, an `abstract`/`extern`/interface event, a delegate type that is not visible outside the assembly, or an event added in this edit leave nothing for the shim's Harmony accessor to bind | diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/AddedMemberBindingGuard.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/AddedMemberBindingGuard.cs index 19f790d34f..06bf72861b 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/AddedMemberBindingGuard.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/AddedMemberBindingGuard.cs @@ -1,4 +1,6 @@ +using System.Collections.Generic; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Text; /// /// Finds the added member bodies the worker's compilation could not bind, whose accessibility @@ -30,4 +32,21 @@ internal static Diagnostic FindFirstBindingError(SemanticModel semanticModel, Sy return null; } + + /// + /// The spans of every error the compilation reports inside . + /// + internal static List FindBindingErrorSpans(SemanticModel semanticModel, SyntaxNode body) + { + List spans = new List(); + foreach (Diagnostic diagnostic in semanticModel.GetDiagnostics(body.Span)) + { + if (diagnostic.Severity == DiagnosticSeverity.Error) + { + spans.Add(diagnostic.Location.SourceSpan); + } + } + + return spans; + } } diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/CompiledSignatureSplitCollector.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/CompiledSignatureSplitCollector.cs new file mode 100644 index 0000000000..7d9aad2900 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/CompiledSignatureSplitCollector.cs @@ -0,0 +1,252 @@ +using System; +using System.Collections.Generic; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Text; + +/// +/// Finds, among the calls and accesses of one body, the compiled members whose signatures name a +/// compiled type this run also declares from source, and the compiled types declaring them. +/// +/// +/// Why: a compiled API keeps expecting the compiled copy of such a type, so a body handing it the +/// copy this run declares fails to bind. Passing the file that declares the API as well rebuilds +/// the API against the same copy, which is the recovery a skipped row can name. +/// +internal static class CompiledSignatureSplitCollector +{ + internal static CompiledSignatureSplit Collect( + SemanticModel semanticModel, + SyntaxNode body, + IReadOnlyList bindingErrorSpans) + { + IAssemblySymbol sourceAssembly = semanticModel.Compilation.Assembly; + SortedSet splitTypes = new SortedSet(StringComparer.Ordinal); + SortedSet declaringTypes = new SortedSet(StringComparer.Ordinal); + foreach (SyntaxNode node in body.DescendantNodesAndSelf()) + { + // Why only uses an error touches: a compiled API elsewhere in the body that binds is + // not what failed, and naming its file would send the reader after the wrong fix. + if (!IsMemberUse(node) || !TouchesAny(node.Span, bindingErrorSpans)) + { + continue; + } + + foreach (ISymbol member in FindUsedMembers(semanticModel, node)) + { + AddSplit(member, sourceAssembly, splitTypes, declaringTypes); + } + } + + return new CompiledSignatureSplit(new List(splitTypes), new List(declaringTypes)); + } + + private static bool IsMemberUse(SyntaxNode node) + { + return node is InvocationExpressionSyntax + || node is MemberAccessExpressionSyntax + || node is ElementAccessExpressionSyntax + || node is BaseObjectCreationExpressionSyntax; + } + + private static bool TouchesAny(TextSpan span, IReadOnlyList errorSpans) + { + foreach (TextSpan errorSpan in errorSpans) + { + if (span.IntersectsWith(errorSpan)) + { + return true; + } + } + + return false; + } + + // A use that failed overload resolution binds to no symbol, so its candidates are where the + // compiled signature is found. + private static List FindUsedMembers(SemanticModel semanticModel, SyntaxNode node) + { + SymbolInfo symbolInfo = semanticModel.GetSymbolInfo(node); + List members = new List(symbolInfo.CandidateSymbols); + if (symbolInfo.Symbol != null) + { + members.Add(symbolInfo.Symbol); + } + + if (members.Count == 0 && node is MemberAccessExpressionSyntax memberAccess) + { + members.AddRange(FindExtensionsOnCompiledCopy(semanticModel, memberAccess)); + } + + return members; + } + + // Why a lookup: a compiled extension whose receiver is the compiled copy leaves no symbol and + // no candidate on a receiver of the copy this run declares (CS1929), so the extension is only + // found by looking the name up on the compiled copy itself. + private static IEnumerable FindExtensionsOnCompiledCopy( + SemanticModel semanticModel, + MemberAccessExpressionSyntax memberAccess) + { + if (!(semanticModel.GetTypeInfo(memberAccess.Expression).Type is INamedTypeSymbol receiverType) + || !IsFromSource(receiverType, semanticModel.Compilation.Assembly)) + { + return Array.Empty(); + } + + INamedTypeSymbol compiledCopy = FindCompiledCopy(semanticModel.Compilation, receiverType); + if (compiledCopy == null) + { + return Array.Empty(); + } + + return semanticModel.LookupSymbols( + memberAccess.Name.SpanStart, + compiledCopy, + memberAccess.Name.Identifier.ValueText, + includeReducedExtensionMethods: true); + } + + private static INamedTypeSymbol FindCompiledCopy(Compilation compilation, INamedTypeSymbol sourceType) + { + string reflectionName = CecilTypeNames.ToMetadataName(sourceType.OriginalDefinition).Replace('/', '+'); + foreach (IAssemblySymbol referenced in compilation.SourceModule.ReferencedAssemblySymbols) + { + INamedTypeSymbol compiledCopy = referenced.GetTypeByMetadataName(reflectionName); + if (compiledCopy != null) + { + return compiledCopy; + } + } + + return null; + } + + private static void AddSplit( + ISymbol member, + IAssemblySymbol sourceAssembly, + SortedSet splitTypes, + SortedSet declaringTypes) + { + INamedTypeSymbol declaringType = member?.ContainingType; + if (declaringType == null || IsFromSource(declaringType, sourceAssembly)) + { + return; + } + + List signatureTypes = new List(); + CollectSignatureTypes(member, signatureTypes); + bool found = false; + foreach (INamedTypeSymbol signatureType in signatureTypes) + { + // Only a type of the declaring type's own assembly can be rebuilt by passing the + // declaring file: that file compiles into the same assembly as the copy it names. + if (!SymbolEqualityComparer.Default.Equals(signatureType.ContainingAssembly, declaringType.ContainingAssembly) + || !HasSourceCopy(signatureType, sourceAssembly)) + { + continue; + } + + splitTypes.Add(CecilTypeNames.ToMetadataName(signatureType.OriginalDefinition)); + found = true; + } + + if (found) + { + declaringTypes.Add(CecilTypeNames.ToMetadataName(declaringType.OriginalDefinition)); + } + } + + private static bool IsFromSource(INamedTypeSymbol type, IAssemblySymbol sourceAssembly) + { + return SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, sourceAssembly); + } + + // Why '+': the source assembly looks nested types up by their reflection name, while the + // name reported to the Editor keeps the metadata form Cecil reads. + private static bool HasSourceCopy(INamedTypeSymbol type, IAssemblySymbol sourceAssembly) + { + string reflectionName = CecilTypeNames.ToMetadataName(type.OriginalDefinition).Replace('/', '+'); + return sourceAssembly.GetTypeByMetadataName(reflectionName) != null; + } + + private static void CollectSignatureTypes(ISymbol member, List types) + { + switch (member) + { + case IMethodSymbol method: + AddType(method.ReturnType, types); + foreach (IParameterSymbol parameter in method.Parameters) + { + AddType(parameter.Type, types); + } + + foreach (ITypeSymbol typeArgument in method.TypeArguments) + { + AddType(typeArgument, types); + } + + // A reduced extension call drops the receiver from its parameters, and a split in + // the receiver is exactly what makes the call fail to bind. + if (method.ReducedFrom != null) + { + CollectSignatureTypes(method.ReducedFrom, types); + } + + return; + case IPropertySymbol property: + AddType(property.Type, types); + foreach (IParameterSymbol parameter in property.Parameters) + { + AddType(parameter.Type, types); + } + + return; + case IFieldSymbol field: + AddType(field.Type, types); + return; + case IEventSymbol eventSymbol: + AddType(eventSymbol.Type, types); + return; + } + } + + // Walks type arguments and array elements, so the payload of Action or Payload[] is + // found the way the lambda's target type needs it. + private static void AddType(ITypeSymbol type, List types) + { + if (type is IArrayTypeSymbol arrayType) + { + AddType(arrayType.ElementType, types); + return; + } + + if (!(type is INamedTypeSymbol namedType)) + { + return; + } + + types.Add(namedType); + foreach (ITypeSymbol typeArgument in namedType.TypeArguments) + { + AddType(typeArgument, types); + } + } +} + +/// +/// The compiled types a body's compiled signatures name while this run declares them from source, +/// and the compiled types declaring those signatures, both as sorted metadata names. +/// +internal sealed class CompiledSignatureSplit +{ + internal CompiledSignatureSplit(List splitTypeMetadataNames, List declaringTypeMetadataNames) + { + SplitTypeMetadataNames = splitTypeMetadataNames; + DeclaringTypeMetadataNames = declaringTypeMetadataNames; + } + + internal List SplitTypeMetadataNames { get; } + + internal List DeclaringTypeMetadataNames { get; } +} diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/MethodTransformDecider.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/MethodTransformDecider.cs index 07646ddfac..a5602f2262 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/MethodTransformDecider.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/MethodTransformDecider.cs @@ -232,6 +232,37 @@ internal static List FindClosureBodies(SyntaxNode bodyNode) return bodies; } + // Why name the compiled types: when the body fails because a compiled API still takes the + // compiled copy of a type this run declares, passing the API's file as well is a recovery + // short of a compile, and only the declaring types tell the reader which file that is. + private static WorkerReason DescribeUnboundBody( + SemanticModel semanticModel, + SyntaxNode methodBodyNode, + Diagnostic bindingError) + { + string diagnosticText = bindingError.Id + ": " + bindingError.GetMessage(CultureInfo.InvariantCulture); + CompiledSignatureSplit split = CompiledSignatureSplitCollector.Collect( + semanticModel, + methodBodyNode, + AddedMemberBindingGuard.FindBindingErrorSpans(semanticModel, methodBodyNode)); + if (split.DeclaringTypeMetadataNames.Count == 0) + { + return WorkerReason.Of(HotReloadWorkerReasonCode.AddedMethodBodyUnbound, diagnosticText); + } + + return WorkerReason.NamingCompiledTypes( + HotReloadWorkerReasonCode.AddedMethodBodyBindsCompiledSignature, + split.DeclaringTypeMetadataNames.ToArray(), + diagnosticText, + QuoteNames(split.SplitTypeMetadataNames), + QuoteNames(split.DeclaringTypeMetadataNames)); + } + + private static string QuoteNames(List names) + { + return "'" + string.Join("', '", names) + "'"; + } + // Why a second plan pass: DecideMethodTransform only sets UsesDelegation for // async/iterator/closure bodies. An ordinary added method JIT-compiles in the // shim assembly, so inaccessible compiled members must take the same accessor @@ -249,10 +280,7 @@ internal static MethodTransformDecision DecideAddedMethodAccessors( Diagnostic bindingError = AddedMemberBindingGuard.FindFirstBindingError(semanticModel, methodBodyNode); if (bindingError != null) { - return MethodTransformDecision.Skip( - WorkerReason.Of( - HotReloadWorkerReasonCode.AddedMethodBodyUnbound, - bindingError.Id + ": " + bindingError.GetMessage(CultureInfo.InvariantCulture))); + return MethodTransformDecision.Skip(DescribeUnboundBody(semanticModel, methodBodyNode, bindingError)); } if (current.UsesDelegation) diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/WorkerReason.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/WorkerReason.cs index bb2a099fa5..8eb1073bd7 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/WorkerReason.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/WorkerReason.cs @@ -12,6 +12,10 @@ internal sealed class WorkerReason // The fragment a composed reason ends with. Null when the code does not compose. public WorkerReason Detail { get; set; } + // Metadata names of the compiled types the sentence names, which only the Editor can resolve + // to their files. Null when the reason names none. + public string[] TypeMetadataNames { get; set; } + internal static WorkerReason Of(HotReloadWorkerReasonCode code, params string[] args) { return new WorkerReason @@ -26,6 +30,21 @@ internal static WorkerReason Composite(HotReloadWorkerReasonCode code, WorkerRea return new WorkerReason { Code = code, Detail = detail }; } + // A reason whose sentence names compiled types the Editor still has to resolve to files. The + // Editor appends the sentence's last value from those files, so args leave it out. + internal static WorkerReason NamingCompiledTypes( + HotReloadWorkerReasonCode code, + string[] typeMetadataNames, + params string[] args) + { + return new WorkerReason + { + Code = code, + Args = args, + TypeMetadataNames = typeMetadataNames + }; + } + // The same as Composite for a sentence that also substitutes values of its own. It is a // separate name because an overload would let a caller drop the args by accident. internal static WorkerReason CompositeWithArgs(