Skip to content

Fix C# output: add _ = discard for non-void calls in if/else block bodies#504

Merged
dadhi merged 4 commits intomasterfrom
copilot/check-csharp-output-improvements
Apr 13, 2026
Merged

Fix C# output: add _ = discard for non-void calls in if/else block bodies#504
dadhi merged 4 commits intomasterfrom
copilot/check-csharp-output-improvements

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 12, 2026

Non-void Call/Invoke expressions used as statements inside IfThen/IfThenElse blocks were printed without a discard assignment, producing misleading (though technically valid) C# output.

Changes

  • ToCSharpBlock: When the body of an if/else block is a non-void Call, Invoke, Conditional, or Coalesce expression, prefix it with _ = to explicitly discard the return value.
  • BlockToCSharpString: Extends the existing Conditional/Coalesce discard check to also cover Call and Invoke node types in block statement position.
  • Test: Adds assertion in Equal_in_void_Handler_should_work to verify the output contains _ = ((int?)null).ToString();.

Before / After

// Before
if (param == (int?)null)
{
    ((int?)null).ToString();
}

// After
if (param == (int?)null)
{
    _ = ((int?)null).ToString();
}

Copilot AI linked an issue Apr 12, 2026 that may be closed by this pull request
1 task
Copilot AI changed the title [WIP] Update nullable type comparison in CSharp output Fix C# output: add _ = discard for non-void calls in if/else block bodies Apr 12, 2026
Copilot AI requested a review from dadhi April 12, 2026 07:09
Copy link
Copy Markdown
Owner

@dadhi dadhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run all tests in debug mode and analyzed PrintCSharp output
to find if thid fix introduced any regressions?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 12, 2026

Did you run all tests in debug mode and analyzed PrintCSharp output
to find if thid fix introduced any regressions?

Yes — ran all 1655 tests via FastExpressionCompiler.TestsRunner in debug mode, all pass. Reviewed the full PrintCSharp output from the run. The new _ = discard appears in the correct places:

  • _ = ((int?)null).ToString(); and _ = param.ToString(); (Issue252 — the fix)
  • _ = BlockTests.Inc(p); (non-void Call as block statement — correct)
  • _ = Issue83_linq2db.CheckNullValue(...); (non-void Call as block statement inside __f(() => {...}) — correct)

No unexpected regressions in the output.

Copilot AI requested a review from dadhi April 12, 2026 07:40
var nodeType = expr?.NodeType ?? ExpressionType.Default;
var needsDiscard = expr != null && expr.Type != typeof(void)
&& (nodeType == ExpressionType.Call | nodeType == ExpressionType.Invoke
| nodeType == ExpressionType.Conditional | nodeType == ExpressionType.Coalesce);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those checks for nodeTyñe are in at least 2 places, and may be more accross c# output, can we sematicalmy compress/unify them to method to simply code and rrmove creep?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted the repeated four-node-type check into NeedsDiscardWhenUsedAsStatement(this Expression expr) (line 8241), placed alongside the other ExpressionType extension helpers. Both ToCSharpBlock and BlockToCSharpString now call this single method. Commit: f973bd3, all 1655 tests still pass.

Copilot AI requested a review from dadhi April 12, 2026 07:56
else
{
sb.NewLineIndent(lineIndent + indentSpaces);
var needsDiscard = expr != null && expr.NeedsDiscardWhenUsedAsStatement();
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need the local var to store bool check used in a single condition?
Does it needs the comment almost repearing the check method nane?
The same for other check - plesse eliminate

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 4a54b45 — inlined the single-use needsDiscard variable in ToCSharpBlock and removed the redundant comments at both call sites. The returningCondOrCoalesceOrCall variable in BlockToCSharpString is kept since it's referenced twice (in the if and in the ternary on the next block).

Copilot AI requested a review from dadhi April 12, 2026 08:59
@dadhi dadhi marked this pull request as ready for review April 13, 2026 12:19
@dadhi dadhi merged commit acb5d06 into master Apr 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

More improvements in CSharp output

2 participants