From c9606280347c0217e812449243f2cd71c7e82a36 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Fri, 18 Sep 2026 17:49:53 +0900 Subject: [PATCH 1/3] fix: Hot reload refuses an edit that would split an introduced type instead of silently dropping earlier added methods (#2831) --- .../HotReloadRetainedArtifactFixture.cs | 141 +++++++++- .../Editor/HotReload/HotReloadToolTests.cs | 29 +- .../HotReloadWorkerReasonTextTests.cs | 5 +- ...rmWorkerRetainedSignatureReferenceTests.cs | 261 ++++++++++++++++++ ...kerRetainedSignatureReferenceTests.cs.meta | 11 + .../HotReloadApplyResponseBuilder.cs | 20 +- ...adWorkerReasonText.AddedMemberTemplates.cs | 6 +- .../RetainedDeclarationStage.cs | 7 + .../RetainedTypeSignatureReferenceFinder.cs | 166 +++++++++++ 9 files changed, 620 insertions(+), 26 deletions(-) create mode 100644 Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs create mode 100644 Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedTypeSignatureReferenceFinder.cs diff --git a/Assets/Tests/Editor/HotReload/HotReloadRetainedArtifactFixture.cs b/Assets/Tests/Editor/HotReload/HotReloadRetainedArtifactFixture.cs index 13c30fc686..b9f1726712 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadRetainedArtifactFixture.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadRetainedArtifactFixture.cs @@ -44,6 +44,12 @@ internal sealed class HotReloadRetainedArtifactFixture private const string SiblingSource = "namespace Example { public class Sibling { public int Get() { return 3; } } }"; + // The retained type whose signatures name the default retained type, recorded next to it + // when a test asks for a referrer. + private const string RetainedMetadataName = "Example.Retained"; + + public const string ReferrerMetadataName = "Example.RetainedFactory"; + private HotReloadRetainedArtifactFixture( string sourcePath, string projectRelativePath, @@ -97,7 +103,27 @@ public static Task CreateAsync( editedSource, SiblingSource, DefaultArtifactMethodNames, - NoArtifactMethodNames); + NoArtifactMethodNames, + RetainedArtifactReferrerShape.None); + } + + /// + /// Builds the same world with a second type in the artifact assembly whose signature names + /// the default retained type, so a test can edit that type while a type the artifact + /// still serves unchanged hands it out. + /// + public static Task CreateWithReferrerAsync( + string name, + string editedSource, + RetainedArtifactReferrerShape referrerShape) + { + return BuildAsync( + name, + editedSource, + SiblingSource, + DefaultArtifactMethodNames, + NoArtifactMethodNames, + referrerShape); } /// @@ -114,7 +140,8 @@ public static Task CreateWithSiblingSourceAsyn editedSource, siblingSource, DefaultArtifactMethodNames, - NoArtifactMethodNames); + NoArtifactMethodNames, + RetainedArtifactReferrerShape.None); } /// @@ -129,7 +156,13 @@ public static Task CreateWithArtifactMethodsAs string[] publicMethodNames, string[] privateMethodNames) { - return BuildAsync(name, editedSource, SiblingSource, publicMethodNames, privateMethodNames); + return BuildAsync( + name, + editedSource, + SiblingSource, + publicMethodNames, + privateMethodNames, + RetainedArtifactReferrerShape.None); } private static async Task BuildAsync( @@ -137,7 +170,8 @@ private static async Task BuildAsync( string editedSource, string siblingSource, string[] publicMethodNames, - string[] privateMethodNames) + string[] privateMethodNames, + RetainedArtifactReferrerShape referrerShape) { string projectRoot = Path.GetFullPath(Path.Combine(Application.dataPath, "..")); string directory = Path.Combine( @@ -155,7 +189,7 @@ private static async Task BuildAsync( string targetAssemblyPath = Path.Combine(directory, "RetainedTarget.dll"); string targetAssemblyMvid = CreateTargetAssembly(targetAssemblyPath); string artifactPath = Path.Combine(directory, "RetainedArtifact.dll"); - CreateArtifactAssembly(artifactPath, publicMethodNames, privateMethodNames); + CreateArtifactAssembly(artifactPath, publicMethodNames, privateMethodNames, referrerShape); HotReloadRetainedArtifactFixture fixture = new HotReloadRetainedArtifactFixture( sourcePath, "Assets/RetainedDeclaration/" + name + "/Edited.cs", @@ -167,7 +201,7 @@ private static async Task BuildAsync( artifactPath); // The recorded fingerprint has to be the value planning really produced for this // source, so it is read back from a prepare run rather than restated by the test. - fixture.RetainedFingerprint = await fixture.ReadPlannedFingerprintAsync(); + fixture.RetainedFingerprint = await fixture.ReadPlannedFingerprintAsync(RetainedMetadataName); return fixture; } @@ -239,7 +273,7 @@ public TransformWorkerIntroducedTypeArtifactDto CreateRecordedArtifact( { new TransformWorkerIntroducedTypeArtifactTypeDto { - metadataName = "Example.Retained", + metadataName = RetainedMetadataName, originalAssemblyName = TargetAssemblyName, originalAssemblyMvid = TargetAssemblyMvid, ownerProjectRelativePath = ProjectRelativePath, @@ -259,6 +293,30 @@ public TransformWorkerIntroducedTypeArtifactDto CreateRecordedArtifact( }; } + /// + /// The recorded artifact with the referrer type listed too, under the fingerprint a test + /// planned for it, so the run knows the artifact serves both types. + /// + public TransformWorkerIntroducedTypeArtifactDto CreateRecordedArtifactWithReferrer( + string declarationFingerprint, + string referrerFingerprint) + { + TransformWorkerIntroducedTypeArtifactDto artifact = CreateRecordedArtifact(declarationFingerprint); + List types = + new List(artifact.types); + types.Add( + new TransformWorkerIntroducedTypeArtifactTypeDto + { + metadataName = ReferrerMetadataName, + originalAssemblyName = TargetAssemblyName, + originalAssemblyMvid = TargetAssemblyMvid, + ownerProjectRelativePath = ProjectRelativePath, + declarationFingerprint = referrerFingerprint + }); + artifact.types = types.ToArray(); + return artifact; + } + private static string ReadAssemblySimpleName(string path) { using (AssemblyDefinition assembly = AssemblyDefinition.ReadAssembly(path)) @@ -324,20 +382,24 @@ private TransformWorkerInputDto BuildInput( }; } - private async Task ReadPlannedFingerprintAsync() + /// + /// The fingerprint planning produces for the named type of the edited source as it is on + /// disk now, so a test can record a second type the way the first one is recorded. + /// + public async Task ReadPlannedFingerprintAsync(string metadataName) { TransformWorkerClientResult prepared = await HotReloadCompositionRoot.Services.TransformWorkerClient.RunAsync(BuildPrepareInput(), CancellationToken.None); Assert.That(prepared.Success, Is.True, prepared.ErrorMessage); foreach (TransformWorkerIntroducedTypeDto introducedType in prepared.Output.files[0].introducedTypes) { - if (introducedType.metadataName == "Example.Retained") + if (introducedType.metadataName == metadataName) { return introducedType.declarationFingerprint; } } - Assert.Fail("Planning did not report the retained type."); + Assert.Fail("Planning did not report " + metadataName + "."); return null; } @@ -372,7 +434,8 @@ private static string CreateTargetAssembly(string path) private static void CreateArtifactAssembly( string path, string[] publicMethodNames, - string[] privateMethodNames) + string[] privateMethodNames, + RetainedArtifactReferrerShape referrerShape) { AssemblyNameDefinition assemblyName = new AssemblyNameDefinition("RetainedArtifact", new Version(1, 0, 0, 0)); using (AssemblyDefinition assembly = AssemblyDefinition.CreateAssembly(assemblyName, "RetainedArtifact", ModuleKind.Dll)) @@ -410,10 +473,55 @@ private static void CreateArtifactAssembly( assembly.MainModule.TypeSystem.Object); AddConstructor(assembly, restricted, CecilMethodAttributes.Private); assembly.MainModule.Types.Add(restricted); + AddReferrer(assembly, retained, referrerShape); assembly.Write(path); } } + // The referrer lives in the same assembly as the type it names, because a type served + // from metadata hands out the artifact's definition whichever retained assembly it is in. + private static void AddReferrer( + AssemblyDefinition assembly, + TypeDefinition retained, + RetainedArtifactReferrerShape referrerShape) + { + if (referrerShape == RetainedArtifactReferrerShape.None) + { + return; + } + + TypeDefinition referrer = new TypeDefinition( + "Example", + "RetainedFactory", + CecilTypeAttributes.Public | CecilTypeAttributes.Class, + assembly.MainModule.TypeSystem.Object); + AddConstructor(assembly, referrer, CecilMethodAttributes.Public); + if (referrerShape == RetainedArtifactReferrerShape.PublicReturnType) + { + MethodDefinition make = new MethodDefinition( + "Make", + CecilMethodAttributes.Public | CecilMethodAttributes.HideBySig, + retained); + ILProcessor makeProcessor = make.Body.GetILProcessor(); + makeProcessor.Append(makeProcessor.Create(OpCodes.Ldnull)); + makeProcessor.Append(makeProcessor.Create(OpCodes.Ret)); + referrer.Methods.Add(make); + } + else + { + MethodDefinition accept = new MethodDefinition( + "Accept", + CecilMethodAttributes.Private | CecilMethodAttributes.HideBySig, + assembly.MainModule.TypeSystem.Void); + accept.Parameters.Add(new ParameterDefinition("value", Mono.Cecil.ParameterAttributes.None, retained)); + ILProcessor acceptProcessor = accept.Body.GetILProcessor(); + acceptProcessor.Append(acceptProcessor.Create(OpCodes.Ret)); + referrer.Methods.Add(accept); + } + + assembly.MainModule.Types.Add(referrer); + } + // The bodies are never executed: the artifact is only ever read as metadata, so a bare // return is enough to give a member the signature the compiler binds against. private static void AddConstructor( @@ -488,4 +596,15 @@ public static UnityEditor.Compilation.Assembly FindCompilationAssembly() return null; } } + + /// + /// Whether the fixture's artifact assembly also holds a type whose signature names the + /// default retained type, and through which member kind. + /// + internal enum RetainedArtifactReferrerShape + { + None, + PublicReturnType, + PrivateParameterType + } } diff --git a/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs b/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs index 23b09e9007..e1905c3880 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs @@ -1523,7 +1523,7 @@ public void BuildApplyResponse_WithWarnings_AppendsWarningCountOnEveryBranch() Assert.That( appliedWithSkipped.Message, Is.EqualTo( - "Hot reload applied. PatchedTotal=1, ActivePatchTotal=1. " + "Hot reload applied. PatchedTotal=1, ActivePatchTotal=1. Skipped: 1. " + "3 warning(s). See Warnings. " + HotReloadConstants.MultiWarningSingleCompileResolutionMessage)); } @@ -1729,10 +1729,35 @@ public void BuildApplyResponse_AppliedWithSkipped_ListsSkippedInWarnings() Assert.That( response.Message, Is.EqualTo( - "Hot reload applied. PatchedTotal=1, ActivePatchTotal=1. " + "Hot reload applied. PatchedTotal=1, ActivePatchTotal=1. Skipped: 1. " + "1 warning(s). See Warnings.")); } + /// + /// What: an applied run that also skipped methods counts them right after Added, so the + /// summary line does not read as if every edit was applied. + /// + [Test] + public void BuildApplyResponse_AddedAndSkippedOutcomes_CountsSkippedAfterAdded() + { + HotReloadOrchestratorResult result = new HotReloadOrchestratorResult( + new List + { + HotReloadMethodOutcome.Added("Type.AddedPing", "Assets/A.cs"), + HotReloadMethodOutcome.Skipped("Type.First", "reason", "Assets/A.cs"), + HotReloadMethodOutcome.Skipped("Type.Second", "reason", "Assets/A.cs") + }, + new List(), + patchedTotal: 0, + activePatchTotal: 1); + + HotReloadResponse response = HotReloadTool.BuildApplyResponse(result); + + Assert.That( + response.Message, + Does.StartWith("Hot reload applied. PatchedTotal=0, ActivePatchTotal=1. Added: 1. Skipped: 2.")); + } + /// /// What: an applied run without any Skipped outcome adds no Skipped-derived warning. /// diff --git a/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs b/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs index 2741ca8935..37748d53e1 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs @@ -272,9 +272,8 @@ private static IEnumerable RenderCases() HotReloadWorkerReasonCode.AddedMethodBodyUnbound, new[] { "CS1503: Argument 1: cannot convert" }, "The added member's body could not be fully bound in the hot-reload compilation " - + "(CS1503: Argument 1: cannot convert), typically because another file in this reload " - + "declares a type that the compiled assembly also contains. Private-member access cannot " - + "be verified, so the member is skipped. Run 'uloop compile'."); + + "(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.AddedFieldStructHost, NoArgs, diff --git a/Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs b/Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs new file mode 100644 index 0000000000..80f28b9244 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs @@ -0,0 +1,261 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// Verifies that a run refuses to change a retained type another retained type names in its + /// signatures while that other type stays bound from the retained assembly, because the edit + /// would leave two definitions of the changed type in one compilation. + /// + public sealed class TransformWorkerRetainedSignatureReferenceTests + { + private const string RetainedTypeMetadataName = "Example.Retained"; + + // The referrer is not part of the edited source in most tests, so its record only has to + // carry a fingerprint for the request to be well formed. + private const string UnplannedReferrerFingerprint = + "0000000000000000000000000000000000000000000000000000000000000000"; + + private const string RetainedSource = + "namespace Example\n" + + "{\n" + + " public class Retained\n" + + " {\n" + + " public static int Value = 1;\n" + + "\n" + + " public int Compute()\n" + + " {\n" + + " return Value * 2;\n" + + " }\n" + + " }\n" + + "}\n"; + + private const string ComputeBody = " return Value * 2;\n"; + + private const string ComputeMember = + " public int Compute()\n" + + " {\n" + + ComputeBody + + " }\n"; + + private const string AddedMethodMember = + " public int Extra()\n" + + " {\n" + + " return Value + 5;\n" + + " }\n"; + + // The referrer as source, matching the artifact's public-return shape, so a test can put + // it into the same run as the type it names. + private const string ReferrerDeclaration = + "\n" + + " public class RetainedFactory\n" + + " {\n" + + " public Retained Make()\n" + + " {\n" + + " return new Retained();\n" + + " }\n" + + " }\n"; + + private const string ReferrerMakeBody = " return new Retained();\n"; + + /// + /// What: adding a method to a retained type that an unchanged retained type returns from a + /// public method fails the run and names both types and 'uloop compile', instead of + /// reporting the added method as unbound and dropping the members added earlier. + /// + [Test] + public async Task Transform_MethodAddedToRetainedTypeReferencedByAnUnchangedRetainedTypesSignature_FailsRunAndNamesTheReferrer() + { + HotReloadRetainedArtifactFixture fixture = await HotReloadRetainedArtifactFixture.CreateWithReferrerAsync( + "ReferrerAddedMethod", + RetainedSource, + RetainedArtifactReferrerShape.PublicReturnType); + File.WriteAllText(fixture.SourcePath, WithAddedMethod(RetainedSource)); + + TransformWorkerClientResult result = await RunAsync( + fixture, + fixture.CreateRecordedArtifactWithReferrer(fixture.RetainedFingerprint, UnplannedReferrerFingerprint)); + + AssertRefusedNamingTheReferrer(result); + } + + /// + /// What: a body-only edit of a retained type an unchanged retained type names is refused + /// the same way, because the edited declaration stays in the source and splits the type + /// exactly as an added member does. + /// + [Test] + public async Task Transform_MethodBodyEditedOnRetainedTypeReferencedByAnUnchangedRetainedTypesSignature_FailsRun() + { + HotReloadRetainedArtifactFixture fixture = await HotReloadRetainedArtifactFixture.CreateWithReferrerAsync( + "ReferrerBodyEdit", + RetainedSource, + RetainedArtifactReferrerShape.PublicReturnType); + File.WriteAllText(fixture.SourcePath, WithEditedComputeBody(RetainedSource)); + + TransformWorkerClientResult result = await RunAsync( + fixture, + fixture.CreateRecordedArtifactWithReferrer(fixture.RetainedFingerprint, UnplannedReferrerFingerprint)); + + AssertRefusedNamingTheReferrer(result); + } + + /// + /// What: a referrer that only names the type in a private parameter still refuses the + /// edit, because its bodies bind against the retained definition all the same. + /// + [Test] + public async Task Transform_MethodAddedToRetainedTypeReferencedByAPrivateParameter_FailsRun() + { + HotReloadRetainedArtifactFixture fixture = await HotReloadRetainedArtifactFixture.CreateWithReferrerAsync( + "ReferrerPrivateParameter", + RetainedSource, + RetainedArtifactReferrerShape.PrivateParameterType); + File.WriteAllText(fixture.SourcePath, WithAddedMethod(RetainedSource)); + + TransformWorkerClientResult result = await RunAsync( + fixture, + fixture.CreateRecordedArtifactWithReferrer(fixture.RetainedFingerprint, UnplannedReferrerFingerprint)); + + AssertRefusedNamingTheReferrer(result); + } + + /// + /// What: a referrer whose unchanged declaration is part of the same run still refuses the + /// edit, because an unchanged declaration leaves the source and binds from the retained + /// assembly like one that was never edited. + /// + [Test] + public async Task Transform_MethodAddedToRetainedTypeWhoseReferrerIsInTheRunUnchanged_FailsRun() + { + string source = WithReferrer(RetainedSource); + HotReloadRetainedArtifactFixture fixture = await HotReloadRetainedArtifactFixture.CreateWithReferrerAsync( + "ReferrerInRunUnchanged", + source, + RetainedArtifactReferrerShape.PublicReturnType); + string referrerFingerprint = + await fixture.ReadPlannedFingerprintAsync(HotReloadRetainedArtifactFixture.ReferrerMetadataName); + File.WriteAllText(fixture.SourcePath, WithAddedMethod(source)); + + TransformWorkerClientResult result = await RunAsync( + fixture, + fixture.CreateRecordedArtifactWithReferrer(fixture.RetainedFingerprint, referrerFingerprint)); + + AssertRefusedNamingTheReferrer(result); + } + + /// + /// What: when the referrer is edited in the same run, both declarations stay in the source + /// and bind to each other, so the added method is still emitted instead of the run being + /// refused. + /// + [Test] + public async Task Transform_MethodAddedToRetainedTypeWhoseReferrerIsEditedInTheSameRun_EmitsAnAddedMethod() + { + string source = WithReferrer(RetainedSource); + HotReloadRetainedArtifactFixture fixture = await HotReloadRetainedArtifactFixture.CreateWithReferrerAsync( + "ReferrerEditedInRun", + source, + RetainedArtifactReferrerShape.PublicReturnType); + string referrerFingerprint = + await fixture.ReadPlannedFingerprintAsync(HotReloadRetainedArtifactFixture.ReferrerMetadataName); + File.WriteAllText(fixture.SourcePath, WithEditedMakeBody(WithAddedMethod(source))); + + TransformWorkerClientResult result = await RunAsync( + fixture, + fixture.CreateRecordedArtifactWithReferrer(fixture.RetainedFingerprint, referrerFingerprint)); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + Assert.That(FindEntries(result, RetainedTypeMetadataName, "Extra").Length, Is.EqualTo(1)); + } + + /// + /// What: a referenced retained type whose declaration is unchanged is not refused, because + /// it leaves the source and binds from the retained assembly like its referrer. + /// + [Test] + public async Task Transform_RetainedTypeReferencedByAnotherRetainedTypeButUnchanged_DoesNotFailTheRun() + { + HotReloadRetainedArtifactFixture fixture = await HotReloadRetainedArtifactFixture.CreateWithReferrerAsync( + "ReferrerTargetUnchanged", + RetainedSource, + RetainedArtifactReferrerShape.PublicReturnType); + + TransformWorkerClientResult result = await RunAsync( + fixture, + fixture.CreateRecordedArtifactWithReferrer(fixture.RetainedFingerprint, UnplannedReferrerFingerprint)); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + Assert.That(FindEntries(result, RetainedTypeMetadataName, "Compute"), Is.Empty); + } + + private static void AssertRefusedNamingTheReferrer(TransformWorkerClientResult result) + { + Assert.That(result.Success, Is.False); + Assert.That(result.Output, Is.Null.Or.Property("entries").Empty); + Assert.That(result.ErrorMessage, Does.Contain("'" + RetainedTypeMetadataName + "'")); + Assert.That( + result.ErrorMessage, + Does.Contain("'" + HotReloadRetainedArtifactFixture.ReferrerMetadataName + "'")); + Assert.That(result.ErrorMessage, Does.Contain("uloop compile")); + } + + private static string WithAddedMethod(string source) + { + return source.Replace(ComputeMember, ComputeMember + "\n" + AddedMethodMember, StringComparison.Ordinal); + } + + private static string WithEditedComputeBody(string source) + { + return source.Replace(ComputeBody, " return Value * 3;\n", StringComparison.Ordinal); + } + + // The referrer is appended inside the namespace, after the retained declaration. + private static string WithReferrer(string source) + { + return source.Replace(" }\n}\n", " }\n" + ReferrerDeclaration + "}\n", StringComparison.Ordinal); + } + + private static string WithEditedMakeBody(string source) + { + return source.Replace( + ReferrerMakeBody, + " Retained made = new Retained();\n return made;\n", + StringComparison.Ordinal); + } + + private static async Task RunAsync( + HotReloadRetainedArtifactFixture fixture, + TransformWorkerIntroducedTypeArtifactDto artifact) + { + return await HotReloadCompositionRoot.Services.TransformWorkerClient.RunAsync( + fixture.BuildTransformInput(new[] { artifact }), + CancellationToken.None); + } + + private static TransformWorkerEntryDto[] FindEntries( + TransformWorkerClientResult result, + string typeMetadataName, + string methodName) + { + List found = new List(); + foreach (TransformWorkerEntryDto entry in result.Output.entries) + { + if (entry.typeMetadataName == typeMetadataName && entry.methodName == methodName) + { + found.Add(entry); + } + } + + return found.ToArray(); + } + } +} diff --git a/Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs.meta b/Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs.meta new file mode 100644 index 0000000000..65e6cf65cf --- /dev/null +++ b/Assets/Tests/Editor/HotReload/TransformWorkerRetainedSignatureReferenceTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: becf5cf7cccbb45748d450cb390903ac +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs index f90379697b..e3d3318484 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs @@ -309,6 +309,14 @@ private static string BuildApplyOutcomeMessage( message += " Added: " + addedCount + "."; } + // Why counted here: the totals only count what was applied, so a run that skipped + // some of the edits otherwise reads as if every one of them took effect. + int skippedCount = CountOutcomesOfKind(result, HotReloadMethodOutcomeKind.Skipped); + if (skippedCount > 0) + { + message += " Skipped: " + skippedCount + "."; + } + return AppendStaleSummary(message, result); } @@ -330,7 +338,7 @@ private static string BuildRequestedFilesAllSkippedMessage(int addedCount) // look inconsistent with the listed outcomes. private static string AppendStaleSummary(string message, HotReloadOrchestratorResult result) { - int staleCount = CountStaleOutcomes(result); + int staleCount = CountOutcomesOfKind(result, HotReloadMethodOutcomeKind.Stale); if (staleCount == 0) { return message; @@ -339,18 +347,18 @@ private static string AppendStaleSummary(string message, HotReloadOrchestratorRe return message + " Stale=" + staleCount + "."; } - private static int CountStaleOutcomes(HotReloadOrchestratorResult result) + private static int CountOutcomesOfKind(HotReloadOrchestratorResult result, HotReloadMethodOutcomeKind kind) { - int staleCount = 0; + int count = 0; for (int index = 0; index < result.Methods.Count; index++) { - if (result.Methods[index].Kind == HotReloadMethodOutcomeKind.Stale) + if (result.Methods[index].Kind == kind) { - staleCount++; + count++; } } - return staleCount; + return count; } private static string AppendUnchangedAndLifecycleNotes( diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs index f1817a15f2..dbe89ab8eb 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.AddedMemberTemplates.cs @@ -59,10 +59,8 @@ private static void AddAddedMemberTemplates( templates.Add( HotReloadWorkerReasonCode.AddedMethodBodyUnbound, Plain( - "The added member's body could not be fully bound in the hot-reload compilation ({0}), " - + "typically because another file in this reload declares a type that the compiled " - + "assembly also contains. Private-member access cannot be verified, so the member is " - + "skipped. " + CompileCallToAction, + "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( diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedDeclarationStage.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedDeclarationStage.cs index 22594aae49..33e5e1c046 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedDeclarationStage.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedDeclarationStage.cs @@ -76,6 +76,13 @@ internal static string PrepareBindingTrees( Dictionary> verdicts = IntroducedTypeDeclarationVerifier.FindRetainedDeclarations( loadedUnits, verificationCompilation, input, targetAssembly, artifactMap); + string splitRefusal = RetainedTypeSignatureReferenceFinder.FindRefusal( + verdicts, verificationCompilation, input, targetAssembly, artifactMap); + if (splitRefusal != null) + { + return splitRefusal; + } + List bindingParseErrors = new List(); foreach (KeyValuePair> entry in verdicts) { diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedTypeSignatureReferenceFinder.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedTypeSignatureReferenceFinder.cs new file mode 100644 index 0000000000..6acc70dd2a --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/RetainedTypeSignatureReferenceFinder.cs @@ -0,0 +1,166 @@ +using System; +using System.Collections.Generic; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; + +// Finds the retained types an edit would split. A retained declaration this run keeps in the +// source binds as a new definition, while a retained type the run leaves to its assembly still +// names the retained definition in its signatures. A value handed out through such a signature is +// then a different type from the one the edit declares, so every body mixing the two fails to +// bind and the members added to the file earlier stop being registered without any error. +internal static class RetainedTypeSignatureReferenceFinder +{ + // The reason the run is refused, or null when no kept declaration is named by a retained type + // bound from its assembly. Refused before anything is emitted, because the referrer cannot be + // rebuilt: the assembly serving it is already loaded. + internal static string FindRefusal( + Dictionary> verdicts, + CSharpCompilation compilation, + WorkerInput input, + IAssemblySymbol targetAssembly, + IntroducedTypeArtifactMap artifactMap) + { + List kept = CollectKeptVerdicts(verdicts); + if (kept.Count == 0) + { + return null; + } + + HashSet keptIdentities = new HashSet(StringComparer.Ordinal); + foreach (RetainedDeclarationVerdict verdict in kept) + { + keptIdentities.Add(BuildIdentity(verdict.Record)); + } + + IntroducedTypeDependencyWalker walker = new IntroducedTypeDependencyWalker( + compilation.Assembly, + targetAssembly, + input.TargetAssemblyName, + input.TargetAssemblyMvid, + artifactMap); + Dictionary> signatureIdentitiesByReferrer = + CollectSignatureIdentitiesOfBoundTypes(compilation, input, artifactMap, walker, keptIdentities); + foreach (RetainedDeclarationVerdict verdict in kept) + { + List referrers = FindReferrers(BuildIdentity(verdict.Record), signatureIdentitiesByReferrer); + if (referrers.Count > 0) + { + return "Introduced type '" + verdict.MetadataName + "' appears in member signatures of '" + + string.Join("', '", referrers) + + "', which an earlier reload retained and this edit leaves unchanged; changing '" + + verdict.MetadataName + + "' would split it between the retained assembly and this edit. Run 'uloop compile' to apply this edit."; + } + } + + return null; + } + + // Only a declaration the run keeps in the source is at risk: an unchanged one leaves the + // source and binds from the retained assembly like its referrers do. + private static List CollectKeptVerdicts( + Dictionary> verdicts) + { + List kept = new List(); + foreach (List unitVerdicts in verdicts.Values) + { + foreach (RetainedDeclarationVerdict verdict in unitVerdicts) + { + if (verdict.Match.Kind == IntroducedTypeFingerprintMatchKind.MethodBodiesOnly + || verdict.Match.Kind == IntroducedTypeFingerprintMatchKind.MembersAdded) + { + kept.Add(verdict); + } + } + } + + return kept; + } + + // Every retained type the run does not keep, whichever artifact holds it: two types of one + // artifact split the same way once only one of them is bound from source. Keyed by metadata + // name so the refusal can name the referrer the way the records do. + private static Dictionary> CollectSignatureIdentitiesOfBoundTypes( + CSharpCompilation compilation, + WorkerInput input, + IntroducedTypeArtifactMap artifactMap, + IntroducedTypeDependencyWalker walker, + HashSet keptIdentities) + { + Dictionary> identitiesByReferrer = + new Dictionary>(StringComparer.Ordinal); + foreach (WorkerIntroducedTypeArtifact artifact in input.IntroducedTypeArtifacts) + { + foreach (WorkerIntroducedTypeArtifactType record in artifact.Types) + { + if (keptIdentities.Contains(BuildIdentity(record))) + { + continue; + } + + INamedTypeSymbol boundType = artifactMap.FindArtifactType( + compilation, + record.OriginalAssemblyName, + record.OriginalAssemblyMvid, + record.MetadataName); + if (boundType != null) + { + identitiesByReferrer[record.MetadataName] = CollectSignatureIdentities(boundType, walker); + } + } + } + + return identitiesByReferrer; + } + + // Private members count too: a private body that takes the retained type binds against the + // retained definition exactly as a public one does. + private static HashSet CollectSignatureIdentities( + INamedTypeSymbol type, + IntroducedTypeDependencyWalker walker) + { + HashSet identities = new HashSet(StringComparer.Ordinal); + walker.AddDependencies(type.BaseType, identities); + foreach (INamedTypeSymbol implemented in type.Interfaces) + { + walker.AddDependencies(implemented, identities); + } + + foreach (ISymbol member in type.GetMembers()) + { + // A nested type is a retained type of its own and is checked under its own record. + if (member is INamedTypeSymbol) + { + continue; + } + + walker.AddDependencies(member, identities); + } + + return identities; + } + + private static List FindReferrers( + string keptIdentity, + Dictionary> signatureIdentitiesByReferrer) + { + List referrers = new List(); + foreach (KeyValuePair> entry in signatureIdentitiesByReferrer) + { + if (entry.Value.Contains(keptIdentity)) + { + referrers.Add(entry.Key); + } + } + + referrers.Sort(StringComparer.Ordinal); + return referrers; + } + + // Spelled the way IntroducedTypeArtifactMap normalizes a retained type, which is the form the + // dependency walker reports it in. + private static string BuildIdentity(WorkerIntroducedTypeArtifactType record) + { + return record.OriginalAssemblyName + "|" + record.OriginalAssemblyMvid + "|" + record.MetadataName; + } +} From 732af1034f44d7653524e252a6aeabad71ad2d31 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Fri, 18 Sep 2026 18:04:53 +0900 Subject: [PATCH 2/3] fix: Hot reload tells you to stop Play Mode when Unity would refuse the fallback compile (#2832) --- .agents/skills/uloop-hot-reload/SKILL.md | 2 +- .../uloop-hot-reload/references/output.md | 4 +- .claude/skills/uloop-hot-reload/SKILL.md | 2 +- .../uloop-hot-reload/references/output.md | 4 +- .../HotReloadCompileFallbackDeciderTests.cs | 62 +++++++++++--- .../Editor/HotReload/HotReloadToolTests.cs | 62 ++++++++++++-- .../HotReloadCompileFallbackDecider.cs | 18 ++-- .../HotReload/HotReloadTools.cs | 38 +++++---- .../HotReload/Shared/HotReloadConstants.cs | 13 +++ .../FirstPartyTools/HotReload/Skill/SKILL.md | 2 +- .../HotReload/Skill/references/output.md | 4 +- cli/common/tools/default-tools.json | 2 +- .../hot_reload_compile_fallback.go | 63 ++++++++++++-- .../hot_reload_compile_fallback_test.go | 84 ++++++++++++++++++- 14 files changed, 307 insertions(+), 53 deletions(-) diff --git a/.agents/skills/uloop-hot-reload/SKILL.md b/.agents/skills/uloop-hot-reload/SKILL.md index a041ca48ff..2f70bb44cd 100644 --- a/.agents/skills/uloop-hot-reload/SKILL.md +++ b/.agents/skills/uloop-hot-reload/SKILL.md @@ -38,7 +38,7 @@ automatically — pass it with `--files`. | `--files` | array | - | Project-relative `.cs` paths to hot-reload (method bodies, added members, and new top-level types). When omitted or empty on apply, selects compiled snapshot sources only — those whose bytes changed since the last compile snapshot, capped at 50 changed files per assembly with a warning when the cap trims the list; a file that has never been compiled is never selected and must be passed explicitly; run `uloop compile` first when no snapshot exists, or pass explicit paths when no changed source is found | | `--revert-all` | flag | - | Remove every active hot-reload patch and added member and clear the ledger; introduced types stay loaded until the next domain reload. When set, `--files` is ignored | | `--status` | flag | - | Lists the currently active changes (patched methods, added members, and introduced types) without applying or reverting anything. | -| `--compile-on-skip` | enum | `auto` | When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run `uloop compile` in the same command: `auto` only in Edit Mode (never stops a Play session), `on` always, `off` never. The response's `CompileFallback` says which; when the compile ran, `Compile` carries its result and `Success` is the compile's. | +| `--compile-on-skip` | enum | `auto` | When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run `uloop compile` in the same command: `auto` only in Edit Mode (never stops a Play session), `on` always unless Unity holds compiles until Play ends, `off` never. The response's `CompileFallback` says which; when the compile ran, `Compile` carries its result and `Success` is the compile's. | ## Status diff --git a/.agents/skills/uloop-hot-reload/references/output.md b/.agents/skills/uloop-hot-reload/references/output.md index f1445f7549..339b7ad7b5 100644 --- a/.agents/skills/uloop-hot-reload/references/output.md +++ b/.agents/skills/uloop-hot-reload/references/output.md @@ -20,6 +20,6 @@ Returns JSON with: - `ActiveIntroducedTypeTotal` (number): Introduced types this domain holds after this run or on `--status`, counted per type rather than per compiled artifact; always present and `0` when there is none. `--revert-all` cannot unload them, so its Message says how many stay loaded until the next Domain Reload, and that Auto Refresh stays held for them until `uloop compile` - `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` - `RecommendedNextAction` (string): Present when any method or introduced-type outcome is `Failed`. A partial apply (some methods patched or added, or some types left active) says to fix and rerun, run `uloop compile`, or `uloop hot-reload --revert-all`; a failure with nothing applied says to fix and rerun or compile. Omitted on success. -- `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. -- `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. +- `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), `BlockedByPlayModeSetting` (`--compile-on-skip on` during Play Mode while Unity's "Script Changes While Playing" is "Recompile After Finished Playing", which refuses the compile; `RecommendedNextAction` says to stop Play Mode first), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. +- `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. A successful compile drops `RecommendedNextAction` and ends `Message` with a sentence saying the compile succeeded; a failed one sets `RecommendedNextAction` to the compile's own `NextActions` when it reports any, and otherwise to fixing `Compile.Errors`. - `CompileFallbackNote` (string, present only with `Compile`): why the compile ran and how it ended. diff --git a/.claude/skills/uloop-hot-reload/SKILL.md b/.claude/skills/uloop-hot-reload/SKILL.md index a041ca48ff..2f70bb44cd 100644 --- a/.claude/skills/uloop-hot-reload/SKILL.md +++ b/.claude/skills/uloop-hot-reload/SKILL.md @@ -38,7 +38,7 @@ automatically — pass it with `--files`. | `--files` | array | - | Project-relative `.cs` paths to hot-reload (method bodies, added members, and new top-level types). When omitted or empty on apply, selects compiled snapshot sources only — those whose bytes changed since the last compile snapshot, capped at 50 changed files per assembly with a warning when the cap trims the list; a file that has never been compiled is never selected and must be passed explicitly; run `uloop compile` first when no snapshot exists, or pass explicit paths when no changed source is found | | `--revert-all` | flag | - | Remove every active hot-reload patch and added member and clear the ledger; introduced types stay loaded until the next domain reload. When set, `--files` is ignored | | `--status` | flag | - | Lists the currently active changes (patched methods, added members, and introduced types) without applying or reverting anything. | -| `--compile-on-skip` | enum | `auto` | When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run `uloop compile` in the same command: `auto` only in Edit Mode (never stops a Play session), `on` always, `off` never. The response's `CompileFallback` says which; when the compile ran, `Compile` carries its result and `Success` is the compile's. | +| `--compile-on-skip` | enum | `auto` | When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run `uloop compile` in the same command: `auto` only in Edit Mode (never stops a Play session), `on` always unless Unity holds compiles until Play ends, `off` never. The response's `CompileFallback` says which; when the compile ran, `Compile` carries its result and `Success` is the compile's. | ## Status diff --git a/.claude/skills/uloop-hot-reload/references/output.md b/.claude/skills/uloop-hot-reload/references/output.md index f1445f7549..339b7ad7b5 100644 --- a/.claude/skills/uloop-hot-reload/references/output.md +++ b/.claude/skills/uloop-hot-reload/references/output.md @@ -20,6 +20,6 @@ Returns JSON with: - `ActiveIntroducedTypeTotal` (number): Introduced types this domain holds after this run or on `--status`, counted per type rather than per compiled artifact; always present and `0` when there is none. `--revert-all` cannot unload them, so its Message says how many stay loaded until the next Domain Reload, and that Auto Refresh stays held for them until `uloop compile` - `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` - `RecommendedNextAction` (string): Present when any method or introduced-type outcome is `Failed`. A partial apply (some methods patched or added, or some types left active) says to fix and rerun, run `uloop compile`, or `uloop hot-reload --revert-all`; a failure with nothing applied says to fix and rerun or compile. Omitted on success. -- `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. -- `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. +- `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), `BlockedByPlayModeSetting` (`--compile-on-skip on` during Play Mode while Unity's "Script Changes While Playing" is "Recompile After Finished Playing", which refuses the compile; `RecommendedNextAction` says to stop Play Mode first), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. +- `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. A successful compile drops `RecommendedNextAction` and ends `Message` with a sentence saying the compile succeeded; a failed one sets `RecommendedNextAction` to the compile's own `NextActions` when it reports any, and otherwise to fixing `Compile.Errors`. - `CompileFallbackNote` (string, present only with `Compile`): why the compile ran and how it ended. diff --git a/Assets/Tests/Editor/HotReload/HotReloadCompileFallbackDeciderTests.cs b/Assets/Tests/Editor/HotReload/HotReloadCompileFallbackDeciderTests.cs index 881f0b4708..38bc5e1924 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadCompileFallbackDeciderTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadCompileFallbackDeciderTests.cs @@ -14,27 +14,69 @@ public class HotReloadCompileFallbackDeciderTests { /// /// What: the decision for each option, with and without an unapplied edit, in and out of - /// Play Mode. Expected values are the names the response carries, because the decision - /// enum is internal and reaches the wire through its name. + /// Play Mode, and with the Editor allowing or refusing compiles during play. Expected + /// values are the names the response carries, because the decision enum is internal and + /// reaches the wire through its name. /// - [TestCase(HotReloadCompileOnSkip.auto, true, false, "Requested")] - [TestCase(HotReloadCompileOnSkip.auto, true, true, "HeldForPlayMode")] - [TestCase(HotReloadCompileOnSkip.auto, false, false, "NotNeeded")] - [TestCase(HotReloadCompileOnSkip.auto, false, true, "NotNeeded")] - [TestCase(HotReloadCompileOnSkip.on, true, true, "Requested")] - [TestCase(HotReloadCompileOnSkip.off, true, false, "Disabled")] - [TestCase(HotReloadCompileOnSkip.off, false, true, "NotNeeded")] + [TestCase(HotReloadCompileOnSkip.auto, true, false, false, "Requested")] + [TestCase(HotReloadCompileOnSkip.auto, true, true, false, "HeldForPlayMode")] + [TestCase(HotReloadCompileOnSkip.auto, true, true, true, "HeldForPlayMode")] + [TestCase(HotReloadCompileOnSkip.auto, false, false, false, "NotNeeded")] + [TestCase(HotReloadCompileOnSkip.auto, false, true, false, "NotNeeded")] + [TestCase(HotReloadCompileOnSkip.on, true, true, false, "Requested")] + [TestCase(HotReloadCompileOnSkip.on, true, false, true, "Requested")] + [TestCase(HotReloadCompileOnSkip.on, false, true, true, "NotNeeded")] + [TestCase(HotReloadCompileOnSkip.off, true, false, false, "Disabled")] + [TestCase(HotReloadCompileOnSkip.off, true, true, true, "Disabled")] + [TestCase(HotReloadCompileOnSkip.off, false, true, false, "NotNeeded")] public void Decide_OptionAndRunState_ChoosesTheFallback( HotReloadCompileOnSkip option, bool hasUnappliedEdit, bool isPlaying, + bool compileRefusedDuringPlay, string expected) { Assert.That( - HotReloadCompileFallbackDecider.Decide(option, hasUnappliedEdit, isPlaying).ToString(), + HotReloadCompileFallbackDecider.Decide( + option, + hasUnappliedEdit, + isPlaying, + compileRefusedDuringPlay).ToString(), Is.EqualTo(expected)); } + /// + /// What: --compile-on-skip on during play does not request a compile the Editor is set to + /// refuse, so the CLI does not run one that can only fail. + /// + [Test] + public void Decide_OnDuringPlayWhenTheEditorRefusesCompiles_ReturnsBlockedByPlayModeSetting() + { + Assert.That( + HotReloadCompileFallbackDecider.Decide( + HotReloadCompileOnSkip.on, + hasUnappliedEdit: true, + isPlaying: true, + compileRefusedDuringPlay: true).ToString(), + Is.EqualTo("BlockedByPlayModeSetting")); + } + + /// + /// What: --compile-on-skip on during play still requests the compile when the Editor + /// compiles during play. + /// + [Test] + public void Decide_OnDuringPlayWhenTheEditorAllowsCompiles_ReturnsRequested() + { + Assert.That( + HotReloadCompileFallbackDecider.Decide( + HotReloadCompileOnSkip.on, + hasUnappliedEdit: true, + isPlaying: true, + compileRefusedDuringPlay: false).ToString(), + Is.EqualTo("Requested")); + } + /// /// What: one skipped method makes the run count as leaving an edit unapplied. /// diff --git a/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs b/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs index e1905c3880..75fca635c8 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs @@ -2096,7 +2096,8 @@ public void ApplyCompileFallbackDecision_SkippedInEditMode_RequestsTheCompileAnd response, CreateSkippedResult(), HotReloadCompileOnSkip.auto, - isPlaying: false); + isPlaying: false, + compileRefusedDuringPlay: false); Assert.That(response.CompileFallback, Is.EqualTo("Requested")); Assert.That(response.RecommendedNextAction, Is.EqualTo(nextActionBefore)); @@ -2118,7 +2119,8 @@ public void ApplyCompileFallbackDecision_SkippedDuringPlay_HoldsTheCompileAndSay response, CreateSkippedResult(), HotReloadCompileOnSkip.auto, - isPlaying: true); + isPlaying: true, + compileRefusedDuringPlay: false); Assert.That(response.CompileFallback, Is.EqualTo("HeldForPlayMode")); Assert.That(response.RecommendedNextAction, Does.StartWith(nextActionBefore)); @@ -2140,13 +2142,61 @@ public void ApplyCompileFallbackDecision_HeldWithNoExistingNextAction_ReportsOnl response, CreateSkippedResult(), HotReloadCompileOnSkip.auto, - isPlaying: true); + isPlaying: true, + compileRefusedDuringPlay: false); Assert.That( response.RecommendedNextAction, Is.EqualTo(HotReloadConstants.CompileFallbackHeldForPlayModeRecommendedNextAction)); } + /// + /// What: a held compile during play, with the Editor set to refuse compiles until play + /// ends, points at stopping Play Mode instead of suggesting --compile-on-skip on, which + /// would run a compile the Editor refuses. + /// + [Test] + public void ApplyCompileFallbackDecision_HeldDuringPlayWhenTheEditorRefusesCompiles_PointsAtStoppingPlayMode() + { + HotReloadResponse response = HotReloadTool.BuildApplyResponse(CreateSkippedResult()); + string nextActionBefore = response.RecommendedNextAction; + + HotReloadTool.ApplyCompileFallbackDecision( + response, + CreateSkippedResult(), + HotReloadCompileOnSkip.auto, + isPlaying: true, + compileRefusedDuringPlay: true); + + Assert.That(response.CompileFallback, Is.EqualTo("HeldForPlayMode")); + Assert.That(response.RecommendedNextAction, Does.StartWith(nextActionBefore)); + Assert.That(response.RecommendedNextAction, Does.Contain("control-play-mode --action Stop")); + Assert.That(response.RecommendedNextAction, Does.Not.Contain("--compile-on-skip on")); + } + + /// + /// What: --compile-on-skip on during play, with the Editor set to refuse compiles until + /// play ends, reports the compile as blocked and points at stopping Play Mode, so the + /// CLI does not run a compile that can only be refused. + /// + [Test] + public void ApplyCompileFallbackDecision_OnDuringPlayWhenTheEditorRefusesCompiles_ReportsBlocked() + { + HotReloadResponse response = new() { RecommendedNextAction = string.Empty }; + + HotReloadTool.ApplyCompileFallbackDecision( + response, + CreateSkippedResult(), + HotReloadCompileOnSkip.on, + isPlaying: true, + compileRefusedDuringPlay: true); + + Assert.That(response.CompileFallback, Is.EqualTo("BlockedByPlayModeSetting")); + Assert.That( + response.RecommendedNextAction, + Is.EqualTo(HotReloadConstants.CompileFallbackRefusedDuringPlayRecommendedNextAction)); + } + /// /// What: --compile-on-skip off reports that the fallback was turned off and leaves the /// run's own next action alone. @@ -2161,7 +2211,8 @@ public void ApplyCompileFallbackDecision_SkippedWithFallbackOff_ReportsDisabled( response, CreateSkippedResult(), HotReloadCompileOnSkip.off, - isPlaying: false); + isPlaying: false, + compileRefusedDuringPlay: false); Assert.That(response.CompileFallback, Is.EqualTo("Disabled")); Assert.That(response.RecommendedNextAction, Is.EqualTo(nextActionBefore)); @@ -2187,7 +2238,8 @@ public void ApplyCompileFallbackDecision_EverythingApplied_ReportsNotNeeded() response, result, HotReloadCompileOnSkip.auto, - isPlaying: true); + isPlaying: true, + compileRefusedDuringPlay: false); Assert.That(response.CompileFallback, Is.EqualTo("NotNeeded")); } diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadCompileFallbackDecider.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadCompileFallbackDecider.cs index 3b16aebcab..76bec2b85e 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadCompileFallbackDecider.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadCompileFallbackDecider.cs @@ -8,7 +8,8 @@ internal enum HotReloadCompileFallbackDecision NotNeeded = 0, Requested = 1, HeldForPlayMode = 2, - Disabled = 3 + Disabled = 3, + BlockedByPlayModeSetting = 4 } /// @@ -44,13 +45,15 @@ internal static bool HasUnappliedEdit(HotReloadOrchestratorResult result) } /// - /// Chooses the fallback for one run. isPlaying is a parameter rather than an Editor read so - /// the decision stays a pure function. + /// Chooses the fallback for one run. isPlaying and compileRefusedDuringPlay (Unity's + /// "Script Changes While Playing" set to recompile only after play ends) are parameters + /// rather than Editor reads so the decision stays a pure function. /// internal static HotReloadCompileFallbackDecision Decide( HotReloadCompileOnSkip option, bool hasUnappliedEdit, - bool isPlaying) + bool isPlaying, + bool compileRefusedDuringPlay) { // Nothing stayed unapplied, so no option asks for a compile: the run already put every // requested edit into the running domain. @@ -64,9 +67,14 @@ internal static HotReloadCompileFallbackDecision Decide( return HotReloadCompileFallbackDecision.Disabled; } + // Why on does not request a compile here: the compile tool refuses to run during play + // under this setting, so the CLI would only report a failed compile in place of the + // step that actually unblocks it. if (option == HotReloadCompileOnSkip.on) { - return HotReloadCompileFallbackDecision.Requested; + return isPlaying && compileRefusedDuringPlay + ? HotReloadCompileFallbackDecision.BlockedByPlayModeSetting + : HotReloadCompileFallbackDecision.Requested; } // Why auto holds during play: a compile reloads the domain and so ends the Play diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadTools.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadTools.cs index b409dcb15a..7acdf9fc42 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadTools.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadTools.cs @@ -32,7 +32,7 @@ public class HotReloadSchema : UnityCliLoopToolSchema public bool Status { get; set; } /// - /// When the apply run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), whether the CLI runs a compile in the same command: auto (default) does so in Edit Mode only and never stops a Play session, on always, off never. Ignored by --status and --revert-all. + /// When the apply run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), whether the CLI runs a compile in the same command: auto (default) does so in Edit Mode only and never stops a Play session, on always unless Unity holds compiles until Play ends, off never. Ignored by --status and --revert-all. /// public HotReloadCompileOnSkip CompileOnSkip { get; set; } = HotReloadCompileOnSkip.auto; } @@ -249,12 +249,15 @@ protected override async Task ExecuteAsync( services, result, selection.ScanLimitWarnings); - // isPlaying is read here because the switch above put this path on the main thread. + // isPlaying and the Play Mode compile setting are read here because the switch above + // put this path on the main thread. ApplyCompileFallbackDecision( response, result, parameters.CompileOnSkip, - EditorApplication.isPlaying); + EditorApplication.isPlaying, + EditorPrefs.GetInt(HotReloadConstants.ScriptCompilationDuringPlayEditorPrefsKey, 0) + == HotReloadConstants.ScriptCompilationDuringPlayRecompileAfterFinishedPlaying); if (!string.IsNullOrEmpty(selection.SelectionMessage)) { response.Message = selection.SelectionMessage + " " + response.Message; @@ -302,36 +305,43 @@ internal static HotReloadResponse BuildApplyResponse( additionalWarnings); } - // Records the compile-fallback decision on an apply response. isPlaying is passed in because - // the decision must not depend on Editor state when tests build responses off the main - // thread. + // Records the compile-fallback decision on an apply response. isPlaying and + // compileRefusedDuringPlay are passed in because the decision must not depend on Editor + // state when tests build responses off the main thread. internal static void ApplyCompileFallbackDecision( HotReloadResponse response, HotReloadOrchestratorResult result, HotReloadCompileOnSkip option, - bool isPlaying) + bool isPlaying, + bool compileRefusedDuringPlay) { HotReloadCompileFallbackDecision decision = HotReloadCompileFallbackDecider.Decide( option, HotReloadCompileFallbackDecider.HasUnappliedEdit(result), - isPlaying); + isPlaying, + compileRefusedDuringPlay); response.CompileFallback = decision.ToString(); - // Why only this decision touches the next action: the others either run the compile + // Why only these decisions touch the next action: the others either run the compile // (the CLI then replaces the next action itself) or leave the run's own advice correct. - if (decision != HotReloadCompileFallbackDecision.HeldForPlayMode) + if (decision != HotReloadCompileFallbackDecision.HeldForPlayMode + && decision != HotReloadCompileFallbackDecision.BlockedByPlayModeSetting) { return; } + // Why the setting picks the sentence: suggesting --compile-on-skip on when the Editor + // refuses compiles during play sends the reader into a compile that is refused. + string reason = compileRefusedDuringPlay + ? HotReloadConstants.CompileFallbackRefusedDuringPlayRecommendedNextAction + : HotReloadConstants.CompileFallbackHeldForPlayModeRecommendedNextAction; + // Why append and not replace: the run's own advice names what to do about the // partially applied or failed edits, and losing it would leave the caller with only // the reason no compile ran. string existingNextAction = response.RecommendedNextAction; response.RecommendedNextAction = string.IsNullOrEmpty(existingNextAction) - ? HotReloadConstants.CompileFallbackHeldForPlayModeRecommendedNextAction - : existingNextAction - + " " - + HotReloadConstants.CompileFallbackHeldForPlayModeRecommendedNextAction; + ? reason + : existingNextAction + " " + reason; } private static HotReloadResponse CreateValidationFailure( diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs index 694ffa953f..51549f63c8 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs @@ -467,6 +467,19 @@ public static bool IsPublicizableProjectAssemblyFileName(string fileNameWithoutE public const string CompileFallbackHeldForPlayModeRecommendedNextAction = "No compile ran because the Editor is in Play Mode and a compile would stop the Play session; rerun with --compile-on-skip on to compile anyway."; + // The same Stop step the compile tool recommends when it refuses to compile during play + // (CompileErrorNextActionsConstants.PlayModeStopNextAction); restated here because a tool + // may not reference another tool's assembly. + public const string CompileFallbackRefusedDuringPlayRecommendedNextAction = + "No compile ran because the Editor is in Play Mode and Unity's 'Script Changes While Playing' is set to 'Recompile After Finished Playing'. Run 'uloop control-play-mode --action Stop' to leave Play Mode, then rerun 'uloop compile'."; + + // Unity's EditorPrefs entry behind "Script Changes While Playing", and its value for + // "Recompile After Finished Playing", under which the compile tool refuses to run during + // play. Only that one value is named because it is the only one hot reload acts on. + public const string ScriptCompilationDuringPlayEditorPrefsKey = "ScriptCompilationDuringPlay"; + + public const int ScriptCompilationDuringPlayRecompileAfterFinishedPlaying = 1; + public const string RequestedFilesAllSkippedRecommendedNextAction = "Run 'uloop compile' to apply the Skipped edits, or change them into the shapes hot reload can patch (see Warnings)."; diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md index a041ca48ff..2f70bb44cd 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md @@ -38,7 +38,7 @@ automatically — pass it with `--files`. | `--files` | array | - | Project-relative `.cs` paths to hot-reload (method bodies, added members, and new top-level types). When omitted or empty on apply, selects compiled snapshot sources only — those whose bytes changed since the last compile snapshot, capped at 50 changed files per assembly with a warning when the cap trims the list; a file that has never been compiled is never selected and must be passed explicitly; run `uloop compile` first when no snapshot exists, or pass explicit paths when no changed source is found | | `--revert-all` | flag | - | Remove every active hot-reload patch and added member and clear the ledger; introduced types stay loaded until the next domain reload. When set, `--files` is ignored | | `--status` | flag | - | Lists the currently active changes (patched methods, added members, and introduced types) without applying or reverting anything. | -| `--compile-on-skip` | enum | `auto` | When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run `uloop compile` in the same command: `auto` only in Edit Mode (never stops a Play session), `on` always, `off` never. The response's `CompileFallback` says which; when the compile ran, `Compile` carries its result and `Success` is the compile's. | +| `--compile-on-skip` | enum | `auto` | When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run `uloop compile` in the same command: `auto` only in Edit Mode (never stops a Play session), `on` always unless Unity holds compiles until Play ends, `off` never. The response's `CompileFallback` says which; when the compile ran, `Compile` carries its result and `Success` is the compile's. | ## Status diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md index f1445f7549..339b7ad7b5 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md @@ -20,6 +20,6 @@ Returns JSON with: - `ActiveIntroducedTypeTotal` (number): Introduced types this domain holds after this run or on `--status`, counted per type rather than per compiled artifact; always present and `0` when there is none. `--revert-all` cannot unload them, so its Message says how many stay loaded until the next Domain Reload, and that Auto Refresh stays held for them until `uloop compile` - `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` - `RecommendedNextAction` (string): Present when any method or introduced-type outcome is `Failed`. A partial apply (some methods patched or added, or some types left active) says to fix and rerun, run `uloop compile`, or `uloop hot-reload --revert-all`; a failure with nothing applied says to fix and rerun or compile. Omitted on success. -- `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. -- `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. +- `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), `BlockedByPlayModeSetting` (`--compile-on-skip on` during Play Mode while Unity's "Script Changes While Playing" is "Recompile After Finished Playing", which refuses the compile; `RecommendedNextAction` says to stop Play Mode first), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. +- `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. A successful compile drops `RecommendedNextAction` and ends `Message` with a sentence saying the compile succeeded; a failed one sets `RecommendedNextAction` to the compile's own `NextActions` when it reports any, and otherwise to fixing `Compile.Errors`. - `CompileFallbackNote` (string, present only with `Compile`): why the compile ran and how it ended. diff --git a/cli/common/tools/default-tools.json b/cli/common/tools/default-tools.json index 7ac0d0d47b..0d3fe3ae41 100644 --- a/cli/common/tools/default-tools.json +++ b/cli/common/tools/default-tools.json @@ -770,7 +770,7 @@ "on", "off" ], - "description": "When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run uloop compile in the same command: auto only in Edit Mode (never stops a Play session), on always, off never. The response's CompileFallback says which; when the compile ran, Compile carries its result and Success is the compile's.", + "description": "When the run leaves edits unapplied (Skipped or Failed methods, Failed type declarations), run uloop compile in the same command: auto only in Edit Mode (never stops a Play session), on always unless Unity holds compiles until Play ends, off never. The response's CompileFallback says which; when the compile ran, Compile carries its result and Success is the compile's.", "default": "auto" } } diff --git a/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback.go b/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback.go index 8ee67a1002..befb4d6d43 100644 --- a/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback.go +++ b/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/hatayama/unity-cli-loop/common/clicore" clierrors "github.com/hatayama/unity-cli-loop/common/errors" @@ -21,6 +22,7 @@ const ( hotReloadCompileFallbackNoteField = "CompileFallbackNote" hotReloadSuccessField = "Success" hotReloadRecommendedNextActionField = "RecommendedNextAction" + hotReloadMessageField = "Message" hotReloadWarningsField = "Warnings" ) @@ -33,6 +35,9 @@ const ( // per-method rows. hotReloadUnappliedInMethodReasons = "Methods[].Reason" hotReloadCompileFallbackFailedNextAction = "Fix the errors in Compile.Errors, then rerun 'uloop compile' or 'uloop hot-reload'." + // The reload's own Message describes only the reload, so without this suffix a reader who stops at + // Message sees failed outcomes even though the compile then applied every edit. + hotReloadCompileFallbackSucceededMessageSuffix = " A compile then ran in this same command and succeeded; see CompileFallbackNote." ) // Test seam, same shape as the run-tests implicit compile. @@ -110,7 +115,8 @@ func injectHotReloadCompileFallback(raw json.RawMessage, compileRaw json.RawMess return nil, errors.New("hot-reload response must be a JSON object") } compile := struct { - Success bool `json:"Success"` + Success bool `json:"Success"` + NextActions []string `json:"NextActions"` }{} if err := json.Unmarshal(compileRaw, &compile); err != nil { return nil, err @@ -126,20 +132,47 @@ func injectHotReloadCompileFallback(raw json.RawMessage, compileRaw json.RawMess fields[hotReloadCompileResultField] = compileRaw fields[hotReloadSuccessField] = success - note, nextAction, err := hotReloadCompileFallbackAdvice(compile.Success, hotReloadUnappliedPointer(fields)) + note, nextAction, err := hotReloadCompileFallbackAdvice( + compile.Success, + hotReloadUnappliedPointer(fields), + compile.NextActions) if err != nil { return nil, err } fields[hotReloadCompileFallbackNoteField] = note - if compile.Success { - // The reload's own next action says to run 'uloop compile', which this command just did. - delete(fields, hotReloadRecommendedNextActionField) - } else { + if !compile.Success { fields[hotReloadRecommendedNextActionField] = nextAction + return json.Marshal(fields) + } + // The reload's own next action says to run 'uloop compile', which this command just did. + delete(fields, hotReloadRecommendedNextActionField) + if err := appendHotReloadCompileSucceededMessage(fields); err != nil { + return nil, err } return json.Marshal(fields) } +// A Message that is missing or not a string is left alone: only an older or unexpected package +// sends one, and inventing a Message would claim a reload summary the Editor never wrote. +// Why the first byte is checked: decoding JSON null into a string succeeds and leaves it empty, +// so the decode alone would turn a null Message into one that holds only the suffix. +func appendHotReloadCompileSucceededMessage(fields map[string]json.RawMessage) error { + raw := fields[hotReloadMessageField] + if len(raw) == 0 || raw[0] != '"' { + return nil + } + message := "" + if err := json.Unmarshal(raw, &message); err != nil { + return err + } + appended, err := json.Marshal(message + hotReloadCompileFallbackSucceededMessageSuffix) + if err != nil { + return err + } + fields[hotReloadMessageField] = appended + return nil +} + // hotReloadUnappliedPointer names the response field that explains the unapplied edits, so the // note never sends the reader to an empty Warnings array. func hotReloadUnappliedPointer(fields map[string]json.RawMessage) string { @@ -150,7 +183,11 @@ func hotReloadUnappliedPointer(fields map[string]json.RawMessage) string { return hotReloadUnappliedInWarnings } -func hotReloadCompileFallbackAdvice(compileSucceeded bool, unappliedPointer string) (json.RawMessage, json.RawMessage, error) { +func hotReloadCompileFallbackAdvice( + compileSucceeded bool, + unappliedPointer string, + compileNextActions []string, +) (json.RawMessage, json.RawMessage, error) { if compileSucceeded { note, err := json.Marshal(fmt.Sprintf(hotReloadCompileFallbackSucceededNoteFormat, unappliedPointer)) if err != nil { @@ -162,9 +199,19 @@ func hotReloadCompileFallbackAdvice(compileSucceeded bool, unappliedPointer stri if err != nil { return nil, nil, err } - nextAction, err := json.Marshal(hotReloadCompileFallbackFailedNextAction) + nextAction, err := json.Marshal(hotReloadCompileFallbackFailedNextActionText(compileNextActions)) if err != nil { return nil, nil, err } return note, nextAction, nil } + +// A compile that refused to run (for example, in Play Mode under a setting that holds compiles) +// reports no errors, so pointing at Compile.Errors would send the reader to an empty list; the +// compile's own next actions say what actually unblocks it. +func hotReloadCompileFallbackFailedNextActionText(compileNextActions []string) string { + if len(compileNextActions) == 0 { + return hotReloadCompileFallbackFailedNextAction + } + return strings.Join(compileNextActions, " ") +} diff --git a/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback_test.go b/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback_test.go index d1e2cfd5bd..2f7ff77d85 100644 --- a/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback_test.go +++ b/cli/project-runner/internal/projectrunner/hot_reload_compile_fallback_test.go @@ -16,7 +16,7 @@ import ( func TestRunHotReloadRunsFallbackCompileAndAdoptsItsSuccess(t *testing.T) { stdout, stderr, compileCalls, code := runHotReloadWithFakeCompile( t, - `{"Success":false,"CompileFallback":"Requested","RecommendedNextAction":"Run 'uloop compile'"}`, + `{"Success":false,"CompileFallback":"Requested","RecommendedNextAction":"Run 'uloop compile'","Message":"Hot reload finished with one or more Failed outcomes."}`, compileExecutionResult{result: json.RawMessage(`{"Success":true,"Message":"ok"}`), exitCode: 0}, ) @@ -44,6 +44,43 @@ func TestRunHotReloadRunsFallbackCompileAndAdoptsItsSuccess(t *testing.T) { if _, present := fields["RecommendedNextAction"]; present { t.Fatalf("RecommendedNextAction must be dropped once the compile succeeded: %s", stdout) } + message := "" + if err := json.Unmarshal(fields["Message"], &message); err != nil { + t.Fatalf("Message must stay a string: %v\n%s", err, stdout) + } + if message != "Hot reload finished with one or more Failed outcomes. A compile then ran in this same command and succeeded; see CompileFallbackNote." { + t.Fatalf("Message must say the compile succeeded after the reload: %q", message) + } +} + +// Verifies a hot-reload response whose Message is JSON null keeps it null when the fallback compile +// succeeds, instead of becoming a Message that holds only the compile sentence. +func TestInjectHotReloadCompileFallbackLeavesANullMessageNull(t *testing.T) { + merged, err := injectHotReloadCompileFallback( + json.RawMessage(`{"Success":false,"CompileFallback":"Requested","Message":null}`), + json.RawMessage(`{"Success":true}`)) + if err != nil { + t.Fatalf("inject failed: %v", err) + } + fields := decodeSingleJSONObject(t, string(merged)) + if string(fields["Message"]) != "null" { + t.Fatalf("a null Message must stay null: %s", merged) + } +} + +// Verifies a hot-reload response without a Message gets none invented when the fallback compile +// succeeds. +func TestInjectHotReloadCompileFallbackLeavesAMissingMessageMissing(t *testing.T) { + merged, err := injectHotReloadCompileFallback( + json.RawMessage(`{"Success":false,"CompileFallback":"Requested"}`), + json.RawMessage(`{"Success":true}`)) + if err != nil { + t.Fatalf("inject failed: %v", err) + } + fields := decodeSingleJSONObject(t, string(merged)) + if _, present := fields["Message"]; present { + t.Fatalf("Message must not be invented: %s", merged) + } } // Verifies a failed fallback compile flips Success to false, points at Compile.Errors, and returns @@ -74,6 +111,51 @@ func TestRunHotReloadReportsFallbackCompileFailure(t *testing.T) { } } +// Verifies a failed fallback compile that reports its own next actions — such as a compile Unity +// refused during Play Mode — promotes them, joined into one sentence, instead of pointing at +// Compile.Errors; an empty list keeps the fixed advice. +func TestInjectHotReloadCompileFallbackPromotesTheCompileNextActions(t *testing.T) { + cases := []struct { + name string + compile string + wantNextAct string + }{ + { + name: "one next action", + compile: `{"Success":false,"NextActions":["Run 'uloop control-play-mode --action Stop' to leave Play Mode, then rerun 'uloop compile'."]}`, + wantNextAct: "Run 'uloop control-play-mode --action Stop' to leave Play Mode, then rerun 'uloop compile'.", + }, + { + name: "several next actions", + compile: `{"Success":false,"NextActions":["First step.","Second step."]}`, + wantNextAct: "First step. Second step.", + }, + { + name: "empty next actions", + compile: `{"Success":false,"NextActions":[]}`, + wantNextAct: "Fix the errors in Compile.Errors, then rerun 'uloop compile' or 'uloop hot-reload'.", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + merged, err := injectHotReloadCompileFallback( + json.RawMessage(`{"Success":true,"CompileFallback":"Requested"}`), + json.RawMessage(tc.compile)) + if err != nil { + t.Fatalf("inject failed: %v", err) + } + fields := decodeSingleJSONObject(t, string(merged)) + nextAction := "" + if err := json.Unmarshal(fields["RecommendedNextAction"], &nextAction); err != nil { + t.Fatalf("RecommendedNextAction must be a string: %v\n%s", err, merged) + } + if nextAction != tc.wantNextAct { + t.Fatalf("RecommendedNextAction mismatch: %q", nextAction) + } + }) + } +} + // Verifies every answer other than Requested — including a missing field from an older package and // a value this CLI does not know — leaves the response untouched and runs no compile. func TestRunHotReloadSkipsFallbackCompileUnlessRequested(t *testing.T) { From 4c694bcd99c2c6703e69baecd53591d1a907a20e Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Fri, 18 Sep 2026 18:16:52 +0900 Subject: [PATCH 3/3] fix: Hot reload messages no longer overstate sibling failures or hide where the counts came from (#2833) --- .agents/skills/uloop-hot-reload/SKILL.md | 4 +- .../references/introduced-types.md | 11 +- .../uloop-hot-reload/references/output.md | 4 +- .../references/captured-variables.md | 2 +- .claude/skills/uloop-hot-reload/SKILL.md | 4 +- .../references/introduced-types.md | 11 +- .../uloop-hot-reload/references/output.md | 4 +- .../references/captured-variables.md | 2 +- .../HotReload/HotReloadCrossFileE2ETests.cs | 12 +- ...HotReloadIntroducedTypeBodyEditE2ETests.cs | 124 +++++++++++++++++- ...ReloadSiblingRebindWarningSelectorTests.cs | 84 ++++++++++++ ...dSiblingRebindWarningSelectorTests.cs.meta | 11 ++ .../Editor/HotReload/HotReloadToolTests.cs | 53 ++++++++ .../Skill/references/captured-variables.md | 2 +- .../HotReloadApplyResponseBuilder.cs | 23 +++- .../HotReloadRequestedFileOutcomeSummary.cs | 57 ++++++-- .../HotReloadSiblingRebindReporter.cs | 47 +------ .../HotReloadSiblingRebindWarningSelector.cs | 50 +++++++ ...ReloadSiblingRebindWarningSelector.cs.meta | 11 ++ .../HotReload/Shared/HotReloadConstants.cs | 7 + .../FirstPartyTools/HotReload/Skill/SKILL.md | 4 +- .../Skill/references/introduced-types.md | 11 +- .../HotReload/Skill/references/output.md | 4 +- 23 files changed, 460 insertions(+), 82 deletions(-) create mode 100644 Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs create mode 100644 Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs create mode 100644 Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs.meta diff --git a/.agents/skills/uloop-hot-reload/SKILL.md b/.agents/skills/uloop-hot-reload/SKILL.md index 2f70bb44cd..9a571781ef 100644 --- a/.agents/skills/uloop-hot-reload/SKILL.md +++ b/.agents/skills/uloop-hot-reload/SKILL.md @@ -74,8 +74,8 @@ changed are patched (`UnchangedTotal` counts the rest). - Signature changes: a return-type change is `Skipped` unless this reload or an earlier one patched every live compiled caller of the old signature; a rename or parameter change applies as an added method and warns about the call sites left on the old signature. -- Constructors, operators, compiled setter/init/indexer accessors, and event accessors are - `Skipped`; finalizers and interface members are silently not applied. +- Constructors, operators, struct methods, compiled setter/init/indexer accessors, and event + accessors are `Skipped`; finalizers and interface members are silently not applied. - A reload applies each file all-or-nothing: a `Failed` method leaves that file unapplied, a `Failed` type leaves every file of its assembly unapplied. diff --git a/.agents/skills/uloop-hot-reload/references/introduced-types.md b/.agents/skills/uloop-hot-reload/references/introduced-types.md index 85ef39762e..cd039188c9 100644 --- a/.agents/skills/uloop-hot-reload/references/introduced-types.md +++ b/.agents/skills/uloop-hot-reload/references/introduced-types.md @@ -38,6 +38,9 @@ apply, and patches from earlier reloads stay active: nothing for it. Not an error. Editing only the bodies of its ordinary methods keeps this row and patches those bodies on the artifact that already carries the type. Ordinary methods, fields and properties added to the type keep it as well and are applied as `Added` rows. + A struct is the exception: its method bodies are `Skipped` ("Struct (value type) methods are + skipped…") on an introduced struct as on a compiled one, so a struct body edit needs + `uloop compile`. - `Failed` — refused; see the table above. - `ActiveIntroducedTypeTotal` counts the types the domain holds after the run, whatever the methods did. Type rows never count toward `PatchedTotal`, `ActivePatchTotal`, @@ -63,8 +66,9 @@ later apply re-introduces them. With Enter Play Mode Options set to disable Doma active changes and the introduced types survive Play entry and nothing is recorded as dropped. Values are not preserved across the reload that ends a type's life. Body-only edits of an -introduced type's ordinary methods are patched on the artifact assembly, and added ordinary -methods, fields and properties are applied as `Added` rows. Constructor, accessor and +introduced type's ordinary methods are patched on the artifact assembly (except on a struct, +whose method bodies are `Skipped`), and added ordinary methods, fields and properties are +applied as `Added` rows. Constructor, accessor and initializer bodies, member removals, signature changes, and added constructors, operators, events, indexers or nested types still require a compile. @@ -73,7 +77,8 @@ events, indexers or nested types still require a compile. Any refused shape above; use of the type from another assembly, from a file that is neither passed to this reload nor already hot-reloaded, or from `uloop execute-dynamic-code`; anything that reaches the type through Unity (serialization, `[SerializeField]`, Inspector, -`AddComponent`, `CreateInstance`, message discovery); a call to a member an earlier or the same +`AddComponent`, `CreateInstance`, message discovery); a method body edit of an introduced +struct, which is `Skipped` like any struct method; a call to a member an earlier or the same reload *added* to a compiled type (an `Added` row), because introduced types compile against the compiled assemblies and retained artifacts only, so the compile fails naming the missing member; and any new or changed `.asmdef` / `.asmref`. When `execute-dynamic-code` fails on an introduced diff --git a/.agents/skills/uloop-hot-reload/references/output.md b/.agents/skills/uloop-hot-reload/references/output.md index 339b7ad7b5..ce6cf9222b 100644 --- a/.agents/skills/uloop-hot-reload/references/output.md +++ b/.agents/skills/uloop-hot-reload/references/output.md @@ -6,7 +6,7 @@ Returns JSON with: - `ErrorCode` (string, optional): Present on parameter validation failure. Values are `HOT_RELOAD_FILES_REQUIRED` when an omitted apply has no compile snapshots, `HOT_RELOAD_NO_CHANGED_FILES` when snapshots contain no changed `.cs` files, `HOT_RELOAD_INVALID_FILES` when `--files` contains a null or empty path, and `HOT_RELOAD_STATUS_CONFLICT` when `--status` is combined with `--files` or `--revert-all`. - `NextActions` (array, optional): Ordered recovery steps, present only with `ErrorCode` on a parameter validation failure. Omitted from every other response, including successful apply, plain `--status`, and `--revert-all` runs. - `Methods` (array): Per-method `{ Kind, Method, Reason, FilePath, InvocationCount, LifecycleNote }` where `Kind` is `Patched`, `Skipped`, `Failed`, `Added`, `AlreadyActive`, or `Stale` on apply runs, and `Active`, `Added`, or `AddedField` on `--status` runs; empty on `--revert-all` runs. `AlreadyActive` means this file's source matched the last fully applied reload (a run with no Skipped or Failed outcomes), so the existing patch was left in place and the row carries the live `InvocationCount`. `Stale` means the method was deleted from the edited source while its patch is still installed: compiled callers keep running the patched body until `uloop compile`, `--revert-all`, or a later reload whose source restores the method to the compiled baseline clears it; a reload that declares the method with a different body replaces the patch instead of clearing it. Stale rows keep counting toward `ActivePatchTotal`, and the Message summary includes `Stale=N`. `InvocationCount` is meaningful on `Active` rows and on `AlreadyActive` and `Stale` apply rows (calls since the current patch was applied); it is `0` on other apply/revert outcomes. On `--status`, an `Active` row with `InvocationCount` 0 sets `Reason` to explain that the method has not run since the patch: finished calls do not re-run, the patched body takes effect on the next call, and how to retrigger an initialization-only path. When the edited source later declares a different signature, that `Active` row's `Reason` instead explains it is superseded by a new declaration of that signature and is no longer the entry point for new calls (superseded wins over the never-invoked sentence). Added-member rows always show InvocationCount 0 — added-member calls are not instrumented, and the row's Reason says so. `AddedField` rows list a live added field as `Type.field` with an empty `Reason`; they are not method patches. `LifecycleNote` is set when a patched method is a Unity one-shot lifecycle message (`private void Awake`/`Start`/`OnEnable`/`OnDisable`/`OnDestroy` on a `MonoBehaviour`), or when every compiled call path into the patched method (callers of callers are followed a few levels within the compiled assemblies) starts at such a message; empty otherwise — it does not change `Kind`. On an `Added` row whose method is a Unity message, `LifecycleNote` instead says whether the engine will reach it: a forwarded message (`Start`, `Update`, the collision/trigger/mouse messages, and the rest listed in [scope-and-limits.md](scope-and-limits.md)) carries the note that a hot-reload proxy component delivers it to live instances while Play Mode runs, that the proxy is rebuilt only when a later reload changes which messages the type adds or their signatures, that execution order relative to other components is not guaranteed, and that it is gone on any compile or domain reload (only an added `Start` row also says that it runs once on each existing instance when the proxy attaches, and again when the proxy is rebuilt); a message this feature leaves to the compiler (`Awake`, `OnEnable`, `OnDisable`, `OnDestroy`, the editor-only messages, and any non-void message) carries the note that the engine does not invoke it until `uloop compile`, and the run adds one `Warnings` line naming every such message together. `Added` rows carry the added member's signature and file; their `InvocationCount` is always `0` (added-member calls are not instrumented). Example `--status` row: `{ "Kind": "Added", "Method": "Ns.Host.NewHelper(System.Int32)", "Reason": "Added-member calls are not instrumented, so InvocationCount is always 0 for this row.", "FilePath": "Assets/Scripts/Host.cs", "InvocationCount": 0, "LifecycleNote": "" }` -- `Warnings` (array): Non-fatal notes — one aggregated line listing the patched methods at risk of being already JIT-inlined into existing callers — those marked `[AggressiveInlining]`, plus (only when Code Optimization is Release) those with tiny pre-patch bodies — meaning the change may not show at those call sites, the pause-point interaction (see [pause-point-interaction.md](pause-point-interaction.md)), and the const drift, outside-body drift, and missing-baseline entries described in [scope-and-limits.md](scope-and-limits.md). Skipped outcomes are echoed here as `Skipped : `, or as one `Skipped N methods: ()` line per reason when several share it, so checking `Warnings` alone is enough to see that an edit was not applied. When a reload re-applies unchanged files so their patches bind to this run's shim, Warnings includes `Also re-applied N unchanged file(s) with active patches in assembly '...' so their patches bind to this reload's shim: ...`. When a pulled-in sibling fails in that reload, Warnings includes `'...' was pulled in to re-bind its active patches but this reload failed for it; see its rows for which patches changed and run uloop compile to clear the run.` instead of the re-applied line. When the reload stopped before re-applying anything at all, so that sibling has no rows, Warnings instead includes `'...' was pulled in to re-bind its active patches, but this reload stopped before re-applying them, so its active patches are unchanged. Fix the refused declaration and rerun, or run uloop compile to clear the run.` When a sibling still has active patches but its source changed since they were applied, Warnings includes `'...' has active patches but its source changed since they were applied, so it was not re-applied; pass it to hot-reload to update it.` Line-count differences that belong only to files whose outcomes are all `AlreadyActive` are summarized as one `Continuing from earlier runs:` entry. When a run carries two or more warnings and all of them are hot reload warnings, the Message ends with "A single 'uloop compile' clears all of them at once." — read it as the shortest recovery, not as an obligation to compile immediately. Pause-point warnings carry their own recovery steps, so that line does not appear when they are present. +- `Warnings` (array): Non-fatal notes — one aggregated line listing the patched methods at risk of being already JIT-inlined into existing callers — those marked `[AggressiveInlining]`, plus (only when Code Optimization is Release) those with tiny pre-patch bodies — meaning the change may not show at those call sites, the pause-point interaction (see [pause-point-interaction.md](pause-point-interaction.md)), and the const drift, outside-body drift, and missing-baseline entries described in [scope-and-limits.md](scope-and-limits.md). Skipped outcomes are echoed here as `Skipped : `, or as one `Skipped N methods: ()` line per reason when several share it, so checking `Warnings` alone is enough to see that an edit was not applied. When a reload re-applies unchanged files so their patches bind to this run's shim, Warnings includes `Also re-applied N unchanged file(s) with active patches in assembly '...' so their patches bind to this reload's shim: ...`. When a pulled-in sibling fails in that reload, Warnings includes `'...' was pulled in to re-bind its active patches but this reload failed for it; see its rows for which patches changed and run uloop compile to clear the run.` instead of the re-applied line. When every row of that sibling was `Skipped` and none failed, Warnings instead includes `'...' was pulled in to re-bind its active patches, but every method there was Skipped this time; see its rows for the reasons. Its earlier patches stay active until uloop compile clears the run.` When the reload stopped before re-applying anything at all, so that sibling has no rows, Warnings instead includes `'...' was pulled in to re-bind its active patches, but this reload stopped before re-applying them, so its active patches are unchanged. Fix the refused declaration and rerun, or run uloop compile to clear the run.` When a sibling still has active patches but its source changed since they were applied, Warnings includes `'...' has active patches but its source changed since they were applied, so it was not re-applied; pass it to hot-reload to update it.` Line-count differences that belong only to files whose outcomes are all `AlreadyActive` are summarized as one `Continuing from earlier runs:` entry. When a run carries two or more warnings and all of them are hot reload warnings, the Message ends with "A single 'uloop compile' clears all of them at once." — read it as the shortest recovery, not as an obligation to compile immediately. Pause-point warnings carry their own recovery steps, so that line does not appear when they are present. - `PatchedTotal` (number): Methods patched in this run - `AddedFields` (array): source-level names ("Type.field") of fields this reload added; their values live outside the compiled type until 'uloop compile'. Every run that adds fields also carries one warning stating that the values live outside the compiled assembly and last only until the next 'uloop compile' or domain reload; the warning names exactly the fields listed in AddedFields. Pause-point `CapturedVariables` never includes these fields; `enable-pause-point` warns when the resolved type has any. - `AddedConsts` (array): source-level names ("Type.const") of consts this reload added. They are folded into edited bodies as literals, so they are not listed in AddedFields and do not emit the added-field lifetime warning. @@ -18,7 +18,7 @@ Returns JSON with: - `ClearedCount` (number): Patches removed by `--revert-all`, or stale patches reverted because their source matched the compiled baseline again - `IntroducedTypes` (array): Per-type `{ Kind, TypeName, AssemblyName, FilePath, Reason }` rows for the type declarations a reload met, always present and empty when there are none. On apply runs `Kind` is `Introduced` (this reload compiled the declaration into a retained assembly and it is now loaded), `AlreadyActive` (the declaration is bound from an assembly an earlier reload retained, so this reload introduced nothing for it), or `Failed` (the declaration was refused — a redefinition of a type already active, the same type declared in more than one file of the group, or a failed artifact compilation; `Reason` says which, and a `Failed` row alone makes `Success` false). On `--status` every row is `Kind` `Active` and lists a type this domain still holds. Declarations a reload simply cannot introduce are reported as `Warnings`, not rows. These rows are never counted in `PatchedTotal`, `ActivePatchTotal`, `AddedFieldTotal`, or `ClearedCount` - `ActiveIntroducedTypeTotal` (number): Introduced types this domain holds after this run or on `--status`, counted per type rather than per compiled artifact; always present and `0` when there is none. `--revert-all` cannot unload them, so its Message says how many stay loaded until the next Domain Reload, and that Auto Refresh stays held for them until `uloop compile` -- `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` +- `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs that pulled in sibling files, Message follows `PatchedTotal` and `Added` with how many of those Patched and Added rows re-applied the siblings' earlier changes (left out when 0). On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` - `RecommendedNextAction` (string): Present when any method or introduced-type outcome is `Failed`. A partial apply (some methods patched or added, or some types left active) says to fix and rerun, run `uloop compile`, or `uloop hot-reload --revert-all`; a failure with nothing applied says to fix and rerun or compile. Omitted on success. - `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), `BlockedByPlayModeSetting` (`--compile-on-skip on` during Play Mode while Unity's "Script Changes While Playing" is "Recompile After Finished Playing", which refuses the compile; `RecommendedNextAction` says to stop Play Mode first), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. - `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. A successful compile drops `RecommendedNextAction` and ends `Message` with a sentence saying the compile succeeded; a failed one sets `RecommendedNextAction` to the compile's own `NextActions` when it reports any, and otherwise to fixing `Compile.Errors`. diff --git a/.agents/skills/uloop-pause-point/references/captured-variables.md b/.agents/skills/uloop-pause-point/references/captured-variables.md index d028e3eee2..f0504dde5f 100644 --- a/.agents/skills/uloop-pause-point/references/captured-variables.md +++ b/.agents/skills/uloop-pause-point/references/captured-variables.md @@ -17,7 +17,7 @@ Read this before interpreting unexpected, missing, or truncated captured values, ## Scopes and the `this` Entry -- `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. `InstanceField` entries come from a reflection walk of the paused instance's declared type, not from the method's IL usage, so a field the method never reads can still appear — and `MaxCapturedVariableCount` still caps the total entry count across all scopes, so a field-heavy type can push some instance fields out of the snapshot. If a specific field you want is missing, read it directly from the live instance instead of waiting on the capped snapshot: while still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live `this` reference, so `execute-dynamic-code` can read any field or property off it regardless of the cap. +- `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. `InstanceField` entries come from a reflection walk of the paused instance's declared type, not from the method's IL usage, so a field the method never reads can still appear — and `MaxCapturedVariableCount` still caps the total entry count across all scopes, so a field-heavy type can push some instance fields out of the snapshot. If a specific field you want is missing, read it directly from the live instance instead of waiting on the capped snapshot: while still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live `this` reference, so `execute-dynamic-code` can read any field or property off it regardless of the cap. It returns a `(bool Found, object Value)` tuple rather than the object itself, so check `Found` and cast `Value` — for example `(bool found, object value) = UloopPausePoint.TryGetCapturedValue("this"); return found ? ((MyComponent)value).someField : null;`. - A `ref`, `out`, or `in` parameter, a pointer, and a `ref struct` value (`Span` and any user-defined `ref struct`) can never be captured, because the snapshot boxes every value and none of these shapes can be boxed. The enable and `pause-point-status` responses list each such parameter with its reason in `NotCapturableVariables` (absent when there are none), so a missing name is explained rather than looking like a capture bug; to observe one of those values, copy the value it refers to into a plain local (dereference a pointer, `ToArray()` a span), or arm the line that consumes it with `--snapshot-timing post-line`. - The snapshot also includes a synthetic `this` entry (Scope `This`) for the paused instance itself, so you can tell which instance or GameObject was hit via its `UnityObjectPath` and `UnityObjectInstanceId`. For an async or coroutine method it resolves to the original outer instance, not the compiler-generated state machine, and static methods emit no `this` entry. While Unity is still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live instance reference (for example so a watch expression can read `transform.position`). - async and coroutine methods work: hoisted locals and the original `this` fields appear under their normal names. diff --git a/.claude/skills/uloop-hot-reload/SKILL.md b/.claude/skills/uloop-hot-reload/SKILL.md index 2f70bb44cd..9a571781ef 100644 --- a/.claude/skills/uloop-hot-reload/SKILL.md +++ b/.claude/skills/uloop-hot-reload/SKILL.md @@ -74,8 +74,8 @@ changed are patched (`UnchangedTotal` counts the rest). - Signature changes: a return-type change is `Skipped` unless this reload or an earlier one patched every live compiled caller of the old signature; a rename or parameter change applies as an added method and warns about the call sites left on the old signature. -- Constructors, operators, compiled setter/init/indexer accessors, and event accessors are - `Skipped`; finalizers and interface members are silently not applied. +- Constructors, operators, struct methods, compiled setter/init/indexer accessors, and event + accessors are `Skipped`; finalizers and interface members are silently not applied. - A reload applies each file all-or-nothing: a `Failed` method leaves that file unapplied, a `Failed` type leaves every file of its assembly unapplied. diff --git a/.claude/skills/uloop-hot-reload/references/introduced-types.md b/.claude/skills/uloop-hot-reload/references/introduced-types.md index 85ef39762e..cd039188c9 100644 --- a/.claude/skills/uloop-hot-reload/references/introduced-types.md +++ b/.claude/skills/uloop-hot-reload/references/introduced-types.md @@ -38,6 +38,9 @@ apply, and patches from earlier reloads stay active: nothing for it. Not an error. Editing only the bodies of its ordinary methods keeps this row and patches those bodies on the artifact that already carries the type. Ordinary methods, fields and properties added to the type keep it as well and are applied as `Added` rows. + A struct is the exception: its method bodies are `Skipped` ("Struct (value type) methods are + skipped…") on an introduced struct as on a compiled one, so a struct body edit needs + `uloop compile`. - `Failed` — refused; see the table above. - `ActiveIntroducedTypeTotal` counts the types the domain holds after the run, whatever the methods did. Type rows never count toward `PatchedTotal`, `ActivePatchTotal`, @@ -63,8 +66,9 @@ later apply re-introduces them. With Enter Play Mode Options set to disable Doma active changes and the introduced types survive Play entry and nothing is recorded as dropped. Values are not preserved across the reload that ends a type's life. Body-only edits of an -introduced type's ordinary methods are patched on the artifact assembly, and added ordinary -methods, fields and properties are applied as `Added` rows. Constructor, accessor and +introduced type's ordinary methods are patched on the artifact assembly (except on a struct, +whose method bodies are `Skipped`), and added ordinary methods, fields and properties are +applied as `Added` rows. Constructor, accessor and initializer bodies, member removals, signature changes, and added constructors, operators, events, indexers or nested types still require a compile. @@ -73,7 +77,8 @@ events, indexers or nested types still require a compile. Any refused shape above; use of the type from another assembly, from a file that is neither passed to this reload nor already hot-reloaded, or from `uloop execute-dynamic-code`; anything that reaches the type through Unity (serialization, `[SerializeField]`, Inspector, -`AddComponent`, `CreateInstance`, message discovery); a call to a member an earlier or the same +`AddComponent`, `CreateInstance`, message discovery); a method body edit of an introduced +struct, which is `Skipped` like any struct method; a call to a member an earlier or the same reload *added* to a compiled type (an `Added` row), because introduced types compile against the compiled assemblies and retained artifacts only, so the compile fails naming the missing member; and any new or changed `.asmdef` / `.asmref`. When `execute-dynamic-code` fails on an introduced diff --git a/.claude/skills/uloop-hot-reload/references/output.md b/.claude/skills/uloop-hot-reload/references/output.md index 339b7ad7b5..ce6cf9222b 100644 --- a/.claude/skills/uloop-hot-reload/references/output.md +++ b/.claude/skills/uloop-hot-reload/references/output.md @@ -6,7 +6,7 @@ Returns JSON with: - `ErrorCode` (string, optional): Present on parameter validation failure. Values are `HOT_RELOAD_FILES_REQUIRED` when an omitted apply has no compile snapshots, `HOT_RELOAD_NO_CHANGED_FILES` when snapshots contain no changed `.cs` files, `HOT_RELOAD_INVALID_FILES` when `--files` contains a null or empty path, and `HOT_RELOAD_STATUS_CONFLICT` when `--status` is combined with `--files` or `--revert-all`. - `NextActions` (array, optional): Ordered recovery steps, present only with `ErrorCode` on a parameter validation failure. Omitted from every other response, including successful apply, plain `--status`, and `--revert-all` runs. - `Methods` (array): Per-method `{ Kind, Method, Reason, FilePath, InvocationCount, LifecycleNote }` where `Kind` is `Patched`, `Skipped`, `Failed`, `Added`, `AlreadyActive`, or `Stale` on apply runs, and `Active`, `Added`, or `AddedField` on `--status` runs; empty on `--revert-all` runs. `AlreadyActive` means this file's source matched the last fully applied reload (a run with no Skipped or Failed outcomes), so the existing patch was left in place and the row carries the live `InvocationCount`. `Stale` means the method was deleted from the edited source while its patch is still installed: compiled callers keep running the patched body until `uloop compile`, `--revert-all`, or a later reload whose source restores the method to the compiled baseline clears it; a reload that declares the method with a different body replaces the patch instead of clearing it. Stale rows keep counting toward `ActivePatchTotal`, and the Message summary includes `Stale=N`. `InvocationCount` is meaningful on `Active` rows and on `AlreadyActive` and `Stale` apply rows (calls since the current patch was applied); it is `0` on other apply/revert outcomes. On `--status`, an `Active` row with `InvocationCount` 0 sets `Reason` to explain that the method has not run since the patch: finished calls do not re-run, the patched body takes effect on the next call, and how to retrigger an initialization-only path. When the edited source later declares a different signature, that `Active` row's `Reason` instead explains it is superseded by a new declaration of that signature and is no longer the entry point for new calls (superseded wins over the never-invoked sentence). Added-member rows always show InvocationCount 0 — added-member calls are not instrumented, and the row's Reason says so. `AddedField` rows list a live added field as `Type.field` with an empty `Reason`; they are not method patches. `LifecycleNote` is set when a patched method is a Unity one-shot lifecycle message (`private void Awake`/`Start`/`OnEnable`/`OnDisable`/`OnDestroy` on a `MonoBehaviour`), or when every compiled call path into the patched method (callers of callers are followed a few levels within the compiled assemblies) starts at such a message; empty otherwise — it does not change `Kind`. On an `Added` row whose method is a Unity message, `LifecycleNote` instead says whether the engine will reach it: a forwarded message (`Start`, `Update`, the collision/trigger/mouse messages, and the rest listed in [scope-and-limits.md](scope-and-limits.md)) carries the note that a hot-reload proxy component delivers it to live instances while Play Mode runs, that the proxy is rebuilt only when a later reload changes which messages the type adds or their signatures, that execution order relative to other components is not guaranteed, and that it is gone on any compile or domain reload (only an added `Start` row also says that it runs once on each existing instance when the proxy attaches, and again when the proxy is rebuilt); a message this feature leaves to the compiler (`Awake`, `OnEnable`, `OnDisable`, `OnDestroy`, the editor-only messages, and any non-void message) carries the note that the engine does not invoke it until `uloop compile`, and the run adds one `Warnings` line naming every such message together. `Added` rows carry the added member's signature and file; their `InvocationCount` is always `0` (added-member calls are not instrumented). Example `--status` row: `{ "Kind": "Added", "Method": "Ns.Host.NewHelper(System.Int32)", "Reason": "Added-member calls are not instrumented, so InvocationCount is always 0 for this row.", "FilePath": "Assets/Scripts/Host.cs", "InvocationCount": 0, "LifecycleNote": "" }` -- `Warnings` (array): Non-fatal notes — one aggregated line listing the patched methods at risk of being already JIT-inlined into existing callers — those marked `[AggressiveInlining]`, plus (only when Code Optimization is Release) those with tiny pre-patch bodies — meaning the change may not show at those call sites, the pause-point interaction (see [pause-point-interaction.md](pause-point-interaction.md)), and the const drift, outside-body drift, and missing-baseline entries described in [scope-and-limits.md](scope-and-limits.md). Skipped outcomes are echoed here as `Skipped : `, or as one `Skipped N methods: ()` line per reason when several share it, so checking `Warnings` alone is enough to see that an edit was not applied. When a reload re-applies unchanged files so their patches bind to this run's shim, Warnings includes `Also re-applied N unchanged file(s) with active patches in assembly '...' so their patches bind to this reload's shim: ...`. When a pulled-in sibling fails in that reload, Warnings includes `'...' was pulled in to re-bind its active patches but this reload failed for it; see its rows for which patches changed and run uloop compile to clear the run.` instead of the re-applied line. When the reload stopped before re-applying anything at all, so that sibling has no rows, Warnings instead includes `'...' was pulled in to re-bind its active patches, but this reload stopped before re-applying them, so its active patches are unchanged. Fix the refused declaration and rerun, or run uloop compile to clear the run.` When a sibling still has active patches but its source changed since they were applied, Warnings includes `'...' has active patches but its source changed since they were applied, so it was not re-applied; pass it to hot-reload to update it.` Line-count differences that belong only to files whose outcomes are all `AlreadyActive` are summarized as one `Continuing from earlier runs:` entry. When a run carries two or more warnings and all of them are hot reload warnings, the Message ends with "A single 'uloop compile' clears all of them at once." — read it as the shortest recovery, not as an obligation to compile immediately. Pause-point warnings carry their own recovery steps, so that line does not appear when they are present. +- `Warnings` (array): Non-fatal notes — one aggregated line listing the patched methods at risk of being already JIT-inlined into existing callers — those marked `[AggressiveInlining]`, plus (only when Code Optimization is Release) those with tiny pre-patch bodies — meaning the change may not show at those call sites, the pause-point interaction (see [pause-point-interaction.md](pause-point-interaction.md)), and the const drift, outside-body drift, and missing-baseline entries described in [scope-and-limits.md](scope-and-limits.md). Skipped outcomes are echoed here as `Skipped : `, or as one `Skipped N methods: ()` line per reason when several share it, so checking `Warnings` alone is enough to see that an edit was not applied. When a reload re-applies unchanged files so their patches bind to this run's shim, Warnings includes `Also re-applied N unchanged file(s) with active patches in assembly '...' so their patches bind to this reload's shim: ...`. When a pulled-in sibling fails in that reload, Warnings includes `'...' was pulled in to re-bind its active patches but this reload failed for it; see its rows for which patches changed and run uloop compile to clear the run.` instead of the re-applied line. When every row of that sibling was `Skipped` and none failed, Warnings instead includes `'...' was pulled in to re-bind its active patches, but every method there was Skipped this time; see its rows for the reasons. Its earlier patches stay active until uloop compile clears the run.` When the reload stopped before re-applying anything at all, so that sibling has no rows, Warnings instead includes `'...' was pulled in to re-bind its active patches, but this reload stopped before re-applying them, so its active patches are unchanged. Fix the refused declaration and rerun, or run uloop compile to clear the run.` When a sibling still has active patches but its source changed since they were applied, Warnings includes `'...' has active patches but its source changed since they were applied, so it was not re-applied; pass it to hot-reload to update it.` Line-count differences that belong only to files whose outcomes are all `AlreadyActive` are summarized as one `Continuing from earlier runs:` entry. When a run carries two or more warnings and all of them are hot reload warnings, the Message ends with "A single 'uloop compile' clears all of them at once." — read it as the shortest recovery, not as an obligation to compile immediately. Pause-point warnings carry their own recovery steps, so that line does not appear when they are present. - `PatchedTotal` (number): Methods patched in this run - `AddedFields` (array): source-level names ("Type.field") of fields this reload added; their values live outside the compiled type until 'uloop compile'. Every run that adds fields also carries one warning stating that the values live outside the compiled assembly and last only until the next 'uloop compile' or domain reload; the warning names exactly the fields listed in AddedFields. Pause-point `CapturedVariables` never includes these fields; `enable-pause-point` warns when the resolved type has any. - `AddedConsts` (array): source-level names ("Type.const") of consts this reload added. They are folded into edited bodies as literals, so they are not listed in AddedFields and do not emit the added-field lifetime warning. @@ -18,7 +18,7 @@ Returns JSON with: - `ClearedCount` (number): Patches removed by `--revert-all`, or stale patches reverted because their source matched the compiled baseline again - `IntroducedTypes` (array): Per-type `{ Kind, TypeName, AssemblyName, FilePath, Reason }` rows for the type declarations a reload met, always present and empty when there are none. On apply runs `Kind` is `Introduced` (this reload compiled the declaration into a retained assembly and it is now loaded), `AlreadyActive` (the declaration is bound from an assembly an earlier reload retained, so this reload introduced nothing for it), or `Failed` (the declaration was refused — a redefinition of a type already active, the same type declared in more than one file of the group, or a failed artifact compilation; `Reason` says which, and a `Failed` row alone makes `Success` false). On `--status` every row is `Kind` `Active` and lists a type this domain still holds. Declarations a reload simply cannot introduce are reported as `Warnings`, not rows. These rows are never counted in `PatchedTotal`, `ActivePatchTotal`, `AddedFieldTotal`, or `ClearedCount` - `ActiveIntroducedTypeTotal` (number): Introduced types this domain holds after this run or on `--status`, counted per type rather than per compiled artifact; always present and `0` when there is none. `--revert-all` cannot unload them, so its Message says how many stay loaded until the next Domain Reload, and that Auto Refresh stays held for them until `uloop compile` -- `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` +- `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs that pulled in sibling files, Message follows `PatchedTotal` and `Added` with how many of those Patched and Added rows re-applied the siblings' earlier changes (left out when 0). On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` - `RecommendedNextAction` (string): Present when any method or introduced-type outcome is `Failed`. A partial apply (some methods patched or added, or some types left active) says to fix and rerun, run `uloop compile`, or `uloop hot-reload --revert-all`; a failure with nothing applied says to fix and rerun or compile. Omitted on success. - `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), `BlockedByPlayModeSetting` (`--compile-on-skip on` during Play Mode while Unity's "Script Changes While Playing" is "Recompile After Finished Playing", which refuses the compile; `RecommendedNextAction` says to stop Play Mode first), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. - `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. A successful compile drops `RecommendedNextAction` and ends `Message` with a sentence saying the compile succeeded; a failed one sets `RecommendedNextAction` to the compile's own `NextActions` when it reports any, and otherwise to fixing `Compile.Errors`. diff --git a/.claude/skills/uloop-pause-point/references/captured-variables.md b/.claude/skills/uloop-pause-point/references/captured-variables.md index d028e3eee2..f0504dde5f 100644 --- a/.claude/skills/uloop-pause-point/references/captured-variables.md +++ b/.claude/skills/uloop-pause-point/references/captured-variables.md @@ -17,7 +17,7 @@ Read this before interpreting unexpected, missing, or truncated captured values, ## Scopes and the `this` Entry -- `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. `InstanceField` entries come from a reflection walk of the paused instance's declared type, not from the method's IL usage, so a field the method never reads can still appear — and `MaxCapturedVariableCount` still caps the total entry count across all scopes, so a field-heavy type can push some instance fields out of the snapshot. If a specific field you want is missing, read it directly from the live instance instead of waiting on the capped snapshot: while still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live `this` reference, so `execute-dynamic-code` can read any field or property off it regardless of the cap. +- `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. `InstanceField` entries come from a reflection walk of the paused instance's declared type, not from the method's IL usage, so a field the method never reads can still appear — and `MaxCapturedVariableCount` still caps the total entry count across all scopes, so a field-heavy type can push some instance fields out of the snapshot. If a specific field you want is missing, read it directly from the live instance instead of waiting on the capped snapshot: while still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live `this` reference, so `execute-dynamic-code` can read any field or property off it regardless of the cap. It returns a `(bool Found, object Value)` tuple rather than the object itself, so check `Found` and cast `Value` — for example `(bool found, object value) = UloopPausePoint.TryGetCapturedValue("this"); return found ? ((MyComponent)value).someField : null;`. - A `ref`, `out`, or `in` parameter, a pointer, and a `ref struct` value (`Span` and any user-defined `ref struct`) can never be captured, because the snapshot boxes every value and none of these shapes can be boxed. The enable and `pause-point-status` responses list each such parameter with its reason in `NotCapturableVariables` (absent when there are none), so a missing name is explained rather than looking like a capture bug; to observe one of those values, copy the value it refers to into a plain local (dereference a pointer, `ToArray()` a span), or arm the line that consumes it with `--snapshot-timing post-line`. - The snapshot also includes a synthetic `this` entry (Scope `This`) for the paused instance itself, so you can tell which instance or GameObject was hit via its `UnityObjectPath` and `UnityObjectInstanceId`. For an async or coroutine method it resolves to the original outer instance, not the compiler-generated state machine, and static methods emit no `this` entry. While Unity is still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live instance reference (for example so a watch expression can read `transform.position`). - async and coroutine methods work: hoisted locals and the original `this` fields appear under their normal names. diff --git a/Assets/Tests/Editor/HotReload/HotReloadCrossFileE2ETests.cs b/Assets/Tests/Editor/HotReload/HotReloadCrossFileE2ETests.cs index a45cd3eb2c..933b0dd0e7 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadCrossFileE2ETests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadCrossFileE2ETests.cs @@ -871,7 +871,8 @@ await HotReloadCompositionRoot.Services.Orchestrator.RunAsync( /// /// What: a sibling pulled in to re-bind is not described as re-applied when the host /// shim compile fails on a broken body beside the added method; isolation reports the - /// caller as Skipped and the live patch stays on the previous body. + /// caller as Skipped, the live patch stays on the previous body, and the sibling gets the + /// Skipped-only warning rather than being told the reload failed for it. /// [Test] public async Task Run_FailedSiblingRebindDoesNotClaimReApplied() @@ -936,7 +937,14 @@ await HotReloadCompositionRoot.Services.Orchestrator.RunAsync( string.Join("\n", second.Warnings)); Assert.That( CountWarningsContaining(second, SiblingRebindFailedWarningNeedle), - Is.EqualTo(1), + Is.EqualTo(0), + string.Join("\n", second.Warnings)); + Assert.That( + second.Warnings, + Does.Contain( + string.Format( + HotReloadConstants.ActiveSiblingRebindSkippedOnlyWarningFormat, + CallerProjectRelativePath())), string.Join("\n", second.Warnings)); Assert.That( new HotReloadCrossFileAddedMemberCaller().Call(new HotReloadCrossFileAddedMemberHost()), diff --git a/Assets/Tests/Editor/HotReload/HotReloadIntroducedTypeBodyEditE2ETests.cs b/Assets/Tests/Editor/HotReload/HotReloadIntroducedTypeBodyEditE2ETests.cs index e75b0d3ffa..098500843f 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadIntroducedTypeBodyEditE2ETests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadIntroducedTypeBodyEditE2ETests.cs @@ -31,6 +31,14 @@ public class HotReloadIntroducedTypeBodyEditE2ETests : HotReloadIntroducedTypeE2 private const string SeedExpression = "_seed"; private const string EditedExpression = "_seed * 2"; private const string NoExtraMembers = ""; + private const string IntroducedStructSimpleName = "HotReloadBodyEditIntroducedStruct"; + private const string IntroducedStructMetadataName = + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload." + IntroducedStructSimpleName; + private const string IntroducedStructReturn = "5"; + private const string EditedStructReturn = "10"; + private const int IntroducedStructValue = 5; + private const string StructHostSkipReason = + "Struct (value type) methods are skipped; byref instance transplant is unverified."; // A constructor the introduced type declares, which hot reload cannot patch and therefore // reports. Kept out of the other reloads' declaration so only this test's fixture carries @@ -258,6 +266,56 @@ await RunReloadAsync( }); } + /// + /// Verifies that editing a method body of an already introduced struct is skipped with the + /// struct-host reason instead of being patched: struct methods are never transplanted, even + /// on an introduced type, so the retained assembly keeps running the body the introducing + /// reload compiled. + /// + [Test] + public async Task Run_IntroducedStructMethodBodyEdited_ReportsTheStructHostSkipRow() + { + string hostPath = FixturePath("HotReloadCrossFileAddedMemberHost.cs"); + string callerPath = FixturePath("HotReloadCrossFileAddedMemberCaller.cs"); + + await RunInIntroducedTypeDomainAsync(async readArtifact => + { + await RunReloadAsync(hostPath, callerPath, CreateStructEdits( + hostPath, + callerPath, + IntroducedStructReturn, + "IntroducedStructHost.cs", + "IntroducedStructCaller.cs")); + Assert.That( + ReadStructValue(readArtifact()), + Is.EqualTo(IntroducedStructValue), + "Precondition: the retained assembly must run the body the first reload compiled."); + + HotReloadOrchestratorResult edited = await RunReloadAsync(hostPath, callerPath, CreateStructEdits( + hostPath, + callerPath, + EditedStructReturn, + "IntroducedStructEditedHost.cs", + "IntroducedStructEditedCaller.cs")); + + HotReloadMethodOutcome structRow = FindStructComputeOutcome(edited); + Assert.That( + structRow.Kind, + Is.EqualTo(HotReloadMethodOutcomeKind.Skipped), + "A method body of an introduced struct must be skipped, not patched.\n" + + DescribeOutcomes(edited)); + Assert.That( + structRow.Reason, + Does.Contain(StructHostSkipReason), + "The skip must name the struct-host limit so the reader knows to compile.\n" + + DescribeOutcomes(edited)); + Assert.That( + ReadStructValue(readArtifact()), + Is.EqualTo(IntroducedStructValue), + "A skipped struct body must leave the retained assembly running the original body."); + }); + } + private static void AssertComputedValue( HotReloadIntroducedTypeArtifact artifact, int expected, @@ -365,6 +423,65 @@ private static void AssertCallerIsPatched(HotReloadOrchestratorResult result) + DescribeOutcomes(result)); } + private static HotReloadMethodOutcome FindStructComputeOutcome(HotReloadOrchestratorResult result) + { + foreach (HotReloadMethodOutcome outcome in result.Methods) + { + if (outcome.Method != null + && outcome.Method.Contains(IntroducedStructSimpleName + ".Compute()", StringComparison.Ordinal)) + { + return outcome; + } + } + + Assert.Fail("The reload must report a row for " + IntroducedStructSimpleName + ".Compute().\n" + + DescribeOutcomes(result)); + return null; + } + + private static int ReadStructValue(HotReloadIntroducedTypeArtifact artifact) + { + Assert.That(artifact, Is.Not.Null, "A reload had to introduce the struct before this check."); + Type introducedType = artifact.Assembly.GetType(IntroducedStructMetadataName, throwOnError: false); + Assert.That(introducedType, Is.Not.Null, "The artifact must hold " + IntroducedStructMetadataName + "."); + + MethodInfo compute = introducedType.GetMethod("Compute", BindingFlags.Instance | BindingFlags.Public); + Assert.That(compute, Is.Not.Null, "The introduced struct must declare Compute()."); + + return (int)compute.Invoke(Activator.CreateInstance(introducedType), Array.Empty()); + } + + // Why the struct has no field: a struct field initializer needs a C# version Unity's + // compiler does not accept, and the edit only has to change a body. + private static Dictionary CreateStructEdits( + string hostPath, + string callerPath, + string returnedValue, + string hostFileName, + string callerFileName) + { + string hostSource = File.ReadAllText(hostPath); + Assert.That(hostSource, Does.Contain(HostTypeAnchor), "Precondition: host type anchor must exist."); + string introduced = + " public struct " + IntroducedStructSimpleName + "\n" + + " {\n" + + " public int Compute()\n" + + " {\n" + + " return " + returnedValue + ";\n" + + " }\n" + + " }\n" + + "\n"; + return new Dictionary + { + [hostPath] = HotReloadTestSourceWriter.WriteEditedSource( + hostFileName, + hostSource.Replace(HostTypeAnchor, introduced + HostTypeAnchor, StringComparison.Ordinal)), + [callerPath] = HotReloadTestSourceWriter.WriteEditedSource( + callerFileName, + CallType(File.ReadAllText(callerPath), IntroducedStructSimpleName)) + }; + } + private static Dictionary CreateIntroducingEdits(string hostPath, string callerPath) { return new Dictionary @@ -503,11 +620,16 @@ private static string InsertIntroducedType( } private static string CallIntroducedType(string callerSource) + { + return CallType(callerSource, IntroducedTypeSimpleName); + } + + private static string CallType(string callerSource, string typeSimpleName) { Assert.That(callerSource, Does.Contain(CallerBodyAnchor), "Precondition: caller body anchor must exist."); return callerSource.Replace( CallerBodyAnchor, - "return new " + IntroducedTypeSimpleName + "().Compute() + host.Value();", + "return new " + typeSimpleName + "().Compute() + host.Value();", StringComparison.Ordinal); } diff --git a/Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs b/Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs new file mode 100644 index 0000000000..1bec57ef68 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs @@ -0,0 +1,84 @@ +using System.Collections.Generic; + +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// Covers which warning a sibling pulled in to re-bind its active patches gets, from the rows + /// the reload wrote for it. + /// + public sealed class HotReloadSiblingRebindWarningSelectorTests + { + private const string SiblingPath = "Assets/Sibling.cs"; + + /// + /// What: a sibling with a Patched row and no Failed row was re-applied, so no warning + /// format is selected for it. + /// + [Test] + public void SelectUnappliedWarningFormat_PatchedWithoutFailure_ReturnsNull() + { + string format = HotReloadSiblingRebindWarningSelector.SelectUnappliedWarningFormat( + CreateResult( + HotReloadMethodOutcome.Patched("Sibling.Earlier", SiblingPath), + HotReloadMethodOutcome.Skipped("Sibling.Skip", "reason", SiblingPath))); + + Assert.That(format, Is.Null); + } + + /// + /// What: a sibling with a Failed row gets the failed-rebind warning even when another of + /// its rows was Patched. + /// + [Test] + public void SelectUnappliedWarningFormat_FailedRow_ReturnsTheFailedWarning() + { + string format = HotReloadSiblingRebindWarningSelector.SelectUnappliedWarningFormat( + CreateResult( + HotReloadMethodOutcome.Patched("Sibling.Earlier", SiblingPath), + HotReloadMethodOutcome.Failed("Sibling.Broken", "reason", SiblingPath))); + + Assert.That(format, Is.EqualTo(HotReloadConstants.ActiveSiblingRebindFailedWarningFormat)); + } + + /// + /// What: a sibling whose every row was Skipped did not fail, so it gets the Skipped-only + /// warning instead of being told the reload failed for it. + /// + [Test] + public void SelectUnappliedWarningFormat_EveryRowSkipped_ReturnsTheSkippedOnlyWarning() + { + string format = HotReloadSiblingRebindWarningSelector.SelectUnappliedWarningFormat( + CreateResult( + HotReloadMethodOutcome.Skipped("Sibling.First", "reason", SiblingPath), + HotReloadMethodOutcome.Skipped("Sibling.Second", "reason", SiblingPath))); + + Assert.That( + format, + Is.EqualTo(HotReloadConstants.ActiveSiblingRebindSkippedOnlyWarningFormat)); + } + + /// + /// What: a sibling the reload wrote no row for gets the stopped-before-re-applying + /// warning, which does not point at rows that were never written. + /// + [Test] + public void SelectUnappliedWarningFormat_NoRows_ReturnsTheStoppedWarning() + { + string format = HotReloadSiblingRebindWarningSelector.SelectUnappliedWarningFormat(CreateResult()); + + Assert.That(format, Is.EqualTo(HotReloadConstants.ActiveSiblingRebindSkippedWarningFormat)); + } + + private static HotReloadFileProcessResult CreateResult(params HotReloadMethodOutcome[] outcomes) + { + return new HotReloadFileProcessResult( + new List(outcomes), + new List(), + patchedCount: 0); + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs.meta new file mode 100644 index 0000000000..33fb4edd79 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadSiblingRebindWarningSelectorTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 6a6599043bce94b94bde887c2eacf2af +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs b/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs index 75fca635c8..77b6cd8d35 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadToolTests.cs @@ -1758,6 +1758,59 @@ public void BuildApplyResponse_AddedAndSkippedOutcomes_CountsSkippedAfterAdded() Does.StartWith("Hot reload applied. PatchedTotal=0, ActivePatchTotal=1. Added: 1. Skipped: 2.")); } + /// + /// What: an applied run that re-applied a sibling's earlier changes says how many of the + /// Patched and Added rows came from those siblings, right after the counts they are part of, + /// so a reader who edited one method is not left wondering where the rest came from. + /// + [Test] + public void BuildApplyResponse_SiblingRowsReapplied_SaysHowManyOfTheCountsCameFromSiblings() + { + HotReloadOrchestratorResult result = new HotReloadOrchestratorResult( + new List + { + HotReloadMethodOutcome.Patched("Type.Edited", "Assets/Requested.cs"), + HotReloadMethodOutcome.Patched("Sibling.Earlier", "Assets/Sibling.cs"), + HotReloadMethodOutcome.Added("Sibling.AddedEarlier", "Assets/Sibling.cs"), + HotReloadMethodOutcome.Skipped("Sibling.Skip", "reason", "Assets/Sibling.cs") + }, + new List(), + patchedTotal: 2, + activePatchTotal: 2, + reappliedSiblingPaths: new[] { "Assets/Sibling.cs" }); + + HotReloadResponse response = HotReloadTool.BuildApplyResponse(result); + + Assert.That( + response.Message, + Does.StartWith( + "Hot reload applied. PatchedTotal=2, ActivePatchTotal=2. Added: 1. " + + "Of these, 2 re-applied changes from earlier reloads in sibling files. Skipped: 1.")); + } + + /// + /// What: a pulled-in sibling whose rows were all Skipped re-applied nothing, so the message + /// adds no re-applied count for it. + /// + [Test] + public void BuildApplyResponse_SiblingRowsAllSkipped_AddsNoReappliedCount() + { + HotReloadOrchestratorResult result = new HotReloadOrchestratorResult( + new List + { + HotReloadMethodOutcome.Patched("Type.Edited", "Assets/Requested.cs"), + HotReloadMethodOutcome.Skipped("Sibling.Skip", "reason", "Assets/Sibling.cs") + }, + new List(), + patchedTotal: 1, + activePatchTotal: 1, + reappliedSiblingPaths: new[] { "Assets/Sibling.cs" }); + + HotReloadResponse response = HotReloadTool.BuildApplyResponse(result); + + Assert.That(response.Message, Does.Not.Contain("re-applied changes from earlier reloads")); + } + /// /// What: an applied run without any Skipped outcome adds no Skipped-derived warning. /// diff --git a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/captured-variables.md b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/captured-variables.md index d028e3eee2..f0504dde5f 100644 --- a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/captured-variables.md +++ b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/captured-variables.md @@ -17,7 +17,7 @@ Read this before interpreting unexpected, missing, or truncated captured values, ## Scopes and the `this` Entry -- `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. `InstanceField` entries come from a reflection walk of the paused instance's declared type, not from the method's IL usage, so a field the method never reads can still appear — and `MaxCapturedVariableCount` still caps the total entry count across all scopes, so a field-heavy type can push some instance fields out of the snapshot. If a specific field you want is missing, read it directly from the live instance instead of waiting on the capped snapshot: while still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live `this` reference, so `execute-dynamic-code` can read any field or property off it regardless of the cap. +- `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. `InstanceField` entries come from a reflection walk of the paused instance's declared type, not from the method's IL usage, so a field the method never reads can still appear — and `MaxCapturedVariableCount` still caps the total entry count across all scopes, so a field-heavy type can push some instance fields out of the snapshot. If a specific field you want is missing, read it directly from the live instance instead of waiting on the capped snapshot: while still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live `this` reference, so `execute-dynamic-code` can read any field or property off it regardless of the cap. It returns a `(bool Found, object Value)` tuple rather than the object itself, so check `Found` and cast `Value` — for example `(bool found, object value) = UloopPausePoint.TryGetCapturedValue("this"); return found ? ((MyComponent)value).someField : null;`. - A `ref`, `out`, or `in` parameter, a pointer, and a `ref struct` value (`Span` and any user-defined `ref struct`) can never be captured, because the snapshot boxes every value and none of these shapes can be boxed. The enable and `pause-point-status` responses list each such parameter with its reason in `NotCapturableVariables` (absent when there are none), so a missing name is explained rather than looking like a capture bug; to observe one of those values, copy the value it refers to into a plain local (dereference a pointer, `ToArray()` a span), or arm the line that consumes it with `--snapshot-timing post-line`. - The snapshot also includes a synthetic `this` entry (Scope `This`) for the paused instance itself, so you can tell which instance or GameObject was hit via its `UnityObjectPath` and `UnityObjectInstanceId`. For an async or coroutine method it resolves to the original outer instance, not the compiler-generated state machine, and static methods emit no `this` entry. While Unity is still paused, `UloopPausePoint.TryGetCapturedValue("this")` returns the live instance reference (for example so a watch expression can read `transform.position`). - async and coroutine methods work: hoisted locals and the original `this` fields appear under their normal names. diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs index e3d3318484..1ae320536c 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadApplyResponseBuilder.cs @@ -115,6 +115,10 @@ public static HotReloadResponse Build( warnings, result.AutoRefreshHoldSceneRefreshWarning); bool allRequestedSkipped = DecideAllRequestedSkipped(result, toProjectRelativeScriptPath); + int reappliedSiblingCount = HotReloadRequestedFileOutcomeSummary.CountReappliedSiblingOutcomes( + result.Methods, + result.ReappliedSiblingPaths, + toProjectRelativeScriptPath); string message = BuildApplyMessage( result, hasFailure, @@ -122,7 +126,8 @@ public static HotReloadResponse Build( warnings.Count, appendCompileResolution: orchestratorWarningCount >= 2 && orchestratorWarningCount == warningCountBeforeHold, - allRequestedSkipped); + allRequestedSkipped, + reappliedSiblingCount); return new HotReloadResponse { Success = !hasFailure, @@ -221,7 +226,8 @@ private static string BuildApplyMessage( bool hasMethodFailure, int warningCount, bool appendCompileResolution, - bool allRequestedSkipped) + bool allRequestedSkipped, + int reappliedSiblingCount) { // Why asked first: the file a run introduces a type into usually holds untouched // methods as well, and every message below would then report the methods only. @@ -247,7 +253,7 @@ private static string BuildApplyMessage( appendCompileResolution); } - string message = BuildApplyOutcomeMessage(result, hasFailure, allRequestedSkipped); + string message = BuildApplyOutcomeMessage(result, hasFailure, allRequestedSkipped, reappliedSiblingCount); message = AppendUnchangedAndLifecycleNotes(message, result); message = HotReloadIntroducedTypeResponseSection.AppendTypeSummary( message, @@ -265,7 +271,8 @@ private static bool ReadsInvocationCountFromLedger(HotReloadMethodOutcomeKind ki private static string BuildApplyOutcomeMessage( HotReloadOrchestratorResult result, bool hasFailure, - bool allRequestedSkipped) + bool allRequestedSkipped, + int reappliedSiblingCount) { int addedCount = CountAddedOutcomes(result); if (hasFailure) @@ -309,6 +316,14 @@ private static string BuildApplyOutcomeMessage( message += " Added: " + addedCount + "."; } + // Why: a sibling re-apply adds its earlier rows to both counts, so a reader who edited + // one method otherwise cannot tell why the counts are much larger than the edit. + if (reappliedSiblingCount > 0) + { + message += " Of these, " + reappliedSiblingCount + + " re-applied changes from earlier reloads in sibling files."; + } + // Why counted here: the totals only count what was applied, so a run that skipped // some of the edits otherwise reads as if every one of them took effect. int skippedCount = CountOutcomesOfKind(result, HotReloadMethodOutcomeKind.Skipped); diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadRequestedFileOutcomeSummary.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadRequestedFileOutcomeSummary.cs index a5c9e0df3e..ba1713a9f6 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadRequestedFileOutcomeSummary.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadRequestedFileOutcomeSummary.cs @@ -6,7 +6,8 @@ namespace io.github.hatayama.UnityCliLoop.FirstPartyTools { /// - /// Decides whether a run left the requested files untouched, ignoring sibling re-applies. + /// Separates the outcomes of the files a run was asked about from those of the sibling files + /// it pulled in to re-apply earlier changes. /// internal static class HotReloadRequestedFileOutcomeSummary { @@ -26,13 +27,7 @@ public static bool AreAllRequestedOutcomesSkipped( toProjectRelativeScriptPath != null, "toProjectRelativeScriptPath must not be null."); - StringComparer fileComparer = Application.platform == RuntimePlatform.WindowsEditor - ? StringComparer.OrdinalIgnoreCase - : StringComparer.Ordinal; - HashSet siblingKeys = BuildSiblingKeys( - reappliedSiblingPaths, - toProjectRelativeScriptPath, - fileComparer); + HashSet siblingKeys = BuildSiblingKeys(reappliedSiblingPaths, toProjectRelativeScriptPath); int requestedCount = 0; for (int index = 0; index < methods.Count; index++) { @@ -57,11 +52,53 @@ public static bool AreAllRequestedOutcomesSkipped( return requestedCount > 0; } + /// + /// Counts the Patched and Added rows that belong to a sibling file the run pulled in to + /// re-apply its earlier changes, as opposed to the files the caller asked about. + /// + // Why only Patched and Added: the sibling paths list every pulled-in file whatever + // happened to it, and only these two kinds are part of the counts the message reports. + public static int CountReappliedSiblingOutcomes( + IReadOnlyList methods, + IReadOnlyCollection reappliedSiblingPaths, + Func toProjectRelativeScriptPath) + { + Debug.Assert(methods != null, "methods must not be null."); + Debug.Assert(reappliedSiblingPaths != null, "reappliedSiblingPaths must not be null."); + Debug.Assert( + toProjectRelativeScriptPath != null, + "toProjectRelativeScriptPath must not be null."); + + HashSet siblingKeys = BuildSiblingKeys(reappliedSiblingPaths, toProjectRelativeScriptPath); + int count = 0; + for (int index = 0; index < methods.Count; index++) + { + HotReloadMethodOutcome outcome = methods[index]; + if (outcome.Kind != HotReloadMethodOutcomeKind.Patched + && outcome.Kind != HotReloadMethodOutcomeKind.Added) + { + continue; + } + + if (!string.IsNullOrEmpty(outcome.FilePath) + && siblingKeys.Contains(toProjectRelativeScriptPath(outcome.FilePath))) + { + count++; + } + } + + return count; + } + + // Why the path is normalized and compared per platform: an outcome's FilePath can be + // absolute while the sibling paths are project-relative, and Windows paths ignore case. private static HashSet BuildSiblingKeys( IReadOnlyCollection reappliedSiblingPaths, - Func toProjectRelativeScriptPath, - StringComparer fileComparer) + Func toProjectRelativeScriptPath) { + StringComparer fileComparer = Application.platform == RuntimePlatform.WindowsEditor + ? StringComparer.OrdinalIgnoreCase + : StringComparer.Ordinal; HashSet keys = new HashSet(fileComparer); foreach (string path in reappliedSiblingPaths) { diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindReporter.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindReporter.cs index 3d7021ae24..0b1be0a069 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindReporter.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindReporter.cs @@ -70,27 +70,15 @@ internal void AddSiblingRebindResultWarnings( for (int position = inputCount; position < filesOfGroup.Count; position++) { string path = filesOfGroup[position].ProjectRelativePath; - if (ShouldDescribeSiblingAsReapplied(groupResults[position])) + string warningFormat = + HotReloadSiblingRebindWarningSelector.SelectUnappliedWarningFormat(groupResults[position]); + if (warningFormat == null) { reappliedPaths.Add(path); + continue; } - else if (groupResults[position].Outcomes.Count == 0) - { - // A run stopped before it applied anything wrote no row for this file, so - // the failed-rebind sentence would send the reader looking for rows that - // were never written. - groupResults[0].Warnings.Add( - string.Format( - HotReloadConstants.ActiveSiblingRebindSkippedWarningFormat, - path)); - } - else - { - groupResults[0].Warnings.Add( - string.Format( - HotReloadConstants.ActiveSiblingRebindFailedWarningFormat, - path)); - } + + groupResults[0].Warnings.Add(string.Format(warningFormat, path)); } if (reappliedPaths.Count > 0) @@ -148,28 +136,5 @@ private void AddChangedSinceApplyWarnings( changedSinceApplyPaths[index])); } } - - // Why not "no Failed row": isolation leaves a sibling as Skipped when its added-method - // callee failed to compile, and claiming that file was re-applied would be false. - private bool ShouldDescribeSiblingAsReapplied(HotReloadFileProcessResult result) - { - Debug.Assert(result != null, "result must not be null."); - bool sawApplied = false; - foreach (HotReloadMethodOutcome outcome in result.Outcomes) - { - if (outcome.Kind == HotReloadMethodOutcomeKind.Failed) - { - return false; - } - - if (outcome.Kind == HotReloadMethodOutcomeKind.Patched - || outcome.Kind == HotReloadMethodOutcomeKind.Added) - { - sawApplied = true; - } - } - - return sawApplied; - } } } diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs new file mode 100644 index 0000000000..9b67609902 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs @@ -0,0 +1,50 @@ +using UnityEngine; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Picks the warning for a sibling pulled in to re-bind its active patches from the rows the + /// reload wrote for it, or none when the sibling was re-applied. + /// + internal static class HotReloadSiblingRebindWarningSelector + { + // Why Failed is checked before Patched/Added: isolation leaves a sibling as Skipped when + // its added-method callee failed to compile, and claiming that file was re-applied would + // be false. Why Skipped-only is separate from Failed: nothing failed for such a sibling, + // so the failed sentence would misdirect the reader. + public static string SelectUnappliedWarningFormat(HotReloadFileProcessResult result) + { + Debug.Assert(result != null, "result must not be null."); + + if (result.Outcomes.Count == 0) + { + // A run stopped before it applied anything wrote no row for this file, so the + // failed-rebind sentence would send the reader looking for rows never written. + return HotReloadConstants.ActiveSiblingRebindSkippedWarningFormat; + } + + bool sawApplied = false; + bool sawOnlySkipped = true; + foreach (HotReloadMethodOutcome outcome in result.Outcomes) + { + if (outcome.Kind == HotReloadMethodOutcomeKind.Failed) + { + return HotReloadConstants.ActiveSiblingRebindFailedWarningFormat; + } + + sawApplied |= outcome.Kind == HotReloadMethodOutcomeKind.Patched + || outcome.Kind == HotReloadMethodOutcomeKind.Added; + sawOnlySkipped &= outcome.Kind == HotReloadMethodOutcomeKind.Skipped; + } + + if (sawApplied) + { + return null; + } + + return sawOnlySkipped + ? HotReloadConstants.ActiveSiblingRebindSkippedOnlyWarningFormat + : HotReloadConstants.ActiveSiblingRebindFailedWarningFormat; + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs.meta b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs.meta new file mode 100644 index 0000000000..4c296b8a9f --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadSiblingRebindWarningSelector.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b48fb3c263f52430f8602a558aa81fa0 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs index 51549f63c8..8be23423c8 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadConstants.cs @@ -96,6 +96,13 @@ internal static class HotReloadConstants + "re-applying them, so its active patches are unchanged. Fix the refused declaration " + "and rerun, or run uloop compile to clear the run."; + // Why a third wording: a sibling whose every row was Skipped did not fail, and its + // earlier patches stay live, so the failed sentence would overstate what happened. + public const string ActiveSiblingRebindSkippedOnlyWarningFormat = + "'{0}' was pulled in to re-bind its active patches, but every method there was Skipped " + + "this time; see its rows for the reasons. Its earlier patches stay active until " + + "uloop compile clears the run."; + public const string ActiveSiblingRebindFailedWarningFormat = "'{0}' was pulled in to re-bind its active patches but this reload failed for it; " + "see its rows for which patches changed and run uloop compile to clear the run."; diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md index 2f70bb44cd..9a571781ef 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/SKILL.md @@ -74,8 +74,8 @@ changed are patched (`UnchangedTotal` counts the rest). - Signature changes: a return-type change is `Skipped` unless this reload or an earlier one patched every live compiled caller of the old signature; a rename or parameter change applies as an added method and warns about the call sites left on the old signature. -- Constructors, operators, compiled setter/init/indexer accessors, and event accessors are - `Skipped`; finalizers and interface members are silently not applied. +- Constructors, operators, struct methods, compiled setter/init/indexer accessors, and event + accessors are `Skipped`; finalizers and interface members are silently not applied. - A reload applies each file all-or-nothing: a `Failed` method leaves that file unapplied, a `Failed` type leaves every file of its assembly unapplied. diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/introduced-types.md b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/introduced-types.md index 85ef39762e..cd039188c9 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/introduced-types.md +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/introduced-types.md @@ -38,6 +38,9 @@ apply, and patches from earlier reloads stay active: nothing for it. Not an error. Editing only the bodies of its ordinary methods keeps this row and patches those bodies on the artifact that already carries the type. Ordinary methods, fields and properties added to the type keep it as well and are applied as `Added` rows. + A struct is the exception: its method bodies are `Skipped` ("Struct (value type) methods are + skipped…") on an introduced struct as on a compiled one, so a struct body edit needs + `uloop compile`. - `Failed` — refused; see the table above. - `ActiveIntroducedTypeTotal` counts the types the domain holds after the run, whatever the methods did. Type rows never count toward `PatchedTotal`, `ActivePatchTotal`, @@ -63,8 +66,9 @@ later apply re-introduces them. With Enter Play Mode Options set to disable Doma active changes and the introduced types survive Play entry and nothing is recorded as dropped. Values are not preserved across the reload that ends a type's life. Body-only edits of an -introduced type's ordinary methods are patched on the artifact assembly, and added ordinary -methods, fields and properties are applied as `Added` rows. Constructor, accessor and +introduced type's ordinary methods are patched on the artifact assembly (except on a struct, +whose method bodies are `Skipped`), and added ordinary methods, fields and properties are +applied as `Added` rows. Constructor, accessor and initializer bodies, member removals, signature changes, and added constructors, operators, events, indexers or nested types still require a compile. @@ -73,7 +77,8 @@ events, indexers or nested types still require a compile. Any refused shape above; use of the type from another assembly, from a file that is neither passed to this reload nor already hot-reloaded, or from `uloop execute-dynamic-code`; anything that reaches the type through Unity (serialization, `[SerializeField]`, Inspector, -`AddComponent`, `CreateInstance`, message discovery); a call to a member an earlier or the same +`AddComponent`, `CreateInstance`, message discovery); a method body edit of an introduced +struct, which is `Skipped` like any struct method; a call to a member an earlier or the same reload *added* to a compiled type (an `Added` row), because introduced types compile against the compiled assemblies and retained artifacts only, so the compile fails naming the missing member; and any new or changed `.asmdef` / `.asmref`. When `execute-dynamic-code` fails on an introduced diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md index 339b7ad7b5..ce6cf9222b 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/output.md @@ -6,7 +6,7 @@ Returns JSON with: - `ErrorCode` (string, optional): Present on parameter validation failure. Values are `HOT_RELOAD_FILES_REQUIRED` when an omitted apply has no compile snapshots, `HOT_RELOAD_NO_CHANGED_FILES` when snapshots contain no changed `.cs` files, `HOT_RELOAD_INVALID_FILES` when `--files` contains a null or empty path, and `HOT_RELOAD_STATUS_CONFLICT` when `--status` is combined with `--files` or `--revert-all`. - `NextActions` (array, optional): Ordered recovery steps, present only with `ErrorCode` on a parameter validation failure. Omitted from every other response, including successful apply, plain `--status`, and `--revert-all` runs. - `Methods` (array): Per-method `{ Kind, Method, Reason, FilePath, InvocationCount, LifecycleNote }` where `Kind` is `Patched`, `Skipped`, `Failed`, `Added`, `AlreadyActive`, or `Stale` on apply runs, and `Active`, `Added`, or `AddedField` on `--status` runs; empty on `--revert-all` runs. `AlreadyActive` means this file's source matched the last fully applied reload (a run with no Skipped or Failed outcomes), so the existing patch was left in place and the row carries the live `InvocationCount`. `Stale` means the method was deleted from the edited source while its patch is still installed: compiled callers keep running the patched body until `uloop compile`, `--revert-all`, or a later reload whose source restores the method to the compiled baseline clears it; a reload that declares the method with a different body replaces the patch instead of clearing it. Stale rows keep counting toward `ActivePatchTotal`, and the Message summary includes `Stale=N`. `InvocationCount` is meaningful on `Active` rows and on `AlreadyActive` and `Stale` apply rows (calls since the current patch was applied); it is `0` on other apply/revert outcomes. On `--status`, an `Active` row with `InvocationCount` 0 sets `Reason` to explain that the method has not run since the patch: finished calls do not re-run, the patched body takes effect on the next call, and how to retrigger an initialization-only path. When the edited source later declares a different signature, that `Active` row's `Reason` instead explains it is superseded by a new declaration of that signature and is no longer the entry point for new calls (superseded wins over the never-invoked sentence). Added-member rows always show InvocationCount 0 — added-member calls are not instrumented, and the row's Reason says so. `AddedField` rows list a live added field as `Type.field` with an empty `Reason`; they are not method patches. `LifecycleNote` is set when a patched method is a Unity one-shot lifecycle message (`private void Awake`/`Start`/`OnEnable`/`OnDisable`/`OnDestroy` on a `MonoBehaviour`), or when every compiled call path into the patched method (callers of callers are followed a few levels within the compiled assemblies) starts at such a message; empty otherwise — it does not change `Kind`. On an `Added` row whose method is a Unity message, `LifecycleNote` instead says whether the engine will reach it: a forwarded message (`Start`, `Update`, the collision/trigger/mouse messages, and the rest listed in [scope-and-limits.md](scope-and-limits.md)) carries the note that a hot-reload proxy component delivers it to live instances while Play Mode runs, that the proxy is rebuilt only when a later reload changes which messages the type adds or their signatures, that execution order relative to other components is not guaranteed, and that it is gone on any compile or domain reload (only an added `Start` row also says that it runs once on each existing instance when the proxy attaches, and again when the proxy is rebuilt); a message this feature leaves to the compiler (`Awake`, `OnEnable`, `OnDisable`, `OnDestroy`, the editor-only messages, and any non-void message) carries the note that the engine does not invoke it until `uloop compile`, and the run adds one `Warnings` line naming every such message together. `Added` rows carry the added member's signature and file; their `InvocationCount` is always `0` (added-member calls are not instrumented). Example `--status` row: `{ "Kind": "Added", "Method": "Ns.Host.NewHelper(System.Int32)", "Reason": "Added-member calls are not instrumented, so InvocationCount is always 0 for this row.", "FilePath": "Assets/Scripts/Host.cs", "InvocationCount": 0, "LifecycleNote": "" }` -- `Warnings` (array): Non-fatal notes — one aggregated line listing the patched methods at risk of being already JIT-inlined into existing callers — those marked `[AggressiveInlining]`, plus (only when Code Optimization is Release) those with tiny pre-patch bodies — meaning the change may not show at those call sites, the pause-point interaction (see [pause-point-interaction.md](pause-point-interaction.md)), and the const drift, outside-body drift, and missing-baseline entries described in [scope-and-limits.md](scope-and-limits.md). Skipped outcomes are echoed here as `Skipped : `, or as one `Skipped N methods: ()` line per reason when several share it, so checking `Warnings` alone is enough to see that an edit was not applied. When a reload re-applies unchanged files so their patches bind to this run's shim, Warnings includes `Also re-applied N unchanged file(s) with active patches in assembly '...' so their patches bind to this reload's shim: ...`. When a pulled-in sibling fails in that reload, Warnings includes `'...' was pulled in to re-bind its active patches but this reload failed for it; see its rows for which patches changed and run uloop compile to clear the run.` instead of the re-applied line. When the reload stopped before re-applying anything at all, so that sibling has no rows, Warnings instead includes `'...' was pulled in to re-bind its active patches, but this reload stopped before re-applying them, so its active patches are unchanged. Fix the refused declaration and rerun, or run uloop compile to clear the run.` When a sibling still has active patches but its source changed since they were applied, Warnings includes `'...' has active patches but its source changed since they were applied, so it was not re-applied; pass it to hot-reload to update it.` Line-count differences that belong only to files whose outcomes are all `AlreadyActive` are summarized as one `Continuing from earlier runs:` entry. When a run carries two or more warnings and all of them are hot reload warnings, the Message ends with "A single 'uloop compile' clears all of them at once." — read it as the shortest recovery, not as an obligation to compile immediately. Pause-point warnings carry their own recovery steps, so that line does not appear when they are present. +- `Warnings` (array): Non-fatal notes — one aggregated line listing the patched methods at risk of being already JIT-inlined into existing callers — those marked `[AggressiveInlining]`, plus (only when Code Optimization is Release) those with tiny pre-patch bodies — meaning the change may not show at those call sites, the pause-point interaction (see [pause-point-interaction.md](pause-point-interaction.md)), and the const drift, outside-body drift, and missing-baseline entries described in [scope-and-limits.md](scope-and-limits.md). Skipped outcomes are echoed here as `Skipped : `, or as one `Skipped N methods: ()` line per reason when several share it, so checking `Warnings` alone is enough to see that an edit was not applied. When a reload re-applies unchanged files so their patches bind to this run's shim, Warnings includes `Also re-applied N unchanged file(s) with active patches in assembly '...' so their patches bind to this reload's shim: ...`. When a pulled-in sibling fails in that reload, Warnings includes `'...' was pulled in to re-bind its active patches but this reload failed for it; see its rows for which patches changed and run uloop compile to clear the run.` instead of the re-applied line. When every row of that sibling was `Skipped` and none failed, Warnings instead includes `'...' was pulled in to re-bind its active patches, but every method there was Skipped this time; see its rows for the reasons. Its earlier patches stay active until uloop compile clears the run.` When the reload stopped before re-applying anything at all, so that sibling has no rows, Warnings instead includes `'...' was pulled in to re-bind its active patches, but this reload stopped before re-applying them, so its active patches are unchanged. Fix the refused declaration and rerun, or run uloop compile to clear the run.` When a sibling still has active patches but its source changed since they were applied, Warnings includes `'...' has active patches but its source changed since they were applied, so it was not re-applied; pass it to hot-reload to update it.` Line-count differences that belong only to files whose outcomes are all `AlreadyActive` are summarized as one `Continuing from earlier runs:` entry. When a run carries two or more warnings and all of them are hot reload warnings, the Message ends with "A single 'uloop compile' clears all of them at once." — read it as the shortest recovery, not as an obligation to compile immediately. Pause-point warnings carry their own recovery steps, so that line does not appear when they are present. - `PatchedTotal` (number): Methods patched in this run - `AddedFields` (array): source-level names ("Type.field") of fields this reload added; their values live outside the compiled type until 'uloop compile'. Every run that adds fields also carries one warning stating that the values live outside the compiled assembly and last only until the next 'uloop compile' or domain reload; the warning names exactly the fields listed in AddedFields. Pause-point `CapturedVariables` never includes these fields; `enable-pause-point` warns when the resolved type has any. - `AddedConsts` (array): source-level names ("Type.const") of consts this reload added. They are folded into edited bodies as literals, so they are not listed in AddedFields and do not emit the added-field lifetime warning. @@ -18,7 +18,7 @@ Returns JSON with: - `ClearedCount` (number): Patches removed by `--revert-all`, or stale patches reverted because their source matched the compiled baseline again - `IntroducedTypes` (array): Per-type `{ Kind, TypeName, AssemblyName, FilePath, Reason }` rows for the type declarations a reload met, always present and empty when there are none. On apply runs `Kind` is `Introduced` (this reload compiled the declaration into a retained assembly and it is now loaded), `AlreadyActive` (the declaration is bound from an assembly an earlier reload retained, so this reload introduced nothing for it), or `Failed` (the declaration was refused — a redefinition of a type already active, the same type declared in more than one file of the group, or a failed artifact compilation; `Reason` says which, and a `Failed` row alone makes `Success` false). On `--status` every row is `Kind` `Active` and lists a type this domain still holds. Declarations a reload simply cannot introduce are reported as `Warnings`, not rows. These rows are never counted in `PatchedTotal`, `ActivePatchTotal`, `AddedFieldTotal`, or `ClearedCount` - `ActiveIntroducedTypeTotal` (number): Introduced types this domain holds after this run or on `--status`, counted per type rather than per compiled artifact; always present and `0` when there is none. `--revert-all` cannot unload them, so its Message says how many stay loaded until the next Domain Reload, and that Auto Refresh stays held for them until `uloop compile` -- `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` +- `Message` (string): Short summary. When a run carries `IntroducedTypes` rows, Message reports them: a run that only introduced or only re-bound types says so instead of reporting the methods, a refused declaration is reported as the failure of the run and points at `IntroducedTypes`, and a run the methods decided ends with `IntroducedTypes=N`. On apply runs that pulled in sibling files, Message follows `PatchedTotal` and `Added` with how many of those Patched and Added rows re-applied the siblings' earlier changes (left out when 0). On apply runs, Message counts the patched rows that carry a `LifecycleNote` in one sentence and the added Unity messages a hot-reload proxy delivers in another, both pointing at `Methods[].LifecycleNote`; forwarded `Added` rows are not in the patched count, and each sentence is left out when its count is 0. On `--status`, Message opens with how many changes are currently active — patched methods, added members, and introduced types together, which is why it can exceed `ActivePatchTotal` — and when any `Active` row has `InvocationCount` 0 it also appends how many such rows there are and points at `Methods[].Reason`; added-member rows are not included in that count. `--revert-all` appends how many introduced types stay loaded until the next Domain Reload, and when the hold is still armed for them, that Auto Refresh stays held until `uloop compile` - `RecommendedNextAction` (string): Present when any method or introduced-type outcome is `Failed`. A partial apply (some methods patched or added, or some types left active) says to fix and rerun, run `uloop compile`, or `uloop hot-reload --revert-all`; a failure with nothing applied says to fix and rerun or compile. Omitted on success. - `CompileFallback` (string, always present): whether the CLI should run a compile after this run — `NotNeeded`, `Requested`, `HeldForPlayMode` (edits stayed unapplied but the Editor is in Play Mode and `--compile-on-skip` is `auto`), `BlockedByPlayModeSetting` (`--compile-on-skip on` during Play Mode while Unity's "Script Changes While Playing" is "Recompile After Finished Playing", which refuses the compile; `RecommendedNextAction` says to stop Play Mode first), or `Disabled` (`--compile-on-skip off`). `--status`, `--revert-all` and validation failures answer `NotNeeded`. - `Compile` (object, present only when the CLI ran the fallback compile): the full `uloop compile` response; the top-level `Success` is then the compile's, and the command's exit code is the compile's. A successful compile drops `RecommendedNextAction` and ends `Message` with a sentence saying the compile succeeded; a failed one sets `RecommendedNextAction` to the compile's own `NextActions` when it reports any, and otherwise to fixing `Compile.Errors`.