Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 14, 2025

Fix ArgumentException in SeparatedSyntaxList.ReplaceSeparator

Fixes the issue where ReplaceSeparator method throws ArgumentException with parameter names as messages instead of proper exception messages.

Changes Made

  • Fix the two ArgumentException calls in ReplaceSeparator method to use proper message and paramName parameters
  • Break combined RawKind/Language check into two separate checks with dedicated resource strings
  • Build and test the changes
  • Code review

Details

  • Fixed first exception (separator not found) to use CodeAnalysisResources.MissingListItem as message and nameof(separatorToken) as paramName
  • Split the combined RawKind/Language check into two separate checks:
    • RawKind mismatch: Uses CodeAnalysisResources.SeparatorTokenMustHaveSameRawKind
    • Language mismatch: Uses CodeAnalysisResources.SeparatorTokenMustHaveSameLanguage
  • Added two new resource strings to CodeAnalysisResources.resx
  • Updated .xlf files using dotnet msbuild /t:UpdateXlf
  • All tests pass (10 SeparatedSyntaxListTests)

Verification

Before fix:

  • Exception Message: 'separatorToken', ParamName: ''
  • Exception Message: 'newSeparator', ParamName: ''

After fix:

  • Exception Message: 'The item specified is not the element of a list. (Parameter 'separatorToken')', ParamName: 'separatorToken'
  • Exception Message: 'New separator must have the same RawKind as the separator being replaced. (Parameter 'newSeparator')', ParamName: 'newSeparator'
  • Exception Message: 'New separator must have the same Language as the separator being replaced. (Parameter 'newSeparator')', ParamName: 'newSeparator'
Original prompt

This section details on the original issue you should resolve

<issue_title>SeparatedSyntaxList.ReplaceSeparator throws ArgumentException with bad message</issue_title>
<issue_description>The ReplaceSeparator method throws ArgumentException passing in the parameter name instead of a message as required by the exception constructor. This results in exceptions with odd one-word messages being thrown.
</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #2496


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…oper messages and parameter names

Co-authored-by: CyrusNajmabadi <[email protected]>
Copilot AI changed the title [WIP] Fix replaced separator error message in SeparatedSyntaxList Fix ArgumentException in SeparatedSyntaxList.ReplaceSeparator to use proper message and paramName Oct 14, 2025
Copilot AI requested a review from CyrusNajmabadi October 14, 2025 09:16
Copilot finished work on behalf of CyrusNajmabadi October 14, 2025 09:16
Assert.Equal(1, newList.GetSeparator(1).GetLeadingTrivia().Count);
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

@copilot remove these tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed the test methods in commit 7c89d.

Copilot AI requested a review from CyrusNajmabadi October 14, 2025 10:41
Copilot finished work on behalf of CyrusNajmabadi October 14, 2025 10:41
newSeparator.Language != nodesWithSeps[index].Language)
{
throw new ArgumentException("newSeparator");
throw new ArgumentException("New separator must have the same RawKind and Language as the separator being replaced.", nameof(newSeparator));
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 16, 2025

Choose a reason for hiding this comment

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

@copilot Break this into two separate checks, and add two new resource string exception messages. One for needing to have the same RawKind and one for needing to have the same Language. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Split into two separate checks with dedicated resource strings SeparatorTokenMustHaveSameRawKind and SeparatorTokenMustHaveSameLanguage in commit 0c9ca.

Copilot AI and others added 2 commits October 16, 2025 11:27
Copilot finished work on behalf of CyrusNajmabadi October 16, 2025 11:29
Copilot AI requested a review from CyrusNajmabadi October 16, 2025 11:29
.gitignore Outdated

# WPF temp projects
*wpftmp.*
Program.cs
Copy link
Member

Choose a reason for hiding this comment

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

@copilot remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed Program.cs from .gitignore in commit 26c01.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review October 16, 2025 11:53
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner October 16, 2025 11:53
Copilot AI requested a review from CyrusNajmabadi October 16, 2025 11:58
Copilot finished work on behalf of CyrusNajmabadi October 16, 2025 11:58
@CyrusNajmabadi
Copy link
Member

@dotnet/roslyn-compiler ptal.

@jcouv jcouv self-assigned this Oct 22, 2025
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (commit 6)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SeparatedSyntaxList.ReplaceSeparator throws ArgumentException with bad message

4 participants