Skip to content

Conversation

@aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Oct 23, 2025

I believe that exception splitting adds almost zero value, so this PR simply gets rid of it.

@github-actions github-actions bot added the C# label Oct 23, 2025
@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Oct 23, 2025
@aschackmull
Copy link
Contributor Author

Dca looks good. dotnet__aspnetcore has one FP removed for cs/constant-condition, and all other result changes are confined to mono in buildmode=none. I've gone through a lot of these, there are a few precision increases, and a number of new FPs, but in all cases it appears that the changes arise from previous mishandling of splitting.

@aschackmull aschackmull marked this pull request as ready for review October 23, 2025 11:05
@aschackmull aschackmull requested a review from a team as a code owner October 23, 2025 11:05
Copilot AI review requested due to automatic review settings October 23, 2025 11:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request removes exception splitting functionality from the C# control flow graph analysis, treating all exceptions uniformly instead of differentiating by exception type.

Key Changes:

  • Removed exception handler splitting logic that tracked specific exception types through catch clauses
  • Updated test expectations to reflect simplified exception handling in control flow graphs
  • Eliminated exception type annotations from SSA definitions and control flow nodes

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
csharp/ql/lib/semmle/code/csharp/controlflow/internal/Splitting.qll Removed ExceptionHandlerSplitting module and TExceptionHandlerSplitKind type
csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll Removed ExceptionHandlerSplit class export
csharp/ql/test/library-tests/exceptions/Exceptions1.expected Added new exception flow edges that were previously filtered by exception type
csharp/ql/test/library-tests/dataflow/ssa/*.expected Removed exception type annotations from SSA definitions
csharp/ql/test/library-tests/controlflow/graph/*.expected Updated control flow graph test expectations to remove exception type splits
csharp/ql/test/library-tests/assignables/AssignableDefinitionNode.expected Removed exception type annotation from assignable definition
csharp/ql/test/library-tests/csharp8/switchexprcontrolflow.expected Simplified exception control flow in switch expressions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

LGTM!
Thx!

@aschackmull aschackmull merged commit 3542cda into github:main Oct 24, 2025
25 checks passed
@aschackmull aschackmull deleted the csharp/disable-exc-split branch October 24, 2025 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants