diff --git a/.agents/skills/uloop-hot-reload/references/scope-and-limits.md b/.agents/skills/uloop-hot-reload/references/scope-and-limits.md index eda3049cbc..5553989c40 100644 --- a/.agents/skills/uloop-hot-reload/references/scope-and-limits.md +++ b/.agents/skills/uloop-hot-reload/references/scope-and-limits.md @@ -404,6 +404,8 @@ source on disk. When a run skips a method it had patched before, `Warnings` name | Method on a `partial` type when another part of the type changed since the last compile and was not passed, or when a file that names the type has syntax errors (passed or not) | Hot reload binds against the compiled type; pass that file with `--files` too, or run `uloop compile`. For a file with syntax errors, fix it and run hot reload again | | Method on a `partial` type when the other parts could not be checked against the last compile (no source snapshot yet, or more than 50 changed files in the assembly) | Run `uloop compile` | | Method on a `partial` type whose body names a member no source file of the assembly declares | A part generated at compile time (a source generator's output) is not visible to hot reload; run `uloop compile` | +| Method or getter on a `partial` type whose body uses an `internal` member of a type the reload was not given, by its bare name, as a method passed as a delegate, inside a lambda, local function, query, iterator or async method, in a body where a lambda, local function or query works with the member's result, or in a body patched through a delegating shim | Hot reload reaches such a member only as a field, a property or a method call in the method's own statements, written with its receiver (`this.Name`, `Type.Name`, `value.Name`); qualify a bare name, or run `uloop compile` | +| Method or getter on a `partial` type whose body uses an `internal` member of a type in another assembly (through `InternalsVisibleTo`) | Reported as a name no source file of the `partial` type declares; hot reload does not patch this use from a `partial` type yet. Run `uloop compile` | | Method on a struct (value type) | Value-type patching is out of scope | | Generic method, or method on a generic type | Harmony cannot safely patch open generics | | Explicit interface implementation | Dotted metadata names cannot be expressed as shim identifiers | diff --git a/.claude/skills/uloop-hot-reload/references/scope-and-limits.md b/.claude/skills/uloop-hot-reload/references/scope-and-limits.md index eda3049cbc..5553989c40 100644 --- a/.claude/skills/uloop-hot-reload/references/scope-and-limits.md +++ b/.claude/skills/uloop-hot-reload/references/scope-and-limits.md @@ -404,6 +404,8 @@ source on disk. When a run skips a method it had patched before, `Warnings` name | Method on a `partial` type when another part of the type changed since the last compile and was not passed, or when a file that names the type has syntax errors (passed or not) | Hot reload binds against the compiled type; pass that file with `--files` too, or run `uloop compile`. For a file with syntax errors, fix it and run hot reload again | | Method on a `partial` type when the other parts could not be checked against the last compile (no source snapshot yet, or more than 50 changed files in the assembly) | Run `uloop compile` | | Method on a `partial` type whose body names a member no source file of the assembly declares | A part generated at compile time (a source generator's output) is not visible to hot reload; run `uloop compile` | +| Method or getter on a `partial` type whose body uses an `internal` member of a type the reload was not given, by its bare name, as a method passed as a delegate, inside a lambda, local function, query, iterator or async method, in a body where a lambda, local function or query works with the member's result, or in a body patched through a delegating shim | Hot reload reaches such a member only as a field, a property or a method call in the method's own statements, written with its receiver (`this.Name`, `Type.Name`, `value.Name`); qualify a bare name, or run `uloop compile` | +| Method or getter on a `partial` type whose body uses an `internal` member of a type in another assembly (through `InternalsVisibleTo`) | Reported as a name no source file of the `partial` type declares; hot reload does not patch this use from a `partial` type yet. Run `uloop compile` | | Method on a struct (value type) | Value-type patching is out of scope | | Generic method, or method on a generic type | Harmony cannot safely patch open generics | | Explicit interface implementation | Dotted metadata names cannot be expressed as shim identifiers | diff --git a/Assets/Tests/Editor/HotReload/HotReloadInternalMemberCaller.cs b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberCaller.cs new file mode 100644 index 0000000000..56b2b9e326 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberCaller.cs @@ -0,0 +1,29 @@ +using System.Runtime.CompilerServices; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// A plain type that calls an internal member of another plain type. The visibility repro tests + /// pass it on its own, next to an edit of a partial type, or as a sibling a run brought back. + /// + public class HotReloadInternalMemberCaller + { + [MethodImpl(MethodImplOptions.NoInlining)] + public int CallsInternal() + { + return HotReloadInternalMemberHost.InternalStaticValue(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int PlainValue() + { + return 1; + } + + public int CallerProperty + { + [MethodImpl(MethodImplOptions.NoInlining)] + get { return 30; } + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadInternalMemberCaller.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberCaller.cs.meta new file mode 100644 index 0000000000..795aa51f20 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberCaller.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: fb228d70ed39d446e87083c77cbfb0a2 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadInternalMemberHost.cs b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberHost.cs new file mode 100644 index 0000000000..2ae28ebefb --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberHost.cs @@ -0,0 +1,92 @@ +using System.Runtime.CompilerServices; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// A plain type whose non-public members the visibility repro tests call from edited bodies. + /// No test passes this file to a run, so the worker can only see the type as compiled. + /// + public class HotReloadInternalMemberHost + { + internal int InternalField = 3; + + internal int InternalProperty + { + get { return 4; } + } + + internal int InternalSettableProperty { get; set; } = 20; + + [MethodImpl(MethodImplOptions.NoInlining)] + internal static int InternalStaticValue() + { + return 1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + internal int InternalInstanceValue() + { + return 2; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + protected int ProtectedValue() + { + return 5; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + protected internal int ProtectedInternalValue() + { + return 6; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private protected int PrivateProtectedValue() + { + return 7; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int PrivateStaticValue() + { + return 13; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + internal static HotReloadInternalMemberHost InternalSelf() + { + return new HotReloadInternalMemberHost(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + internal static HotReloadInternalMemberHost[] InternalHosts() + { + return new HotReloadInternalMemberHost[] { new HotReloadInternalMemberHost() }; + } + + /// + /// A type nested in the host, so a test can name an internal member through the nested type. + /// + public class Nested + { + [MethodImpl(MethodImplOptions.NoInlining)] + internal static int NestedInternalValue() + { + return 11; + } + } + } + + /// + /// An internal type the visibility repro tests name from edited bodies. + /// + internal static class HotReloadInternalOnlyType + { + [MethodImpl(MethodImplOptions.NoInlining)] + public static int Value() + { + return 8; + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadInternalMemberHost.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberHost.cs.meta new file mode 100644 index 0000000000..d1617955de --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadInternalMemberHost.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 5d10bb384a8c545c5b44131477f562be +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialDerivedFixture.cs b/Assets/Tests/Editor/HotReload/HotReloadPartialDerivedFixture.cs new file mode 100644 index 0000000000..b5b1ce1fcb --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialDerivedFixture.cs @@ -0,0 +1,61 @@ +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// A partial type deriving from , so its edited bodies can + /// reach the protected members of a compiled base type. + /// + public partial class HotReloadPartialDerivedFixture : HotReloadInternalMemberHost + { + private int _seed = 1000; + + [MethodImpl(MethodImplOptions.NoInlining)] + public int DerivedValue() + { + return 9; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int ClosureValue() + { + Func read = () => 30; + return read(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int ClosureSeedValue() + { + Func read = () => _seed; + return read(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public IEnumerable IteratorValues() + { + yield return _seed; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int ClosureSeedPlusValue() + { + Func read = () => this._seed; + return read() + 7; + } + + public int DerivedProperty + { + [MethodImpl(MethodImplOptions.NoInlining)] + get { return 40; } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public async System.Threading.Tasks.Task AsyncValue() + { + await System.Threading.Tasks.Task.CompletedTask; + return 50; + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialDerivedFixture.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadPartialDerivedFixture.cs.meta new file mode 100644 index 0000000000..07b4403f42 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialDerivedFixture.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 4173d2e8c5b8c4f6592fccd1ad5a5eb8 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialInternalGuardedPeer.Extra.cs b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalGuardedPeer.Extra.cs new file mode 100644 index 0000000000..4e2bf02d05 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalGuardedPeer.Extra.cs @@ -0,0 +1,19 @@ +#if UNITY_EDITOR +using System.Runtime.CompilerServices; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// The other part of . The whole file sits in a + /// conditional-compilation block, the way an editor-only or debug-only part usually does. + /// + public partial class HotReloadPartialInternalGuardedPeer + { + [MethodImpl(MethodImplOptions.NoInlining)] + internal int GuardedPeerInternalValue() + { + return 12; + } + } +} +#endif diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialInternalGuardedPeer.Extra.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalGuardedPeer.Extra.cs.meta new file mode 100644 index 0000000000..45ed9d9773 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalGuardedPeer.Extra.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: c534c4bfd695c4efa9de7002d82bb910 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.Extra.cs b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.Extra.cs new file mode 100644 index 0000000000..20011441fe --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.Extra.cs @@ -0,0 +1,17 @@ +using System.Runtime.CompilerServices; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// The other part of , holding the internal method the + /// visibility repro tests call. + /// + public partial class HotReloadPartialInternalPeer + { + [MethodImpl(MethodImplOptions.NoInlining)] + internal int PeerInternalValue() + { + return 11; + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.Extra.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.Extra.cs.meta new file mode 100644 index 0000000000..989f477d61 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.Extra.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b9bd97bfefce64410a8e7eb97a622374 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.cs b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.cs new file mode 100644 index 0000000000..25953d41dc --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.cs @@ -0,0 +1,30 @@ +using System.Runtime.CompilerServices; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// The main part of a partial type whose internal method lives in another file. No test passes + /// this file, so the worker sees the type as compiled. + /// + public partial class HotReloadPartialInternalPeer + { + [MethodImpl(MethodImplOptions.NoInlining)] + public int PeerMain() + { + return 1; + } + } + + /// + /// The main part of a partial type whose internal method lives in a file wrapped in a + /// conditional-compilation block. + /// + public partial class HotReloadPartialInternalGuardedPeer + { + [MethodImpl(MethodImplOptions.NoInlining)] + public int GuardedPeerMain() + { + return 2; + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.cs.meta new file mode 100644 index 0000000000..14240d0979 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialInternalPeer.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: a28b8e4d2be89448dbe32de6cef97290 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.Other.cs b/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.Other.cs index 7dd4dbce58..521d119924 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.Other.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.Other.cs @@ -30,6 +30,12 @@ public int OtherPartOwnMethod() return PartialTuning - 1; } + [MethodImpl(MethodImplOptions.NoInlining)] + internal int OtherPartInternalValue() + { + return 12; + } + /// /// A type only this part declares, named in a method signature of the edited part. /// diff --git a/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.cs b/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.cs index 8845fdd6f3..549f05f63e 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadPartialTypeFixture.cs @@ -1,3 +1,4 @@ +using System.Linq; using System.Runtime.CompilerServices; namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload diff --git a/Assets/Tests/Editor/HotReload/HotReloadPlainDerivedFixture.cs b/Assets/Tests/Editor/HotReload/HotReloadPlainDerivedFixture.cs new file mode 100644 index 0000000000..2956f22c0a --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPlainDerivedFixture.cs @@ -0,0 +1,54 @@ +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// A plain type deriving from : the control for the + /// partial type that derives from it. + /// + public class HotReloadPlainDerivedFixture : HotReloadInternalMemberHost + { + private int _seed = 1000; + + [MethodImpl(MethodImplOptions.NoInlining)] + public int DerivedValue() + { + return 10; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int ClosureValue() + { + Func read = () => 30; + return read(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int ClosureSeedValue() + { + Func read = () => _seed; + return read(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public IEnumerable IteratorValues() + { + yield return _seed; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public int ClosureSeedPlusValue() + { + Func read = () => this._seed; + return read() + 7; + } + + public int DerivedProperty + { + [MethodImpl(MethodImplOptions.NoInlining)] + get { return 40; } + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadPlainDerivedFixture.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadPlainDerivedFixture.cs.meta new file mode 100644 index 0000000000..522553368f --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadPlainDerivedFixture.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 52992140133b44b419f2e5f5e401d603 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadUnpassedInternalMemberE2ETests.cs b/Assets/Tests/Editor/HotReload/HotReloadUnpassedInternalMemberE2ETests.cs new file mode 100644 index 0000000000..41cbbd3af6 --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadUnpassedInternalMemberE2ETests.cs @@ -0,0 +1,542 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +using NUnit.Framework; + +using UnityEngine; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload +{ + /// + /// End-to-end EditMode coverage for edited bodies that use internal members of a compiled type + /// the reload was not given: what each row reports, and what the method does when called. + /// + /// + /// Why two groups of edits: a straight-line body runs as IL copied into the patched method, + /// while a lambda or iterator body runs in code the shim assembly compiles on its own, and a + /// name inherited by simple name has to be qualified before the shim can compile it at all. + /// + public class HotReloadUnpassedInternalMemberE2ETests + { + private const string PlainDerivedFileName = "HotReloadPlainDerivedFixture.cs"; + private const string PartialDerivedFileName = "HotReloadPartialDerivedFixture.cs"; + private const string CallerFileName = "HotReloadInternalMemberCaller.cs"; + private const string PlainDerivedValueBody = "return 10;"; + private const string PartialDerivedValueBody = "return 9;"; + private const string DerivedValueAnchor = + " [MethodImpl(MethodImplOptions.NoInlining)]\n public int DerivedValue()"; + private const string DerivedValue = "DerivedValue"; + private const string ClosureValue = "ClosureValue"; + private const string ClosureSeedValue = "ClosureSeedValue"; + private const string IteratorValues = "IteratorValues"; + private const string ClosureSeedPlusValue = "ClosureSeedPlusValue"; + private const string DerivedPropertyGetter = "get_DerivedProperty"; + + // Public only because a test case argument has to be as visible as the test method. + public enum FixtureKind + { + Plain, + Partial + } + + private HotReloadDomainTestScope _scope; + + [SetUp] + public void SetUp() + { + _scope = new HotReloadDomainTestScope(); + HotReloadAutoRefreshHold.SyncToActiveChanges(); + } + + [TearDown] + public void TearDown() + { + _scope.Dispose(); + HotReloadAutoRefreshHold.SyncToActiveChanges(); + VibeLogger.ClearMemoryLogs(); + } + + // Edits whose body runs as IL copied into the patched method and uses an internal member of + // a type in the edited file's own assembly. + private static IEnumerable EditsOfTheSameAssemblyThatRunInThePatchedMethod() + { + yield return "InternalStaticMethod"; + yield return "InternalInstanceMethod"; + yield return "InternalFieldReadAndWrite"; + yield return "InternalPropertyGet"; + yield return "InternalPropertyGetAndSet"; + yield return "InheritedInternalMethodThroughThis"; + yield return "InternalInstanceMethodThroughConditionalAccess"; + yield return "InternalStaticMethodOfNestedType"; + yield return "InternalFieldInsideNameof"; + yield return "InternalStaticMethodNextToLambdaReadingOwnPrivateField"; + yield return "InternalInstanceMethodOfAnInternalResult"; + } + + // Edits whose body runs as IL copied into the patched method. + private static IEnumerable EditsThatRunInThePatchedMethod() + { + foreach (string editName in EditsOfTheSameAssemblyThatRunInThePatchedMethod()) + { + yield return editName; + } + + yield return "InternalMethodOfPublicTypeOfAnotherAssemblyThroughInternalsVisibleTo"; + } + + // Edits the shim cannot run as written: a simple name it cannot qualify, or a use inside a + // lambda or iterator, which the shim assembly compiles as ordinary code of its own, also when + // the lambda reaches the member through the result of another internal member. + private static IEnumerable EditsThatDoNotRunInThePatchedMethod() + { + yield return "InheritedInternalMethodBySimpleName"; + yield return "InternalStaticMethodInLambda"; + yield return "InternalInstanceMethodInLambda"; + yield return "InternalStaticMethodInLambdaReadingOwnPrivateField"; + yield return "InternalStaticMethodInIteratorReadingOwnPrivateField"; + yield return "InternalInstanceMethodInIteratorReadingOwnPrivateField"; + yield return "InternalStaticMethodInIterator"; + yield return "InternalInstanceMethodInLambdaOverAnInternalResult"; + yield return "InternalFieldInLambdaParameterFromAnInternalResult"; + } + + /// + /// What: an edited body of a plain type that uses an internal member of a compiled type the + /// reload was not given is patched, and the method returns the edited value. + /// + [TestCaseSource(nameof(EditsThatRunInThePatchedMethod))] + public async Task Run_PlainTypeBodyUsingInternalMemberOfUnpassedType_PatchesBehavior(string editName) + { + BodyEdit edit = FindEdit(editName); + HotReloadOrchestratorResult result = await RunEditAsync(FixtureKind.Plain, edit); + + AssertPatchedAsEdited(result, FixtureKind.Plain, edit); + } + + /// + /// What: the edits that use an internal member of a type in the edited file's own assembly + /// are patched on a partial type too, and the method returns the edited value, as on a plain + /// type. + /// + [TestCaseSource(nameof(EditsOfTheSameAssemblyThatRunInThePatchedMethod))] + public async Task Run_PartialTypeBodyUsingInternalMemberOfUnpassedType_PatchesBehavior(string editName) + { + BodyEdit edit = FindEdit(editName); + HotReloadOrchestratorResult result = await RunEditAsync(FixtureKind.Partial, edit); + + AssertPatchedAsEdited(result, FixtureKind.Partial, edit); + } + + /// + /// What: an edited body of a partial type that uses an internal member of a type in another + /// assembly is either patched and returns the edited value, or skipped and keeps the + /// compiled behavior. + /// + [Test] + public async Task Run_PartialTypeBodyUsingInternalMemberOfATypeOfAnotherAssembly_IsAppliedAsEditedOrSkipped() + { + BodyEdit edit = FindEdit("InternalMethodOfPublicTypeOfAnotherAssemblyThroughInternalsVisibleTo"); + HotReloadOrchestratorResult result = await RunEditAsync(FixtureKind.Partial, edit); + + AssertAppliedAsEditedOrSkipped(result, FixtureKind.Partial, edit); + } + + /// + /// What: edits on a partial type that the shim cannot run as written are either patched and + /// return the edited value, or skipped and keep the compiled behavior. They are never + /// reported as patched and then fail when called, and never fail the file. + /// + [TestCaseSource(nameof(EditsThatDoNotRunInThePatchedMethod))] + public async Task Run_PartialTypeBodyUsingInternalMemberOfUnpassedType_IsAppliedAsEditedOrSkipped(string editName) + { + BodyEdit edit = FindEdit(editName); + HotReloadOrchestratorResult result = await RunEditAsync(FixtureKind.Partial, edit); + + AssertAppliedAsEditedOrSkipped(result, FixtureKind.Partial, edit); + } + + /// + /// What: an edited getter that uses an internal member of a compiled type the reload was not + /// given is patched on a plain type and on a partial type, and the property returns the edited + /// value. + /// + [TestCase(FixtureKind.Plain)] + [TestCase(FixtureKind.Partial)] + public async Task Run_GetterUsingInternalMemberOfUnpassedType_PatchesBehavior(FixtureKind fixture) + { + BodyEdit edit = new BodyEdit( + DerivedPropertyGetter, + "return 40;", + "return HotReloadInternalMemberHost.InternalStaticValue() + 100;", + 101); + HotReloadOrchestratorResult result = await RunEditAsync(fixture, edit); + + AssertPatchedAsEdited(result, fixture, edit); + } + + /// + /// What: a file whose applied body uses an internal member of a type the reload was not given + /// is brought back by a later reload of another file, and its method still returns the value + /// that body computes, whether the row re-applies it or skips it. + /// + [TestCase("CallsInternal", FixtureKind.Plain)] + [TestCase("CallsInternal", FixtureKind.Partial)] + [TestCase("PlainValue", FixtureKind.Plain)] + [TestCase("PlainValue", FixtureKind.Partial)] + public async Task Run_SiblingBroughtBackUsingInternalMemberOfUnpassedType_KeepsTheAppliedBodyRunning( + string methodName, + FixtureKind passedFixture) + { + bool callsInternal = methodName == "CallsInternal"; + int edited = callsInternal ? 201 : 202; + string callerPath = FixturePath(CallerFileName); + Dictionary overrides = new Dictionary + { + [callerPath] = WriteEdited( + callerPath, + "UnpassedInternalSiblingCaller.cs", + callsInternal ? "return HotReloadInternalMemberHost.InternalStaticValue();" : "return 1;", + callsInternal + ? "return HotReloadInternalMemberHost.InternalStaticValue() + 200;" + : "return new HotReloadInternalMemberHost().InternalInstanceValue() + 200;") + }; + HotReloadOrchestratorResult first = await RunAsync(new[] { callerPath }, overrides); + Assert.That(FindRow(first, "HotReloadInternalMemberCaller", methodName).Kind, Is.EqualTo(HotReloadMethodOutcomeKind.Patched), "Precondition: the caller must be patched.\n" + FormatOutcomes(first)); + Assert.That(CallCaller(methodName), Is.EqualTo(edited), "Precondition: the caller must run the edited body.\n" + FormatOutcomes(first)); + + string passedPath = FixturePath(FixtureFileName(passedFixture)); + overrides[passedPath] = WriteEdited( + passedPath, + "UnpassedInternalSiblingPassed.cs", + ValueBody(passedFixture), + "return 50;"); + HotReloadOrchestratorResult second = await RunAsync(new[] { passedPath }, overrides); + + Assert.That(second.ReappliedSiblingPaths, Has.Some.EndsWith(CallerFileName), FormatOutcomes(second)); + Assert.That( + FindRow(second, "HotReloadInternalMemberCaller", methodName).Kind, + Is.EqualTo(HotReloadMethodOutcomeKind.Patched).Or.EqualTo(HotReloadMethodOutcomeKind.Skipped), + FormatOutcomes(second)); + Assert.That(CallCaller(methodName), Is.EqualTo(edited), FormatOutcomes(second)); + } + + /// + /// What: an added method whose body calls an internal method of a type the reload was not + /// given is either added and runs the edited body, or skipped together with its caller so + /// the compiled behavior stays. It is never added and then fails when called. + /// + [TestCase("HotReloadInternalMemberHost.InternalStaticValue() + 300", 301)] + [TestCase("new HotReloadInternalMemberHost().InternalInstanceValue() + 300", 302)] + public async Task Run_AddedMethodUsingInternalMemberOfUnpassedType_IsAddedAsEditedOrSkipped( + string expression, + int edited) + { + string path = FixturePath(PlainDerivedFileName); + string source = ReplaceOnce(File.ReadAllText(path), PlainDerivedValueBody, "return AddedInternalCall();"); + source = ReplaceOnce( + source, + DerivedValueAnchor, + " private int AddedInternalCall()\n {\n return " + expression + ";\n }\n\n" + + DerivedValueAnchor); + HotReloadOrchestratorResult result = await RunAsync( + new[] { path }, + new Dictionary + { + [path] = HotReloadTestSourceWriter.WriteEditedSource("UnpassedInternalAddedMethod.cs", source) + }); + + HotReloadMethodOutcomeKind addedKind = FindRow(result, "HotReloadPlainDerivedFixture", "AddedInternalCall").Kind; + Assert.That( + addedKind, + Is.EqualTo(HotReloadMethodOutcomeKind.Added).Or.EqualTo(HotReloadMethodOutcomeKind.Skipped), + FormatOutcomes(result)); + int expected = addedKind == HotReloadMethodOutcomeKind.Added ? edited : 10; + Assert.That(new HotReloadPlainDerivedFixture().DerivedValue(), Is.EqualTo(expected), FormatOutcomes(result)); + } + + private static BodyEdit FindEdit(string editName) + { + switch (editName) + { + case "InternalStaticMethod": + return BodyEdit.OfDerivedValue("return HotReloadInternalMemberHost.InternalStaticValue() + 100;", 101); + case "InternalInstanceMethod": + return BodyEdit.OfDerivedValue("return new HotReloadInternalMemberHost().InternalInstanceValue() + 100;", 102); + case "InternalFieldReadAndWrite": + return BodyEdit.OfDerivedValue( + "HotReloadInternalMemberHost host = new HotReloadInternalMemberHost();\n" + + " host.InternalField = host.InternalField + 40;\n" + + " return host.InternalField + 100;", + 143); + case "InternalPropertyGet": + return BodyEdit.OfDerivedValue("return new HotReloadInternalMemberHost().InternalProperty + 100;", 104); + case "InternalPropertyGetAndSet": + return BodyEdit.OfDerivedValue( + "HotReloadInternalMemberHost host = new HotReloadInternalMemberHost();\n" + + " host.InternalSettableProperty = host.InternalSettableProperty + 50;\n" + + " return host.InternalSettableProperty + 100;", + 170); + case "InheritedInternalMethodThroughThis": + return BodyEdit.OfDerivedValue("return this.InternalInstanceValue() + 100;", 102); + case "InternalInstanceMethodThroughConditionalAccess": + return BodyEdit.OfDerivedValue( + "HotReloadInternalMemberHost host = new HotReloadInternalMemberHost();\n" + + " return (host?.InternalInstanceValue() ?? 0) + 100;", + 102); + case "InternalStaticMethodOfNestedType": + return BodyEdit.OfDerivedValue("return HotReloadInternalMemberHost.Nested.NestedInternalValue() + 100;", 111); + case "InternalFieldInsideNameof": + return BodyEdit.OfDerivedValue("return nameof(HotReloadInternalMemberHost.InternalField).Length + 100;", 113); + case "InternalStaticMethodNextToLambdaReadingOwnPrivateField": + return new BodyEdit(ClosureSeedPlusValue, "read() + 7", "read() + HotReloadInternalMemberHost.InternalStaticValue()", 1001); + case "InternalInstanceMethodOfAnInternalResult": + return BodyEdit.OfDerivedValue("return HotReloadInternalMemberHost.InternalSelf().InternalInstanceValue() + 120;", 122); + case "InternalMethodOfPublicTypeOfAnotherAssemblyThroughInternalsVisibleTo": + return BodyEdit.OfDerivedValue( + "return global::io.github.hatayama.UnityCliLoop.FirstPartyTools.PausePointResponse" + + ".NormalizeNotCapturableVariables(new string[] { \"a\", \"b\" }).Count + 100;", + 102); + case "InheritedInternalMethodBySimpleName": + return BodyEdit.OfDerivedValue("return InternalInstanceValue() + 100;", 102); + case "InternalStaticMethodInLambda": + return new BodyEdit(ClosureValue, "() => 30", "() => HotReloadInternalMemberHost.InternalStaticValue() + 100", 101); + case "InternalInstanceMethodInLambda": + return new BodyEdit(ClosureValue, "() => 30", "() => new HotReloadInternalMemberHost().InternalInstanceValue() + 100", 102); + case "InternalStaticMethodInLambdaReadingOwnPrivateField": + return new BodyEdit(ClosureSeedValue, "() => _seed", "() => _seed + HotReloadInternalMemberHost.InternalStaticValue()", 1001); + case "InternalStaticMethodInIteratorReadingOwnPrivateField": + return new BodyEdit(IteratorValues, "yield return _seed;", "yield return _seed + HotReloadInternalMemberHost.InternalStaticValue();", 1001); + case "InternalInstanceMethodInIteratorReadingOwnPrivateField": + return new BodyEdit(IteratorValues, "yield return _seed;", "yield return _seed + new HotReloadInternalMemberHost().InternalInstanceValue();", 1002); + case "InternalStaticMethodInIterator": + return new BodyEdit(IteratorValues, "yield return _seed;", "yield return HotReloadInternalMemberHost.InternalStaticValue() + 100;", 101); + case "InternalInstanceMethodInLambdaOverAnInternalResult": + return BodyEdit.OfDerivedValue( + "var host = HotReloadInternalMemberHost.InternalSelf();\n" + + " System.Func read = () => host.InternalInstanceValue() + 100;\n" + + " return read();", + 102); + case "InternalFieldInLambdaParameterFromAnInternalResult": + return BodyEdit.OfDerivedValue( + "return System.Array.Exists(HotReloadInternalMemberHost.InternalHosts(), host => host.InternalField > 0) ? 100 : 0;", + 100); + default: + throw new ArgumentException("Unknown edit: " + editName); + } + } + + private static async Task RunEditAsync(FixtureKind fixture, BodyEdit edit) + { + string path = FixturePath(FixtureFileName(fixture)); + string fragment = edit.Fragment ?? ValueBody(fixture); + return await RunAsync( + new[] { path }, + new Dictionary + { + [path] = WriteEdited(path, "UnpassedInternal" + fixture + edit.MethodName + ".cs", fragment, edit.Replacement) + }); + } + + private static void AssertPatchedAsEdited(HotReloadOrchestratorResult result, FixtureKind fixture, BodyEdit edit) + { + Assert.That(FindRow(result, FixtureTypeName(fixture), edit.MethodName).Kind, Is.EqualTo(HotReloadMethodOutcomeKind.Patched), FormatOutcomes(result)); + Assert.That(CallFixture(fixture, edit.MethodName), Is.EqualTo(edit.EditedValue), FormatOutcomes(result)); + } + + // Why the value follows the row: a skipped row keeps the compiled body, and a patched row + // must run the edited one. A call that throws fails the test, which is the point. + private static void AssertAppliedAsEditedOrSkipped(HotReloadOrchestratorResult result, FixtureKind fixture, BodyEdit edit) + { + HotReloadMethodOutcomeKind kind = FindRow(result, FixtureTypeName(fixture), edit.MethodName).Kind; + Assert.That( + kind, + Is.EqualTo(HotReloadMethodOutcomeKind.Patched).Or.EqualTo(HotReloadMethodOutcomeKind.Skipped), + FormatOutcomes(result)); + int expected = kind == HotReloadMethodOutcomeKind.Patched ? edit.EditedValue : CompiledValue(fixture, edit.MethodName); + Assert.That(CallFixture(fixture, edit.MethodName), Is.EqualTo(expected), FormatOutcomes(result)); + } + + private static int CompiledValue(FixtureKind fixture, string methodName) + { + switch (methodName) + { + case DerivedValue: + return fixture == FixtureKind.Plain ? 10 : 9; + case ClosureValue: + return 30; + case ClosureSeedPlusValue: + return 1007; + default: + return 1000; + } + } + + private static int CallFixture(FixtureKind fixture, string methodName) + { + if (fixture == FixtureKind.Plain) + { + HotReloadPlainDerivedFixture plain = new HotReloadPlainDerivedFixture(); + switch (methodName) + { + case DerivedValue: + return plain.DerivedValue(); + case ClosureValue: + return plain.ClosureValue(); + case ClosureSeedValue: + return plain.ClosureSeedValue(); + case ClosureSeedPlusValue: + return plain.ClosureSeedPlusValue(); + case IteratorValues: + return First(plain.IteratorValues()); + case DerivedPropertyGetter: + return plain.DerivedProperty; + } + } + else + { + HotReloadPartialDerivedFixture partial = new HotReloadPartialDerivedFixture(); + switch (methodName) + { + case DerivedValue: + return partial.DerivedValue(); + case ClosureValue: + return partial.ClosureValue(); + case ClosureSeedValue: + return partial.ClosureSeedValue(); + case ClosureSeedPlusValue: + return partial.ClosureSeedPlusValue(); + case IteratorValues: + return First(partial.IteratorValues()); + case DerivedPropertyGetter: + return partial.DerivedProperty; + } + } + + throw new ArgumentException("Unknown fixture method: " + methodName); + } + + private static int CallCaller(string methodName) + { + HotReloadInternalMemberCaller caller = new HotReloadInternalMemberCaller(); + return methodName == "CallsInternal" ? caller.CallsInternal() : caller.PlainValue(); + } + + private static int First(IEnumerable values) + { + foreach (int value in values) + { + return value; + } + + throw new InvalidOperationException("The iterator yielded nothing."); + } + + private static string FixtureFileName(FixtureKind fixture) + { + return fixture == FixtureKind.Plain ? PlainDerivedFileName : PartialDerivedFileName; + } + + private static string FixtureTypeName(FixtureKind fixture) + { + return fixture == FixtureKind.Plain ? nameof(HotReloadPlainDerivedFixture) : nameof(HotReloadPartialDerivedFixture); + } + + private static string ValueBody(FixtureKind fixture) + { + return fixture == FixtureKind.Plain ? PlainDerivedValueBody : PartialDerivedValueBody; + } + + // Why the type and the parenthesis: a bare method name would also match a longer name that + // contains it, or the same method on the other fixture type. + private static HotReloadMethodOutcome FindRow(HotReloadOrchestratorResult result, string typeName, string methodName) + { + string labelPart = "." + typeName + "." + methodName + "("; + foreach (HotReloadMethodOutcome outcome in result.Methods) + { + if (outcome.Method != null && outcome.Method.Contains(labelPart)) + { + return outcome; + } + } + + Assert.Fail("No row for " + typeName + "." + methodName + ".\n" + FormatOutcomes(result)); + return null; + } + + private static Task RunAsync(string[] files, Dictionary overrides) + { + return HotReloadCompositionRoot.Services.Orchestrator.RunAsync( + files, + contentPathOverride: null, + CancellationToken.None, + new Dictionary(overrides)); + } + + private static string WriteEdited(string sourcePath, string editedFileName, string fragment, string replacement) + { + return HotReloadTestSourceWriter.WriteEditedSource( + editedFileName, + ReplaceOnce(File.ReadAllText(sourcePath), fragment, replacement)); + } + + // Why the uniqueness check: a fragment that also matched another member would edit a method + // the test does not call, and the assert would pass or fail for the wrong reason. + private static string ReplaceOnce(string source, string fragment, string replacement) + { + int first = source.IndexOf(fragment, StringComparison.Ordinal); + Assert.That(first, Is.GreaterThanOrEqualTo(0), "Fragment missing from the fixture: " + fragment); + Assert.That( + source.IndexOf(fragment, first + fragment.Length, StringComparison.Ordinal), + Is.EqualTo(-1), + "Fragment occurs more than once in the fixture: " + fragment); + return source.Replace(fragment, replacement, StringComparison.Ordinal); + } + + private static string FixturePath(string fileName) + { + string path = Path.GetFullPath(Path.Combine(Application.dataPath, "Tests", "Editor", "HotReload", fileName)); + Assert.That(File.Exists(path), Is.True, "Fixture missing: " + path); + return path; + } + + private static string FormatOutcomes(HotReloadOrchestratorResult result) + { + List lines = new List(); + foreach (HotReloadMethodOutcome outcome in result.Methods) + { + lines.Add(outcome.Kind + " " + outcome.Method + " :: " + outcome.Reason); + } + + return string.Join("\n", lines) + "\nWarnings:\n" + string.Join("\n", result.Warnings); + } + + // One edit of a fixture method: the fragment it replaces (null for the fixture's own + // DerivedValue body, which differs between the two fixtures) and the value the edit returns. + private sealed class BodyEdit + { + internal BodyEdit(string methodName, string fragment, string replacement, int editedValue) + { + MethodName = methodName; + Fragment = fragment; + Replacement = replacement; + EditedValue = editedValue; + } + + internal string MethodName { get; } + internal string Fragment { get; } + internal string Replacement { get; } + internal int EditedValue { get; } + + internal static BodyEdit OfDerivedValue(string replacement, int editedValue) + { + return new BodyEdit(DerivedValue, null, replacement, editedValue); + } + } + } +} diff --git a/Assets/Tests/Editor/HotReload/HotReloadUnpassedInternalMemberE2ETests.cs.meta b/Assets/Tests/Editor/HotReload/HotReloadUnpassedInternalMemberE2ETests.cs.meta new file mode 100644 index 0000000000..ffe4cbfe7a --- /dev/null +++ b/Assets/Tests/Editor/HotReload/HotReloadUnpassedInternalMemberE2ETests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: bcb10cd9148204b379eb58e797e6a7fb +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs b/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs index 86b999354d..2171a81e93 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadWorkerReasonTextTests.cs @@ -228,6 +228,17 @@ private static IEnumerable RenderCases() + "type's source files known to hot reload declares that name: a part generated at compile " + "time is not visible to it, and a file added since the last compile must be passed with " + "--files. Otherwise run 'uloop compile'."); + yield return Case( + HotReloadWorkerReasonCode.MethodTransformUnpassedInternalMemberOutOfReach, + new[] { "CS0117: 'Host' does not contain a definition for 'Value'", "'Host'" }, + "CS0117: 'Host' does not contain a definition for 'Value'. That member is internal to 'Host', " + + "whose source this reload was not given. Hot reload patches a use of such a member only where " + + "it is a field, a property or a method call written with its receiver ('this.Name', " + + "'Type.Name', 'value.Name') in the method's own statements: not a bare name, a method passed " + + "as a delegate, or a use inside a lambda, local function, query, iterator or async method, or " + + "in a body patched through a delegating shim. A lambda, local function or query that works " + + "with a value hot reload could not resolve, such as the member's result, keeps the whole body " + + "out as well. Qualify a bare name with 'this.' or the type name, or run 'uloop compile'."); yield return Case( HotReloadWorkerReasonCode.MethodTransformStructHost, NoArgs, diff --git a/Assets/Tests/Editor/HotReload/TransformWorkerPartialTypeTests.cs b/Assets/Tests/Editor/HotReload/TransformWorkerPartialTypeTests.cs index 0b32ff3d25..a36ab6f650 100644 --- a/Assets/Tests/Editor/HotReload/TransformWorkerPartialTypeTests.cs +++ b/Assets/Tests/Editor/HotReload/TransformWorkerPartialTypeTests.cs @@ -67,6 +67,20 @@ public class TransformWorkerPartialTypeTests private const string OtherPartPropertyGetter = "get { return OtherPartProperty; }"; private const string OtherPartPropertyGetterEdited = "get { return OtherPartProperty + 100; }"; + // Fixtures of the internal-visibility repro tests. None of them declares the types whose + // non-public members the edited bodies use, so the worker sees those types as compiled. + private const string FixtureDirectoryProjectRelativePath = "Assets/Tests/Editor/HotReload/"; + private const string CallerFileName = "HotReloadInternalMemberCaller.cs"; + private const string PlainDerivedFileName = "HotReloadPlainDerivedFixture.cs"; + private const string PartialDerivedFileName = "HotReloadPartialDerivedFixture.cs"; + private const string CallerInternalCallBody = "return HotReloadInternalMemberHost.InternalStaticValue();"; + private const string CallerInternalCallBodyEdited = "return HotReloadInternalMemberHost.InternalStaticValue() + 100;"; + private const string CallerPlainValueBody = "return 1;"; + private const string PlainDerivedValueBody = "return 10;"; + private const string PartialDerivedValueBody = "return 9;"; + private const string InternalMemberOfATypeOfAnotherAssembly = + "global::io.github.hatayama.UnityCliLoop.FirstPartyTools.PausePointCapturedVariable.FromSnapshot(null).Name.Length"; + /// /// What: a body that reads a private field declared in another part of the type is emitted. /// @@ -503,6 +517,524 @@ public async Task Skip_PartialTypeBodyEdit_WhenAnotherRunFileCannotBeRead_NamesT AssertFileHasParseErrors(result, UnreadableProjectRelativePath); } + /// + /// What: a body of a partial type that calls an internal method of a plain type the run was + /// not given binds, as the same call from a plain type does (the control run of the same test). + /// + [Test] + public async Task Run_PartialTypeBodyCallingInternalMethodOfUnpassedPlainType_Binds() + { + const string call = "HotReloadInternalMemberHost.InternalStaticValue()"; + TransformWorkerClientResult control = await RunWorkerOnSourcesAsync(new[] + { + BuildCallerPlainValueEdit("ReproAControl.cs", call) + }); + TransformWorkerClientResult repro = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("ReproAPartial.cs", call) + }); + + AssertControlAndReproEmitted(control, "PlainValue", repro, "OwnOnly"); + } + + /// + /// What: a plain type's body that calls an internal method of an unpassed type binds when an + /// edit of a partial type is in the same run, as it does when the plain file is passed alone + /// (the control run of the same test). + /// + [Test] + public async Task Run_PlainFilePassedNextToPartialTypeEdit_CallingInternalMethodOfUnpassedType_Binds() + { + TransformWorkerClientResult control = await RunWorkerOnSourcesAsync(new[] + { + BuildCallerInternalCallEdit("ReproB1Control.cs") + }); + TransformWorkerClientResult repro = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("ReproB1Partial.cs", "2"), + BuildCallerInternalCallEdit("ReproB1Caller.cs") + }); + + AssertControlAndReproEmitted(control, "CallsInternal", repro, "CallsInternal"); + } + + /// + /// What: a plain file that comes back as a sibling to re-bind its active patches while an edit + /// of a partial type is passed: its body that calls an internal method of an unpassed type + /// binds, as it does when the passed edit is in a plain type (the control run of the same test). + /// + [Test] + public async Task Run_PlainSiblingBroughtBackNextToPartialTypeEdit_CallingInternalMethodOfUnpassedType_Binds() + { + TransformWorkerClientResult control = await RunWorkerOnSourcesAsync(new[] + { + BuildEditedFixtureSource(PlainDerivedFileName, "ReproB2ControlPassed.cs", PlainDerivedValueBody, "return 11;"), + AsReappliedSibling(BuildCallerInternalCallEdit("ReproB2ControlSibling.cs")) + }); + TransformWorkerClientResult repro = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("ReproB2Partial.cs", "2"), + AsReappliedSibling(BuildCallerInternalCallEdit("ReproB2Sibling.cs")) + }); + + AssertControlAndReproEmitted(control, "CallsInternal", repro, "CallsInternal"); + } + + /// + /// What: a body of a partial type that calls an internal method another partial type declares + /// in a separate file the run was not given binds, as the same call from a plain type does + /// (the control run of the same test). + /// + [Test] + public async Task Run_PartialTypeBodyCallingInternalMethodOfAnotherUnpassedPartialType_Binds() + { + const string call = "new HotReloadPartialInternalPeer().PeerInternalValue()"; + TransformWorkerClientResult control = await RunWorkerOnSourcesAsync(new[] + { + BuildCallerPlainValueEdit("ReproC1Control.cs", call) + }); + TransformWorkerClientResult repro = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("ReproC1Partial.cs", call) + }); + + AssertControlAndReproEmitted(control, "PlainValue", repro, "OwnOnly"); + } + + /// + /// What: the same call, with the other partial type's file wrapped whole in a + /// conditional-compilation block whose symbol the assembly defines, binds as the same call + /// from a plain type does (the control run of the same test). + /// + [Test] + public async Task Run_PartialTypeBodyCallingInternalMethodOfAnotherUnpassedPartialTypeInAConditionalFile_Binds() + { + const string call = "new HotReloadPartialInternalGuardedPeer().GuardedPeerInternalValue()"; + TransformWorkerClientResult control = await RunWorkerOnSourcesAsync(new[] + { + BuildCallerPlainValueEdit("ReproC2Control.cs", call) + }); + TransformWorkerClientResult repro = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("ReproC2Partial.cs", call) + }); + + AssertControlAndReproEmitted(control, "PlainValue", repro, "OwnOnly"); + } + + /// + /// What: a body of a partial type that uses a non-public member of a type the run was not + /// given binds, whatever the kind of member, as the same use from a plain type does (the + /// control run of the same test). + /// + [TestCase("InternalInstanceMethod", "new HotReloadInternalMemberHost().InternalInstanceValue()")] + [TestCase("InternalField", "new HotReloadInternalMemberHost().InternalField")] + [TestCase("InternalProperty", "new HotReloadInternalMemberHost().InternalProperty")] + [TestCase("InternalType", "HotReloadInternalOnlyType.Value()")] + [TestCase("InternalMethodOfAnInternalResult", "HotReloadInternalMemberHost.InternalSelf().InternalInstanceValue()")] + [TestCase("ProtectedInternalMethod", "new HotReloadInternalMemberHost().ProtectedInternalValue()")] + [TestCase( + "PublicMemberOfInternalTypeOfAnotherAssemblyThroughInternalsVisibleTo", + "global::io.github.hatayama.UnityCliLoop.FirstPartyTools.HotReloadConstants.TestSourcesRelativeDirectory.Length")] + public async Task Run_PartialTypeBodyUsingNonPublicMemberOfUnpassedType_Binds(string memberKind, string expression) + { + TransformWorkerClientResult control = await RunWorkerOnSourcesAsync(new[] + { + BuildCallerPlainValueEdit("ReproKindControl" + memberKind + ".cs", expression) + }); + TransformWorkerClientResult repro = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("ReproKindPartial" + memberKind + ".cs", expression) + }); + + AssertControlAndReproEmitted(control, "PlainValue", repro, "OwnOnly"); + } + + /// + /// What: a body of a partial type that uses a member a compiled base type grants its derived + /// types binds, as the same use from a plain derived type does (the control run of the same + /// test). + /// + [TestCase("Protected", "ProtectedValue()")] + [TestCase("PrivateProtected", "PrivateProtectedValue()")] + [TestCase("InheritedInternalThroughThis", "this.InternalInstanceValue()")] + public async Task Run_DerivedPartialTypeBodyUsingBaseMemberOfUnpassedType_Binds(string memberKind, string expression) + { + TransformWorkerClientResult control = await RunWorkerOnSourcesAsync(new[] + { + BuildEditedFixtureSource( + PlainDerivedFileName, + "ReproBaseControl" + memberKind + ".cs", + PlainDerivedValueBody, + "return " + expression + ";") + }); + TransformWorkerClientResult repro = await RunWorkerOnSourcesAsync(new[] + { + BuildEditedFixtureSource( + PartialDerivedFileName, + "ReproBasePartial" + memberKind + ".cs", + PartialDerivedValueBody, + "return " + expression + ";") + }); + + AssertControlAndReproEmitted(control, "DerivedValue", repro, "DerivedValue"); + } + + /// + /// What: a body of a partial type that uses an internal member of a type the run was not given + /// where the patched method cannot run it in place (inside a lambda, a local function or a + /// query, in an iterator or async method, as a method passed as a delegate, by a bare name, or + /// next to a lambda, local function or query that works with the member's result) is skipped + /// with a reason that says the member is internal, not that a part is missing. + /// + [TestCase("Lambda", "OwnOnly", null, "System.Func read = () => HotReloadInternalMemberHost.InternalStaticValue(); return read();")] + [TestCase("LocalFunction", "OwnOnly", null, "int Read() { return HotReloadInternalMemberHost.InternalStaticValue(); } return Read();")] + [TestCase("Query", "OwnOnly", null, "return (from value in new[] { 1 } select value + HotReloadInternalMemberHost.InternalStaticValue()).First();")] + [TestCase("LambdaUsingTheResult", "OwnOnly", null, "var host = HotReloadInternalMemberHost.InternalSelf(); System.Func read = () => host.InternalInstanceValue(); return read();")] + [TestCase("LambdaParameterFromTheResult", "OwnOnly", null, "return System.Array.Exists(HotReloadInternalMemberHost.InternalHosts(), host => host.InternalField > 0) ? 1 : 0;")] + [TestCase("LocalFunctionUsingTheResult", "OwnOnly", null, "var host = HotReloadInternalMemberHost.InternalSelf(); int Read() { return host.InternalInstanceValue(); } return Read();")] + [TestCase("QueryOverTheResult", "OwnOnly", null, "var hosts = HotReloadInternalMemberHost.InternalHosts(); return (from host in hosts select host.InternalField).First();")] + [TestCase("MethodPassedAsDelegate", "OwnOnly", null, "System.Func read = HotReloadInternalMemberHost.InternalStaticValue; return read();")] + [TestCase("Iterator", "IteratorValues", "yield return _seed;", "yield return HotReloadInternalMemberHost.InternalStaticValue();")] + [TestCase("Async", "AsyncValue", "return 50;", "return HotReloadInternalMemberHost.InternalStaticValue();")] + [TestCase("BareName", "DerivedValue", PartialDerivedValueBody, "return InternalInstanceValue();")] + public async Task Skip_PartialTypeBodyUsingInternalMemberWhereItCannotBePatchedInPlace_SaysWhy( + string form, + string methodName, + string partialDerivedFragment, + string replacement) + { + string editedFileName = "InternalOutOfReach" + form + ".cs"; + TransformWorkerSourceDto edit = partialDerivedFragment == null + ? BuildFixtureOwnOnlyBodyEdit(editedFileName, replacement) + : BuildEditedFixtureSource(PartialDerivedFileName, editedFileName, partialDerivedFragment, replacement); + + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] { edit }); + + string reason = AssertSkipped(result, methodName); + Assert.That(reason, Does.Contain("is internal to 'HotReloadInternalMemberHost'"), FormatSkipped(result)); + Assert.That(reason, Does.Not.Contain("generated at compile time"), FormatSkipped(result)); + } + + /// + /// What: a getter of a partial type whose lambda reads a private member, so the whole getter + /// runs through a delegating shim, is skipped with a reason that says the internal member it + /// uses outside the lambda is internal. + /// + [Test] + public async Task Skip_PartialTypeGetterWithALambdaReadingAPrivateMember_UsingInternalMemberDirectly_SaysWhy() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] + { + BuildEditedFixtureSource( + PartialDerivedFileName, + "InternalOutOfReachDelegatingGetter.cs", + "return 40;", + "System.Func read = () => this._seed; return read() + HotReloadInternalMemberHost.InternalStaticValue();") + }); + + Assert.That(AssertSkipped(result, "get_DerivedProperty"), Does.Contain("is internal to"), FormatSkipped(result)); + } + + /// + /// What: a getter of a partial type whose lambda works with the result of an internal member + /// of a type the run was not given is skipped with the internal-member reason, as a method + /// with the same body is. + /// + [Test] + public async Task Skip_PartialTypeGetterWithALambdaUsingTheResultOfAnInternalMember_SaysWhy() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] + { + BuildEditedFixtureSource( + PartialDerivedFileName, + "InternalOutOfReachGetterLambdaUsingTheResult.cs", + "return 40;", + "var host = HotReloadInternalMemberHost.InternalSelf(); System.Func read = () => host.InternalInstanceValue(); return read();") + }); + + Assert.That(AssertSkipped(result, "get_DerivedProperty"), Does.Contain("is internal to"), FormatSkipped(result)); + } + + /// + /// What: a body of a partial type that uses a private member of a type the run was not given + /// keeps the missing-name reason. + /// + [Test] + public async Task Skip_PartialTypeBodyUsingPrivateMemberOfUnpassedType_KeepsTheMissingNameReason() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("UnpassedPrivateMember.cs", "HotReloadInternalMemberHost.PrivateStaticValue()") + }); + + Assert.That(AssertSkipped(result, "OwnOnly"), Does.Contain("generated at compile time"), FormatSkipped(result)); + } + + /// + /// What: a body of a partial type that uses an internal member of a type in another assembly + /// keeps the missing-name reason. + /// + [Test] + public async Task Skip_PartialTypeBodyUsingInternalMemberOfATypeOfAnotherAssembly_KeepsTheMissingNameReason() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("AnotherAssemblyInternalMember.cs", InternalMemberOfATypeOfAnotherAssembly) + }); + + Assert.That(AssertSkipped(result, "OwnOnly"), Does.Contain("generated at compile time"), FormatSkipped(result)); + } + + /// + /// What: a body of a partial type that uses an internal member of an unpassed type next to a + /// name nothing declares is skipped with a reason that names the undeclared name. + /// + [Test] + public async Task Skip_PartialTypeBodyUsingInternalMemberNextToAnUnresolvedName_NamesTheUnresolvedName() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit( + "InternalMemberNextToUnresolvedName.cs", + "HotReloadInternalMemberHost.InternalStaticValue() + NoSuchName") + }); + + string reason = AssertSkipped(result, "OwnOnly"); + Assert.That(reason, Does.Contain("NoSuchName"), FormatSkipped(result)); + Assert.That(reason, Does.Contain("generated at compile time"), FormatSkipped(result)); + } + + /// + /// What: when a part of the type is in no source file the run can see, a body that calls that + /// part's internal method through 'this' keeps the missing-name reason, although the compiled + /// type declares the method. + /// + [Test] + public async Task Skip_PartialTypeBodyUsingThisInternalMemberOfAPartNotAmongTheAssemblySources_KeepsTheMissingNameReason() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync( + new[] { BuildFixtureOwnOnlyEdit("PartialThisInternalOfPartNotInSources.cs", "this.OtherPartInternalValue()") }, + assemblySourcePathsOverride: BuildAssemblySourcePathsWithout(OtherPartFileName)); + + Assert.That(AssertSkipped(result, "OwnOnly"), Does.Contain("generated at compile time"), FormatSkipped(result)); + } + + /// + /// What: a body of a partial type that calls an internal method of a type nested in a type the + /// run was not given is emitted. + /// + [Test] + public async Task Run_PartialTypeBodyUsingInternalMemberOfANestedUnpassedType_EmitsEntry() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("NestedTypeInternalMember.cs", "HotReloadInternalMemberHost.Nested.NestedInternalValue()") + }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + AssertEmitted(result, "OwnOnly"); + } + + /// + /// What: an existing getter of a partial type that calls an internal method of a type the run + /// was not given is emitted. + /// + [Test] + public async Task Run_PartialTypeGetterUsingInternalMemberOfUnpassedType_EmitsEntry() + { + TransformWorkerClientResult result = await RunEditedFixtureAsync( + "PartialGetterUsesUnpassedInternal.cs", + OtherPartPropertyGetter, + "get { return HotReloadInternalMemberHost.InternalStaticValue(); }"); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + AssertEmitted(result, "get_ReadsOtherPartProperty"); + } + + /// + /// What: a getter of a file brought back to re-bind its active patches that calls an internal + /// method of a type the run was not given is emitted. + /// + [Test] + public async Task Run_SiblingBroughtBack_GetterUsingInternalMemberOfUnpassedType_EmitsEntry() + { + TransformWorkerClientResult result = await RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit("SiblingGetterPassed.cs", "2"), + AsReappliedSibling(BuildEditedFixtureSource( + CallerFileName, + "SiblingGetterCaller.cs", + "get { return 30; }", + "get { return HotReloadInternalMemberHost.InternalStaticValue(); }")) + }); + + Assert.That(result.Success, Is.True, result.ErrorMessage); + AssertEmitted(result, "get_CallerProperty"); + } + + /// + /// What: a method of a file brought back to re-bind its active patches whose body uses an + /// internal member of an unpassed type inside a lambda keeps the sibling's skip reason. + /// + [Test] + public async Task Skip_SiblingBroughtBack_UsingInternalMemberInsideALambda_KeepsTheSiblingReason() + { + TransformWorkerClientResult result = await RunWithSiblingPlainValueAsync( + "SiblingInternalInLambda", + "System.Func read = () => HotReloadInternalMemberHost.InternalStaticValue(); return read();"); + + Assert.That(AssertSkipped(result, "PlainValue"), Does.Contain("brought back to re-bind"), FormatSkipped(result)); + } + + /// + /// What: a method of a file brought back to re-bind its active patches whose lambda works with + /// the result of an internal member of an unpassed type keeps the sibling's skip reason. + /// + [Test] + public async Task Skip_SiblingBroughtBack_WithALambdaUsingTheResultOfAnInternalMember_KeepsTheSiblingReason() + { + TransformWorkerClientResult result = await RunWithSiblingPlainValueAsync( + "SiblingLambdaUsingTheResult", + "var host = HotReloadInternalMemberHost.InternalSelf(); System.Func read = () => host.InternalInstanceValue(); return read();"); + + Assert.That(AssertSkipped(result, "PlainValue"), Does.Contain("brought back to re-bind"), FormatSkipped(result)); + } + + /// + /// What: a method of a file brought back to re-bind its active patches whose body uses an + /// internal member of an unpassed type next to a name nothing declares keeps the sibling's skip + /// reason. + /// + [Test] + public async Task Skip_SiblingBroughtBack_UsingInternalMemberNextToAnUnresolvedName_KeepsTheSiblingReason() + { + TransformWorkerClientResult result = await RunWithSiblingPlainValueAsync( + "SiblingInternalNextToUnresolvedName", + "return HotReloadInternalMemberHost.InternalStaticValue() + NoSuchName;"); + + Assert.That(AssertSkipped(result, "PlainValue"), Does.Contain("brought back to re-bind"), FormatSkipped(result)); + } + + // A fixture file next to these tests, edited once and described as a passed run source whose + // snapshot is the file on disk. + private static TransformWorkerSourceDto BuildEditedFixtureSource( + string fileName, + string editedFileName, + string fragment, + string replacement) + { + string onDisk = File.ReadAllText(ResolveFixturePath(fileName)); + return new TransformWorkerSourceDto + { + sourcePath = WriteEdited(editedFileName, ReplaceOnce(onDisk, fragment, replacement)), + projectRelativePath = FixtureDirectoryProjectRelativePath + fileName, + snapshotSource = onDisk + }; + } + + // The edited main part of the partial fixture, with OwnOnly returning the given expression. + private static TransformWorkerSourceDto BuildFixtureOwnOnlyEdit(string editedFileName, string returnedExpression) + { + string editedOwnOnly = + " public int OwnOnly()\n {\n return " + returnedExpression + ";\n }"; + return BuildEditedFixtureSource(FixtureFileName, editedFileName, OwnOnlyDeclaration, editedOwnOnly); + } + + // The edited main part of the partial fixture, with OwnOnly's body replaced by the given line, + // which may hold several statements. + private static TransformWorkerSourceDto BuildFixtureOwnOnlyBodyEdit(string editedFileName, string bodyLine) + { + string editedOwnOnly = + " public int OwnOnly()\n {\n " + bodyLine + "\n }"; + return BuildEditedFixtureSource(FixtureFileName, editedFileName, OwnOnlyDeclaration, editedOwnOnly); + } + + // An edit of the partial fixture passed next to the plain caller, which comes back as a sibling + // whose PlainValue body is the given line. + private static Task RunWithSiblingPlainValueAsync(string label, string plainValueBody) + { + return RunWorkerOnSourcesAsync(new[] + { + BuildFixtureOwnOnlyEdit(label + "Passed.cs", "2"), + AsReappliedSibling(BuildEditedFixtureSource(CallerFileName, label + "Sibling.cs", CallerPlainValueBody, plainValueBody)) + }); + } + + // The plain caller with PlainValue returning the given expression. + private static TransformWorkerSourceDto BuildCallerPlainValueEdit(string editedFileName, string returnedExpression) + { + return BuildEditedFixtureSource( + CallerFileName, + editedFileName, + CallerPlainValueBody, + "return " + returnedExpression + ";"); + } + + // The plain caller with an edit inside the body that calls the internal method. + private static TransformWorkerSourceDto BuildCallerInternalCallEdit(string editedFileName) + { + return BuildEditedFixtureSource( + CallerFileName, + editedFileName, + CallerInternalCallBody, + CallerInternalCallBodyEdited); + } + + private static TransformWorkerSourceDto AsReappliedSibling(TransformWorkerSourceDto source) + { + source.reappliedSibling = true; + return source; + } + + // Why one assertion over both runs: the control and the repro fail for different reasons, and + // the reader needs both runs' rows even when the control already failed. + private static void AssertControlAndReproEmitted( + TransformWorkerClientResult control, + string controlMethodName, + TransformWorkerClientResult repro, + string reproMethodName) + { + List failures = new List(); + CollectMissingEntry("control", control, controlMethodName, failures); + CollectMissingEntry("repro", repro, reproMethodName, failures); + Assert.That(failures, Is.Empty, string.Join("\n\n", failures)); + } + + private static void CollectMissingEntry( + string runLabel, + TransformWorkerClientResult result, + string methodName, + List failures) + { + if (!result.Success) + { + failures.Add(runLabel + " run failed: " + result.ErrorMessage); + return; + } + + if (FindEntry(result, methodName) == null) + { + failures.Add( + runLabel + ": missing entry for " + methodName + ".\n" + + FormatSkipped(result) + "\n" + FormatFileErrors(result)); + } + } + + private static string FormatFileErrors(TransformWorkerClientResult result) + { + List rows = new List(); + foreach (TransformWorkerFileOutputDto file in result.Output.files) + { + foreach (string parseError in file.parseErrors ?? Array.Empty()) + { + rows.Add(file.projectRelativePath + " :: " + parseError); + } + } + + return rows.Count == 0 ? "FileErrors=(none)" : "FileErrors=\n" + string.Join("\n", rows); + } + // The edited main part. OwnOnly reads only its own part, so it binds whether or not the other // part is visible, and only an untrusted other part can keep it from being emitted. private static TransformWorkerSourceDto BuildOwnOnlyEditSource(string editedFileName) @@ -586,6 +1118,17 @@ private static string ReplaceOnce(string source, string fragment, string replace return source.Substring(0, first) + replacement + source.Substring(first + fragment.Length); } + // The rendered reason of the method's skipped row, once the run succeeded without an entry for + // the method. + private static string AssertSkipped(TransformWorkerClientResult result, string methodName) + { + Assert.That(result.Success, Is.True, result.ErrorMessage); + Assert.That(FindEntry(result, methodName), Is.Null, methodName + " must not be applied.\n" + FormatSkipped(result)); + TransformWorkerSkippedDto skipped = FindSkipped(result, methodName); + Assert.That(skipped, Is.Not.Null, "Missing skipped row for " + methodName + ".\n" + FormatSkipped(result)); + return HotReloadWorkerReasonText.Render(skipped.reason); + } + private static TransformWorkerEntryDto AssertEmitted(TransformWorkerClientResult result, string methodName) { TransformWorkerEntryDto entry = FindEntry(result, methodName); diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs index ecfc103db4..df6cec0dd1 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonCode.cs @@ -21,6 +21,7 @@ internal enum HotReloadWorkerReasonCode MethodTransformPartialOtherPartChanged, MethodTransformPartialOtherPartsUnverified, MethodTransformPartialBodyUnbound, + MethodTransformUnpassedInternalMemberOutOfReach, MethodTransformStructHost, MethodTransformGenericMethodOrType, MethodTransformExplicitInterfaceImplementation, diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.MethodTransformTemplates.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.MethodTransformTemplates.cs index bd962c07e4..2d77bcc13d 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.MethodTransformTemplates.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Shared/HotReloadWorkerReasonText.MethodTransformTemplates.cs @@ -55,6 +55,18 @@ private static void AddMethodTransformTemplates( + "a part generated at compile time is not visible to it, and a file added since the " + "last compile must be passed with --files. Otherwise run 'uloop compile'.", 1)); + templates.Add( + HotReloadWorkerReasonCode.MethodTransformUnpassedInternalMemberOutOfReach, + Plain( + "{0}. That member is internal to {1}, whose source this reload was not given. Hot reload patches " + + "a use of such a member only where it is a field, a property or a method call written with its " + + "receiver ('this.Name', 'Type.Name', 'value.Name') in the method's own statements: not a bare " + + "name, a method passed as a delegate, or a use inside a lambda, local function, query, iterator " + + "or async method, or in a body patched through a delegating shim. A lambda, local function or " + + "query that works with a value hot reload could not resolve, such as the member's result, keeps " + + "the whole body out as well. Qualify a bare name with 'this.' or the type name, or run " + + "'uloop compile'.", + 2)); templates.Add( HotReloadWorkerReasonCode.MethodTransformStructHost, Plain( diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md index eda3049cbc..5553989c40 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Skill/references/scope-and-limits.md @@ -404,6 +404,8 @@ source on disk. When a run skips a method it had patched before, `Warnings` name | Method on a `partial` type when another part of the type changed since the last compile and was not passed, or when a file that names the type has syntax errors (passed or not) | Hot reload binds against the compiled type; pass that file with `--files` too, or run `uloop compile`. For a file with syntax errors, fix it and run hot reload again | | Method on a `partial` type when the other parts could not be checked against the last compile (no source snapshot yet, or more than 50 changed files in the assembly) | Run `uloop compile` | | Method on a `partial` type whose body names a member no source file of the assembly declares | A part generated at compile time (a source generator's output) is not visible to hot reload; run `uloop compile` | +| Method or getter on a `partial` type whose body uses an `internal` member of a type the reload was not given, by its bare name, as a method passed as a delegate, inside a lambda, local function, query, iterator or async method, in a body where a lambda, local function or query works with the member's result, or in a body patched through a delegating shim | Hot reload reaches such a member only as a field, a property or a method call in the method's own statements, written with its receiver (`this.Name`, `Type.Name`, `value.Name`); qualify a bare name, or run `uloop compile` | +| Method or getter on a `partial` type whose body uses an `internal` member of a type in another assembly (through `InternalsVisibleTo`) | Reported as a name no source file of the `partial` type declares; hot reload does not patch this use from a `partial` type yet. Run `uloop compile` | | Method on a struct (value type) | Value-type patching is out of scope | | Generic method, or method on a generic type | Harmony cannot safely patch open generics | | Explicit interface implementation | Dotted metadata names cannot be expressed as shim identifiers | diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/OrdinaryMethodQueue.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/OrdinaryMethodQueue.cs index 0b263769e1..d270524907 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/OrdinaryMethodQueue.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/OrdinaryMethodQueue.cs @@ -303,7 +303,10 @@ internal static MethodTransformDecision DecideOrdinaryMethodTransform( WorkerReason siblingSkip = ReappliedSiblingBodyGuard.DescribeSkipOrNull( semanticModel, methodBodyNode, - typeState.TargetAssembly); + typeState.TargetAssembly, + methodDeclaration, + decision, + typeState.TypeSymbol); if (siblingSkip != null) { decision = MethodTransformDecision.Skip(siblingSkip); @@ -317,7 +320,11 @@ internal static MethodTransformDecision DecideOrdinaryMethodTransform( WorkerReason partialSkip = PartialTypeBodyGuard.DescribeSkipOrNull( typeState.TypeDeclaration, semanticModel, - methodBodyNode); + methodBodyNode, + methodDeclaration, + decision, + typeState.TypeSymbol, + typeState.TargetAssembly); if (partialSkip != null) { decision = MethodTransformDecision.Skip(partialSkip); diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PartialTypeBodyGuard.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PartialTypeBodyGuard.cs index 6e01cd8d86..fab773df84 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PartialTypeBodyGuard.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PartialTypeBodyGuard.cs @@ -9,28 +9,40 @@ /// /// Skips a body on a partial type that names something no visible part declares, instead of /// letting it fail the whole file in the shim compile: a part generated at compile time is -/// invisible to the worker, so the name may be perfectly valid. +/// invisible to the worker, so the name may be perfectly valid. An internal member of a compiled +/// type the run was not given looks just as missing to the worker; such a use goes through when +/// the patched method runs it itself, and is skipped with a reason that says why otherwise. /// internal static class PartialTypeBodyGuard { // Why only these: other binding errors are expected (a compiled API still expects the // compiled copy of a type the run declares from source) and the shim compile settles - // them. An unresolved name is what a missing part looks like, and it cannot compile - // in the shim either. + // them. An unresolved name is what a missing part looks like, and it cannot compile in the + // shim either, except for an internal member of a compiled type of the target assembly, + // which the shim compile sees. private static readonly HashSet UnresolvedNameDiagnosticIds = new HashSet(StringComparer.Ordinal) { "CS0103", "CS1061", "CS0117", "CS0246" }; - /// The skip reason for the body, or null when it names nothing unresolved or the type is not partial. + /// + /// The skip reason for the body, or null when the type is not partial, or when the body names + /// nothing unresolved apart from internal members the patched method can reach. + /// internal static WorkerReason DescribeSkipOrNull( TypeDeclarationSyntax typeDeclaration, SemanticModel semanticModel, - SyntaxNode bodyNode) + SyntaxNode bodyNode, + MethodDeclarationSyntax methodDeclarationOrNull, + MethodTransformDecision decision, + INamedTypeSymbol typeSymbol, + IAssemblySymbol targetAssembly) { if (bodyNode == null || !PartialTypeParts.IsPartialOrNestedInPartial(typeDeclaration)) { return null; } + // Why every error and not the first: an internal member the patched method can reach says + // nothing about the next error, which may be a name nothing declares. foreach (Diagnostic diagnostic in semanticModel.GetDiagnostics(bodyNode.Span)) { if (diagnostic.Severity != DiagnosticSeverity.Error) @@ -43,9 +55,29 @@ internal static WorkerReason DescribeSkipOrNull( continue; } + string diagnosticText = diagnostic.Id + ": " + diagnostic.GetMessage(CultureInfo.InvariantCulture); + UnpassedInternalMemberUse use = UnpassedInternalMemberUse.FindOrNull( + diagnostic, + semanticModel, + bodyNode, + methodDeclarationOrNull, + decision, + typeSymbol, + targetAssembly); + if (use == null) + { + return WorkerReason.Of(HotReloadWorkerReasonCode.MethodTransformPartialBodyUnbound, diagnosticText); + } + + if (use.CanBePatchedInPlace) + { + continue; + } + return WorkerReason.Of( - HotReloadWorkerReasonCode.MethodTransformPartialBodyUnbound, - diagnostic.Id + ": " + diagnostic.GetMessage(CultureInfo.InvariantCulture)); + HotReloadWorkerReasonCode.MethodTransformUnpassedInternalMemberOutOfReach, + diagnosticText, + "'" + use.DeclaringType.Name + "'"); } return null; diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterClassifier.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterClassifier.cs index f5414c2d5c..d9c50ae260 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterClassifier.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterClassifier.cs @@ -69,6 +69,7 @@ internal static (bool SkipGetter, MethodTransformDecision Decision) TrySkipPrope AddedFieldCatalog addedFieldCatalog, AddedPropertyCatalog addedPropertyCatalog, PartialTypeParts partialTypeParts, + IAssemblySymbol targetAssembly, List skipped) { MethodTransformDecision decision = MethodTransformDecider.DecideMethodTransform( @@ -93,7 +94,14 @@ internal static (bool SkipGetter, MethodTransformDecision Decision) TrySkipPrope return (true, decision); } - WorkerReason partialSkip = PartialTypeBodyGuard.DescribeSkipOrNull(typeDeclaration, semanticModel, getterBodyNode); + WorkerReason partialSkip = PartialTypeBodyGuard.DescribeSkipOrNull( + typeDeclaration, + semanticModel, + getterBodyNode, + methodDeclarationOrNull: null, + decision, + typeSymbol, + targetAssembly); if (partialSkip != null) { skipped.Add(new WorkerSkipped diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterEmitter.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterEmitter.cs index 68376d388f..202bf8802b 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterEmitter.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/PropertyGetterEmitter.cs @@ -216,6 +216,7 @@ internal static ShimTypeBuilder AppendPropertyGetterEntry( addedFieldCatalog, addedPropertyCatalog, partialTypeParts, + targetAssembly, skipped); if (skipGetter) { @@ -230,7 +231,10 @@ internal static ShimTypeBuilder AppendPropertyGetterEntry( WorkerReason siblingSkip = ReappliedSiblingBodyGuard.DescribeSkipOrNull( semanticModel, getterBodyNode, - targetAssembly); + targetAssembly, + methodDeclarationOrNull: null, + decision, + typeSymbol); if (siblingSkip != null) { skipped.Add(new WorkerSkipped diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/ReappliedSiblingBodyGuard.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/ReappliedSiblingBodyGuard.cs index 23fdef6758..d198d7422a 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/ReappliedSiblingBodyGuard.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/ReappliedSiblingBodyGuard.cs @@ -7,7 +7,9 @@ /// /// Skips an existing method of a file the run pulled in to re-bind its active patches when that -/// method's body no longer binds, and names the file the reader can pass so it binds again. +/// method's body no longer binds, and names the file the reader can pass so it binds again. A body +/// whose only errors are internal members of unpassed compiled types that the patched method +/// reaches itself goes through, because the shim compile binds them. /// /// /// Why only such files: the binding guard runs for added methods alone, so an existing body @@ -22,7 +24,10 @@ internal static class ReappliedSiblingBodyGuard internal static WorkerReason DescribeSkipOrNull( SemanticModel semanticModel, SyntaxNode methodBodyNode, - IAssemblySymbol targetAssembly) + IAssemblySymbol targetAssembly, + MethodDeclarationSyntax methodDeclarationOrNull, + MethodTransformDecision decision, + INamedTypeSymbol typeSymbol) { Diagnostic bindingError = AddedMemberBindingGuard.FindFirstBindingError(semanticModel, methodBodyNode); if (bindingError == null) @@ -30,6 +35,17 @@ internal static WorkerReason DescribeSkipOrNull( return null; } + if (EveryErrorCanBePatchedInPlace( + semanticModel, + methodBodyNode, + methodDeclarationOrNull, + decision, + typeSymbol, + targetAssembly)) + { + return null; + } + string diagnosticText = bindingError.Id + ": " + bindingError.GetMessage(CultureInfo.InvariantCulture); INamedTypeSymbol receiver = FindCompiledReceiverOfUnboundMember(semanticModel, methodBodyNode, targetAssembly); if (receiver == null) @@ -44,6 +60,41 @@ internal static WorkerReason DescribeSkipOrNull( "'" + receiver.Name + "'"); } + // Why every error has to qualify, and the skip keeps today's reason otherwise: one other error + // still fails the file in the shim, or a use out of the patched method's reach throws once + // called, and the first error is what the reason has always named. + private static bool EveryErrorCanBePatchedInPlace( + SemanticModel semanticModel, + SyntaxNode methodBodyNode, + MethodDeclarationSyntax methodDeclarationOrNull, + MethodTransformDecision decision, + INamedTypeSymbol typeSymbol, + IAssemblySymbol targetAssembly) + { + foreach (Diagnostic diagnostic in semanticModel.GetDiagnostics(methodBodyNode.Span)) + { + if (diagnostic.Severity != DiagnosticSeverity.Error) + { + continue; + } + + UnpassedInternalMemberUse use = UnpassedInternalMemberUse.FindOrNull( + diagnostic, + semanticModel, + methodBodyNode, + methodDeclarationOrNull, + decision, + typeSymbol, + targetAssembly); + if (use == null || !use.CanBePatchedInPlace) + { + return false; + } + } + + return true; + } + // Why not CompiledSignatureSplitCollector: it names a compiled API whose signature still takes // the compiled copy of a type this run declares, while here the member itself is missing from // the compiled type, so there is no split for it to find. diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/UnpassedInternalMemberUse.cs b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/UnpassedInternalMemberUse.cs new file mode 100644 index 0000000000..1a4ba1d812 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/HotReload/TransformWorker~/UnpassedInternalMemberUse.cs @@ -0,0 +1,291 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +/// +/// A use, inside a method body, of an internal member declared by a compiled type of the target +/// assembly that the run does not declare: the worker's binding reports the member as missing, +/// while the shim compile binds it and the patched method can call it. +/// +/// +/// Why the worker reports such a member as missing rather than inaccessible: its binding +/// compilation imports a referenced assembly through the public surface only, so an internal +/// member of a compiled type is not there at all. The shim compile references a copy of the +/// target assembly with every member made public. +/// +internal sealed class UnpassedInternalMemberUse +{ + private static readonly HashSet MemberNotFoundDiagnosticIds = + new HashSet(StringComparer.Ordinal) { "CS0103", "CS1061", "CS0117" }; + + private UnpassedInternalMemberUse(INamedTypeSymbol declaringType, bool canBePatchedInPlace) + { + DeclaringType = declaringType; + CanBePatchedInPlace = canBePatchedInPlace; + } + + /// The compiled type that declares the member, read with every member visible. + internal INamedTypeSymbol DeclaringType { get; } + + /// True when the patched method itself runs the use, so a guard may let the body through. + internal bool CanBePatchedInPlace { get; } + + /// + /// The use reports, or null when the error is not a missing member that + /// turns out to be an internal member of a compiled type of the target assembly the run does + /// not declare. + /// + internal static UnpassedInternalMemberUse FindOrNull( + Diagnostic error, + SemanticModel semanticModel, + SyntaxNode bodyNode, + MethodDeclarationSyntax methodDeclarationOrNull, + MethodTransformDecision decision, + INamedTypeSymbol enclosingType, + IAssemblySymbol targetAssembly) + { + Debug.Assert(error != null, "error must not be null."); + Debug.Assert(semanticModel != null, "semanticModel must not be null."); + Debug.Assert(bodyNode != null, "bodyNode must not be null."); + Debug.Assert(decision != null, "decision must not be null."); + Debug.Assert(enclosingType != null, "enclosingType must not be null."); + + if (targetAssembly == null || !MemberNotFoundDiagnosticIds.Contains(error.Id)) + { + return null; + } + + SimpleNameSyntax name = FindReportedNameOrNull(error, bodyNode); + if (name == null) + { + return null; + } + + bool hasReceiver = HasReceiver(name); + INamedTypeSymbol start = hasReceiver ? FindReceiverTypeOrNull(name, semanticModel) : enclosingType; + if (start == null) + { + return null; + } + + ISymbol member = FindInternalMemberOfUnpassedTypeOrNull( + start, + name.Identifier.ValueText, + semanticModel, + targetAssembly); + if (member == null) + { + return null; + } + + // Why a bare name is out of reach: the shim is a static method, and it qualifies a bare + // member name only when the worker binds the name. This member never binds there, so the + // name would reach the shim compile unqualified and fail the whole file. + bool canBePatchedInPlace = hasReceiver + && IsPatchableKind(member, name) + && !RunsOutsideThePatchedMethod(name, bodyNode, methodDeclarationOrNull, decision) + && !HasAClosureOverAnUnresolvedValue(bodyNode, semanticModel); + return new UnpassedInternalMemberUse(member.ContainingType, canBePatchedInPlace); + } + + private static SimpleNameSyntax FindReportedNameOrNull(Diagnostic error, SyntaxNode bodyNode) + { + Location location = error.Location; + if (!location.IsInSource + || location.SourceTree != bodyNode.SyntaxTree + || !bodyNode.Span.Contains(location.SourceSpan)) + { + return null; + } + + // Why a member binding is unwrapped: for 'value?.Name' the compiler reports the whole + // '.Name' binding, while for 'value.Name' and a bare name it reports the name itself. + SyntaxNode reported = bodyNode.FindNode(location.SourceSpan, getInnermostNodeForTie: true); + if (reported is MemberBindingExpressionSyntax binding) + { + return binding.Name; + } + + return reported as SimpleNameSyntax; + } + + // 'value.Name', 'Type.Name' and 'this.Name', or 'value?.Name'. + private static bool HasReceiver(SimpleNameSyntax name) + { + if (name.Parent is MemberAccessExpressionSyntax access) + { + return access.Name == name; + } + + return name.Parent is MemberBindingExpressionSyntax binding && binding.Name == name; + } + + private static INamedTypeSymbol FindReceiverTypeOrNull(SimpleNameSyntax name, SemanticModel semanticModel) + { + ExpressionSyntax receiver = name.Parent is MemberAccessExpressionSyntax access + ? access.Expression + : name.FirstAncestorOrSelf()?.Expression; + if (receiver == null) + { + return null; + } + + // Why not a type parameter, pointer, dynamic or Nullable receiver: the member is not + // looked up on a compiled type of the target assembly through any of them. + INamedTypeSymbol type = semanticModel.GetTypeInfo(receiver).Type as INamedTypeSymbol; + if (type == null || type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T) + { + return null; + } + + return type; + } + + private static ISymbol FindInternalMemberOfUnpassedTypeOrNull( + INamedTypeSymbol start, + string memberName, + SemanticModel semanticModel, + IAssemblySymbol targetAssembly) + { + INamedTypeSymbol compiled = FindInTargetAssemblyOrNull(start.OriginalDefinition, semanticModel, targetAssembly); + for (INamedTypeSymbol type = compiled; type != null; type = type.BaseType?.OriginalDefinition) + { + // Why only types of the target assembly: the shim compile makes every member public in + // the target assembly and in some project assemblies, and the worker cannot tell which + // others. The full import shows the internal members of another assembly's base type too. + if (!IsOfAssembly(type, targetAssembly)) + { + continue; + } + + // Why skip a type the run declares: its members come from the source parts the run + // reads, so a member missing there is a part the worker cannot see, which is what the + // missing-name reason explains. Its compiled copy would still list that member. + if (IsDeclaredByTheRun(type, semanticModel)) + { + continue; + } + + // Why internal only: another type's private member is a use the real compiler rejects + // too, and protected or protected internal members already bind in the worker. + foreach (ISymbol member in type.GetMembers(memberName)) + { + if (member.DeclaredAccessibility == Accessibility.Internal) + { + return member; + } + } + } + + return null; + } + + // Why names and not symbols: the target assembly comes from a separate compilation that + // imports every member, so its types are other symbols than the ones this body binds to. + private static INamedTypeSymbol FindInTargetAssemblyOrNull( + INamedTypeSymbol type, + SemanticModel semanticModel, + IAssemblySymbol targetAssembly) + { + IAssemblySymbol owner = type.ContainingAssembly; + if (owner == null) + { + return null; + } + + bool declaredByTheRun = owner.Identity.Equals(semanticModel.Compilation.Assembly.Identity); + if (!declaredByTheRun && !owner.Identity.Equals(targetAssembly.Identity)) + { + return null; + } + + return targetAssembly.GetTypeByMetadataName(ConstDriftCollector.ToReflectionMetadataName(type)); + } + + private static bool IsOfAssembly(INamedTypeSymbol type, IAssemblySymbol assembly) + { + return type.ContainingAssembly != null && type.ContainingAssembly.Identity.Equals(assembly.Identity); + } + + // Why the compilation's own assembly: it holds only the types the run declares from source, + // while Compilation.GetTypeByMetadataName would also find every referenced type. + private static bool IsDeclaredByTheRun(INamedTypeSymbol type, SemanticModel semanticModel) + { + return semanticModel.Compilation.Assembly.GetTypeByMetadataName(ConstDriftCollector.ToReflectionMetadataName(type)) != null; + } + + // Why only fields, properties and invoked methods: those are the uses a run has shown to bind + // in the shim and to work once patched. A method passed as a delegate and an event have not + // been run that way, so they stay skipped. + private static bool IsPatchableKind(ISymbol member, SimpleNameSyntax name) + { + if (member is IFieldSymbol || member is IPropertySymbol) + { + return true; + } + + return member is IMethodSymbol method && method.MethodKind == MethodKind.Ordinary && IsInvoked(name); + } + + private static bool IsInvoked(SimpleNameSyntax name) + { + SyntaxNode callee = HasReceiver(name) ? name.Parent : name; + return callee.Parent is InvocationExpressionSyntax invocation && invocation.Expression == callee; + } + + // Why these places are out of reach: a closure body, an async or iterator state machine, and a + // delegating shim run as ordinary code of the shim assembly, which the runtime checks for + // access, so a call to an internal member there throws MethodAccessException after the run + // reported success. Only the statements copied into the patched method skip that check. + private static bool RunsOutsideThePatchedMethod( + SyntaxNode name, + SyntaxNode bodyNode, + MethodDeclarationSyntax methodDeclarationOrNull, + MethodTransformDecision decision) + { + if (decision.UsesDelegation) + { + return true; + } + + if (MethodTransformDecider.IsAsyncOrIterator(methodDeclarationOrNull, bodyNode)) + { + return true; + } + + foreach (SyntaxNode closureBody in MethodTransformDecider.FindClosureBodies(bodyNode)) + { + if (closureBody.Span.Contains(name.Span)) + { + return true; + } + } + + return false; + } + + // Why a closure over a value the worker could not resolve keeps every use in the body out: the + // worker reports no error for a use that follows such a value, like the result of an internal + // member, so it cannot tell whether the closure reaches an internal member through it. A closure + // runs as ordinary code of the shim assembly, where that use throws once called. The method's + // own statements run inside the patched method, where a following internal use works, so only + // closures are checked. + private static bool HasAClosureOverAnUnresolvedValue(SyntaxNode bodyNode, SemanticModel semanticModel) + { + foreach (SyntaxNode closureBody in MethodTransformDecider.FindClosureBodies(bodyNode)) + { + foreach (SyntaxNode node in closureBody.DescendantNodesAndSelf()) + { + if (node is ExpressionSyntax expression + && semanticModel.GetTypeInfo(expression).Type?.TypeKind == TypeKind.Error) + { + return true; + } + } + } + + return false; + } +}