diff --git a/docs/design/value-typed-emission.md b/docs/design/value-typed-emission.md index d87fc4de44..e6096cbd78 100644 --- a/docs/design/value-typed-emission.md +++ b/docs/design/value-typed-emission.md @@ -1444,11 +1444,29 @@ change (instance 3); no claim that the frozen policy is a correctness proof. Which up-front locals keep `= default` is decided before printing, not by the writer (#2095). The analysis is the conservative structured definite-assignment -walk in `DefiniteAssignment` (the `#631` flow walk), unchanged: it yields the +walk in `DefiniteAssignment` (the `#631` flow walk): it yields the locals that may be read before they are definitely assigned and gives up — marking every local — on control flow it does not model, so it can only keep a redundant initializer, never drop a required one. +Primitive-boolean short-circuit conditions carry separate definitely-assigned +sets for their true and false outcomes. Calls establish assignment only for +metadata-verified `out` arguments; a skipped call, an input reference, or an +unknown ref kind cannot. +A branch body consumes its outcome's set, and only paths that reach the normal +continuation participate in its join. Unsupported control flow retains the +conservative fallback, as do logical chains containing user-defined truthiness: +the printer can expose a user-typed operand and bind an overloaded `&&`/`||`, +which does not establish C#'s primitive-boolean outcome facts. +`ShortCircuitDefiniteAssignmentTests` gates both +outcomes, returning guards, nested short-circuit reads, and close declines. +The motivating real witness is `dotnet-inspect.any` 0.14.0, +`ILInspector.Decompiler.Pipeline.TypeRef::DisplayName`: the previous projection +added `string keyword = default` before a guarded `TryGetValue`, introducing +an `ldnull stloc` pair absent from the original. The pinned witness and a +compiler-produced returning-guard sample retain durable regression coverage; +Issue #9931 owns the focused slice; #9929 records the remaining fidelity families. + `DefiniteAssignmentPass` runs last in the emission tail, after residual storage binding and every statement-rewriting pass, and issues `IrFunction.ZeroInitializedLocals`: the analysis result minus the residual-bound @@ -1476,10 +1494,11 @@ depend on it. Two pre-existing analysis limits are unchanged: a lambda that shares its enclosing scope has no separate decision, and a local bound by an `is` pattern counts as read before assignment. -On the pinned 14-assembly, 89,065-method corpus the decision reaches 10,077 -methods (42,822 up-front declarations): 7,392 declarations in 1,781 methods -keep `= default`, 35,309 declare bare, and the residual exclusion changes 121 -declarations in 73 methods; the analysis gives up in 474 methods. Computed at +At the Instance 3 introduction, the pinned 14-assembly, 89,065-method corpus +decision reached 10,077 methods (42,822 up-front declarations): 7,392 +declarations in 1,781 methods kept `= default`, 35,309 declared bare, and the +residual exclusion changed 121 declarations in 73 methods; the analysis gave +up in 474 methods. Computed at the host's tail, all 594 analysed nested bodies match what the writer computed at print time. `DefiniteAssignmentPassTests` gates the host set, the residual exclusion (synthetic and Newtonsoft.Json 13.0.4 diff --git a/src/ILInspector.Decompiler/Pipeline/Ir/DefiniteAssignment.cs b/src/ILInspector.Decompiler/Pipeline/Ir/DefiniteAssignment.cs index fda6f87484..bc3e4eb58e 100644 --- a/src/ILInspector.Decompiler/Pipeline/Ir/DefiniteAssignment.cs +++ b/src/ILInspector.Decompiler/Pipeline/Ir/DefiniteAssignment.cs @@ -91,10 +91,8 @@ void CheckReads(IrNode? node, HashSet assigned) return; case Conditional conditional: { - CheckReads(conditional.Condition, assigned); - var trueSet = new HashSet(assigned); + var (trueSet, falseSet) = CheckCondition(conditional.Condition, assigned); CheckReads(conditional.WhenTrue, trueSet); - var falseSet = new HashSet(assigned); CheckReads(conditional.WhenFalse, falseSet); trueSet.IntersectWith(falseSet); assigned.Clear(); @@ -103,9 +101,9 @@ void CheckReads(IrNode? node, HashSet assigned) } case LogicalBinary logical: { - CheckReads(logical.Left, assigned); - var rightSet = new HashSet(assigned); - CheckReads(logical.Right, rightSet); + var (trueSet, falseSet) = CheckCondition(logical, assigned); + trueSet.IntersectWith(falseSet); + assigned.UnionWith(trueSet); return; } case Coalesce coalesce: @@ -216,6 +214,63 @@ void CheckReads(IrNode? node, HashSet assigned) CheckReads(child, assigned); } + // These sets describe the paths that actually yield each boolean + // outcome. The right operand sees only the outcome that evaluates it; + // the skipped path participates in the other outcome's intersection. + (HashSet WhenTrue, HashSet WhenFalse) CheckCondition( + IrExpression condition, HashSet entry) + { + if (condition is LogicalNot not) + { + var (whenTrue, whenFalse) = CheckCondition(not.Operand, entry); + return (whenFalse, whenTrue); + } + if (condition is LogicalBinary logical) + { + // Condition spelling can expose a user-typed operand by stripping + // op_True/op_False. Its overloaded &&/|| does not establish C#'s + // primitive-boolean outcome facts, even though this IR result is + // bool. Keep the unconditional, pre-existing short-circuit proof. + if (ContainsUserDefinedTruthiness(logical)) + { + var unconditional = new HashSet(entry); + CheckReads(logical.Left, unconditional); + CheckReads(logical.Right, new HashSet(unconditional)); + return (unconditional, new HashSet(unconditional)); + } + var (leftTrue, leftFalse) = CheckCondition(logical.Left, entry); + if (logical.Kind == LogicalKind.And) + { + var (rightTrue, rightFalse) = CheckCondition(logical.Right, leftTrue); + leftFalse.IntersectWith(rightFalse); + return (rightTrue, leftFalse); + } + else + { + var (rightTrue, rightFalse) = CheckCondition(logical.Right, leftFalse); + leftTrue.IntersectWith(rightTrue); + return (leftTrue, rightFalse); + } + } + + var assigned = new HashSet(entry); + CheckReads(condition, assigned); + return (assigned, new HashSet(assigned)); + } + + static bool ContainsUserDefinedTruthiness(IrNode node) + { + if (node is Lambda or LocalFunctionStatement) + return false; + if (node is IrExpression expression + && ShortCircuitFidelity.IsUserDefinedTruthiness(expression)) + return true; + foreach (var child in node.Children) + if (ContainsUserDefinedTruthiness(child)) + return true; + return false; + } + void CheckCall(MethodRef callee, IReadOnlyList arguments, int parameterStart, HashSet assigned) { if (parameterStart == 1 && arguments.Count > 0) @@ -726,13 +781,11 @@ when structuredGotoReaching is not null DefiniteFlow If(IfStatement branch, HashSet assigned) { - CheckReads(branch.Condition, assigned); - var thenSet = new HashSet(assigned); + var (thenSet, elseSet) = CheckCondition(branch.Condition, assigned); var thenFlow = Statement(branch.Then, thenSet); - if (!branch.HasElse) - return DefiniteFlow.FallThrough; // the then-arm may be skipped - var elseSet = new HashSet(assigned); - var elseFlow = Statement(branch.Else!, elseSet); + var elseFlow = branch.HasElse + ? Statement(branch.Else!, elseSet) + : DefiniteFlow.FallThrough; var reaching = new List>(); if (thenFlow == DefiniteFlow.FallThrough) reaching.Add(thenSet); diff --git a/tests/ILInspector.Decompiler.Tests/ShortCircuitDefiniteAssignmentTests.cs b/tests/ILInspector.Decompiler.Tests/ShortCircuitDefiniteAssignmentTests.cs new file mode 100644 index 0000000000..f3762589fa --- /dev/null +++ b/tests/ILInspector.Decompiler.Tests/ShortCircuitDefiniteAssignmentTests.cs @@ -0,0 +1,396 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using ILInspector.Decompiler.Pipeline; +using ILInspector.DecompilerHarness; + +namespace ILInspector.Decompiler.Tests; + +[Trait("Area", "Pass")] +public class ShortCircuitDefiniteAssignmentTests +{ + static readonly TypeRef Holder = TypeRef.Definition("Synthetic", "Samples", "Holder"); + static readonly TypeRef Int32 = TypeRef.CoreLib("System", "Int32"); + static readonly TypeRef Bool = TypeRef.CoreLib("System", "Boolean"); + + [Theory] + [InlineData(LogicalKind.And, true, false)] + [InlineData(LogicalKind.And, false, true)] + [InlineData(LogicalKind.Or, true, true)] + [InlineData(LogicalKind.Or, false, false)] + public void OutcomeKeepsOnlyAssignmentsOnEveryReachingPath( + LogicalKind kind, bool readOnTrue, bool needsDefault) + { + var condition = new LogicalBinary(kind, + new LoadArgument(0, "choose", Bool), OutCall()); + var read = Returning(new LoadLocal(0, Int32)); + var other = Returning(new Constant(0, Int32)); + var block = new Block(0); + block.Add(new IfStatement(condition, + readOnTrue ? read : other, + readOnTrue ? other : read)); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Equal(needsDefault, function.ZeroInitializedLocals!.Contains(0)); + } + + [Theory] + [InlineData(ArgumentRefKind.Ref, ParameterRefKindFacts.Known)] + [InlineData(ArgumentRefKind.Out, ParameterRefKindFacts.Unknown)] + public void UnverifiedOrInputReferenceKeepsInitialization( + ArgumentRefKind refKind, ParameterRefKindFacts facts) + { + var block = new Block(0); + block.Add(new IfStatement( + new LogicalBinary(LogicalKind.And, + new LoadArgument(0, "choose", Bool), OutCall(refKind, facts)), + Returning(new LoadLocal(0, Int32)), + Returning(new Constant(0, Int32)))); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Contains(0, function.ZeroInitializedLocals!); + } + + [Fact] + public void ReturningGuardCarriesNegatedFalseOutcomeToContinuation() + { + var block = new Block(0); + block.Add(new IfStatement( + new LogicalBinary(LogicalKind.Or, + new LoadArgument(0, "choose", Bool), new LogicalNot(OutCall())), + Returning(new Constant(0, Int32)), null)); + block.Add(new Return(new LoadLocal(0, Int32))); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Empty(function.ZeroInitializedLocals!); + } + + [Fact] + public void NestedAndUsesTrueOutcomeBeforeReadingRightOperand() + { + var block = new Block(0); + block.Add(new IfStatement(new LogicalBinary(LogicalKind.And, + new LogicalBinary(LogicalKind.And, + new LoadArgument(0, "choose", Bool), OutCall()), + new Comparison(ComparisonKind.GreaterThan, false, + new LoadLocal(0, Int32), new Constant(0, Int32))), + Returning(new LoadLocal(0, Int32)), + Returning(new Constant(0, Int32)))); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Empty(function.ZeroInitializedLocals!); + } + + [Fact] + public void SkippedCallDoesNotAssignTheFallthroughContinuation() + { + var block = new Block(0); + block.Add(new IfStatement(new LogicalBinary(LogicalKind.And, + new LoadArgument(0, "choose", Bool), OutCall()), + Returning(new Constant(0, Int32)), null)); + block.Add(new Return(new LoadLocal(0, Int32))); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Contains(0, function.ZeroInitializedLocals!); + } + + [Fact] + public void ExternalBranchRetainsConservativeInitialization() + { + var block = new Block(0); + block.Add(new ExpressionStatement(OutCall())); + block.Add(new Branch(999)); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Contains(0, function.ZeroInitializedLocals!); + } + + [Fact] + public void HandlerEntryDoesNotInheritProtectedBodyAssignment() + { + var protectedBlock = new Block(0); + protectedBlock.Add(new IfStatement(new LogicalBinary(LogicalKind.And, + new LoadArgument(0, "choose", Bool), OutCall()), + Returning(new LoadLocal(0, Int32)), + Returning(new Constant(0, Int32)))); + var protectedBody = new BlockContainer(); + protectedBody.Add(protectedBlock); + var handler = new BlockContainer(); + handler.Add(Returning(new LoadLocal(0, Int32))); + var block = new Block(0); + block.Add(new TryCatch(protectedBody, + [new CatchClause(TypeRef.CoreLib("System", "Exception"), handler)])); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Contains(0, function.ZeroInitializedLocals!); + } + + [Fact] + public void IsolatedNestedBodyDoesNotInheritHostLocalAssignment() + { + var nestedBody = new BlockContainer(); + nestedBody.Add(Returning(new LoadLocal(0, Int32))); + var nested = new LocalFunctionStatement("Inner", Int32, [], + isStatic: true, [Int32], [], usesUpdatedMemorySafetyRules: false, + skipLocalsInit: false, nestedBody); + var block = new Block(0); + block.Add(nested); + block.Add(new IfStatement(new LogicalBinary(LogicalKind.And, + new LoadArgument(0, "choose", Bool), OutCall()), + Returning(new LoadLocal(0, Int32)), + Returning(new Constant(0, Int32)))); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Empty(function.ZeroInitializedLocals!); + Assert.Contains(0, nested.ZeroInitializedLocals!); + } + + [Theory] + [InlineData(true, false)] + [InlineData(false, true)] + public void ConditionalExpressionUsesItsOwnOutcomeAssignments( + bool readOnTrue, bool needsDefault) + { + var block = new Block(0); + IrExpression value = new LoadLocal(0, Int32); + IrExpression zero = new Constant(0, Int32); + block.Add(new Return(new Conditional(new LogicalBinary(LogicalKind.And, + new LoadArgument(0, "choose", Bool), OutCall()), + readOnTrue ? value : zero, readOnTrue ? zero : value))); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + Assert.Equal(needsDefault, function.ZeroInitializedLocals!.Contains(0)); + } + + [Theory] + [Trait("Speed", "Slow")] + [InlineData(LogicalKind.And, true)] + [InlineData(LogicalKind.And, false)] + [InlineData(LogicalKind.Or, true)] + [InlineData(LogicalKind.Or, false)] + public void RenderedOutcomeAssignmentsBind(LogicalKind kind, bool readOnTrue) + { + var read = Returning(new LoadLocal(0, Int32)); + var other = Returning(new Constant(0, Int32)); + var block = new Block(0); + block.Add(new IfStatement(new LogicalBinary(kind, + new LoadArgument(0, "choose", Bool), OutCall()), + readOnTrue ? read : other, readOnTrue ? other : read)); + var function = Function(block); + + new DefiniteAssignmentPass().Run(function, PassContext.None); + + function.CheckInvariant(); + AssertBinds(function); + } + + [Fact] + [Trait("Speed", "Slow")] + public async Task CompilerProducedGuardCompilesBackExactlyUnderNativeRts() + { + var type = typeof(ShortCircuitAssignmentSample); + string path = type.Assembly.Location; + using var metadata = CorpusMetadata.Create([path]); + using var source = MetadataSource.Open(path, context: metadata); + var function = Assert.IsType(IrImporter.Import( + source, type.FullName!, nameof(ShortCircuitAssignmentSample.Parse))); + var results = await FidelityCheck.EvaluateChangedMethodTargetsForTesting([path], + [new FidelityCheck.CompileBackTarget(path, type.FullName!, + nameof(ShortCircuitAssignmentSample.Parse), 0, + CorpusMethodIdentity.SignatureText(function.Signature))]); + + Assert.Equal(FidelityCheck.CompileBackStatus.Exact, Assert.Single(results).Status); + } + + [Fact] + public void CompilerProducedReturningGuardHasNoDefaultStore() + { + var type = typeof(ShortCircuitAssignmentSample); + using var metadata = CorpusMetadata.Create([type.Assembly.Location]); + using var source = MetadataSource.Open(type.Assembly.Location, context: metadata); + var function = Assert.IsType(IrImporter.Import( + source, type.FullName!, nameof(ShortCircuitAssignmentSample.Parse))); + var result = CSharpPrinter.PrintRaised(function, + method => IrImporter.Import(source, method), + typesProvablyDisjoint: source.AreProvablyDisjoint); + + function.CheckInvariant(); + Assert.Equal(DecompilationFidelity.Full, result.Fidelity); + Assert.NotNull(result.Output); + Assert.DoesNotContain("= default", result.Output); + } + + [Fact] + public void PinnedTypeRefDisplayNameHasNoDefaultStore() + { + string path = Path.Combine(AppContext.BaseDirectory, + "RealAssets", "SwitchSection", "ILInspector.Decompiler.dll"); + using var metadata = CorpusMetadata.Create([path]); + using var source = MetadataSource.Open(path, context: metadata); + var function = Assert.IsType(IrImporter.Import(source, + "ILInspector.Decompiler.Pipeline.TypeRef", "DisplayName")); + var result = CSharpPrinter.PrintRaised(function, + method => IrImporter.Import(source, method), + typesProvablyDisjoint: source.AreProvablyDisjoint); + + function.CheckInvariant(); + Assert.Equal(DecompilationFidelity.Full, result.Fidelity); + Assert.NotNull(result.Output); + Assert.DoesNotContain("= default", result.Output); + } + + [Fact] + [Trait("Speed", "Slow")] + public void CompilerProducedUserTruthinessKeepsInitializationAndBinds() + { + var type = typeof(ShortCircuitTruthinessSample); + using var source = MetadataSource.Open(type.Assembly.Location); + var function = Assert.IsType(IrImporter.Import(source, + type.FullName!, nameof(ShortCircuitTruthinessSample.NestedGuard))); + var result = CSharpPrinter.PrintRaised(function, + method => IrImporter.Import(source, method), + typesProvablyDisjoint: source.AreProvablyDisjoint); + + function.CheckInvariant(); + Assert.Equal(DecompilationFidelity.Full, result.Fidelity); + string body = Assert.IsType(result.Output); + Assert.Contains("&&", body); + Assert.Contains("= default", body); + string text = $$""" + using ILInspector.Decompiler.Tests; + class Shell + { + static ShortCircuitTruthValue Read(out int value) + { + value = 42; + return new ShortCircuitTruthValue(true); + } + static int M(ShortCircuitTruthValue input) + { + {{body}} + } + } + """; + var compilation = CSharpCompilation.Create("truthiness-decline-binding", + [CSharpSyntaxTree.ParseText(text, + cancellationToken: TestContext.Current.CancellationToken)], + RoslynTestReferences.TrustedPlatform.Add( + MetadataReference.CreateFromFile(type.Assembly.Location)), + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + Assert.Empty(compilation.GetDiagnostics(TestContext.Current.CancellationToken) + .Where(d => d.Severity == DiagnosticSeverity.Error)); + } + + static void AssertBinds(IrFunction function, ArgumentRefKind kind = ArgumentRefKind.Out) + { + string body = Assert.IsType(CSharpPrinter.Print(function).Output); + string refKind = kind == ArgumentRefKind.Ref ? "ref" : "out"; + string text = $$""" + namespace Samples; + static class Holder + { + static bool TryGet({{refKind}} int value) { value = 42; return true; } + static int M(bool choose) + { + {{body}} + } + } + """; + var compilation = CSharpCompilation.Create("assignment-gate", + [CSharpSyntaxTree.ParseText(text, new CSharpParseOptions(LanguageVersion.Preview))], + RoslynTestReferences.TrustedPlatform, + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + Assert.Empty(compilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error)); + } + + static Call OutCall(ArgumentRefKind kind = ArgumentRefKind.Out, + ParameterRefKindFacts facts = ParameterRefKindFacts.Known) + => new(new MethodRef(Holder, "TryGet", Bool, [TypeRef.ByRef(Int32)], HasThis: false) + { + ParameterRefKinds = [kind], + ParameterRefKindsFacts = facts, + }, isVirtual: false, [new LoadLocalAddress(0, Int32)]); + + static Block Returning(IrExpression value) + { + var block = new Block(1); + block.Add(new Return(value)); + return block; + } + + static IrFunction Function(Block block) + { + var body = new BlockContainer(); + body.Add(block); + return new IrFunction("M", Holder, + new MethodSignature(Int32, [new Parameter("choose", Bool)], + HasThis: false, GenericParameterCount: 0), [Int32], body); + } +} + +public static class ShortCircuitAssignmentSample +{ + public static int Parse(string text) + { + if (text is null || !int.TryParse(text, out int value)) + return 0; + return value; + } +} + +public readonly struct ShortCircuitTruthValue +{ + readonly bool _value; + + public ShortCircuitTruthValue(bool value) => _value = value; + public static bool operator true(ShortCircuitTruthValue value) => value._value; + public static bool operator false(ShortCircuitTruthValue value) => !value._value; + public static ShortCircuitTruthValue operator &( + ShortCircuitTruthValue left, ShortCircuitTruthValue right) + => new(left._value && right._value); +} + +public static class ShortCircuitTruthinessSample +{ + static ShortCircuitTruthValue Read(out int value) + { + value = 42; + return new ShortCircuitTruthValue(true); + } + + public static int NestedGuard(ShortCircuitTruthValue input) + { + if (input) + { + if (Read(out int value)) + return value; + } + return 0; + } +}