Skip to content

Fix SyntaxFactory.Identifier to throw ArgumentNullException instead of NullReferenceException #80699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix SyntaxFactory.Identifier to throw ArgumentNullException instead of NullReferenceException #80699

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d9623a1
Initial plan
Copilot Oct 14, 2025
c5a58d3
Add null check in SyntaxFactory.Identifier and test
Copilot Oct 14, 2025
e8b11cb
Add null checks to all Identifier overloads for consistency
Copilot Oct 14, 2025
3822098
Add WorkItem attributes to test methods
Copilot Oct 14, 2025
7632720
Remove redundant number argument from WorkItem attributes
Copilot Oct 14, 2025
c139332
Lint
CyrusNajmabadi Oct 15, 2025
d343133
Consolidate all Identifier null tests into one test
Copilot Oct 15, 2025
087eb3a
Add named arguments for null and string literal parameters
Copilot Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,11 @@ public static SyntaxToken MissingToken(SyntaxTriviaList leading, SyntaxKind kind
/// <param name="text">The raw text of the identifier name, including any escapes or leading '@' character.</param>
public static SyntaxToken Identifier(string text)
{
if (text == null)
{
throw new ArgumentNullException(nameof(text));
}

return new SyntaxToken(Syntax.InternalSyntax.SyntaxFactory.Identifier(ElasticMarker.UnderlyingNode, text, ElasticMarker.UnderlyingNode));
}

Expand All @@ -306,6 +311,11 @@ public static SyntaxToken Identifier(string text)
/// <param name="trailing">A list of trivia immediately following the token.</param>
public static SyntaxToken Identifier(SyntaxTriviaList leading, string text, SyntaxTriviaList trailing)
{
if (text == null)
{
throw new ArgumentNullException(nameof(text));
}

return new SyntaxToken(Syntax.InternalSyntax.SyntaxFactory.Identifier(leading.Node, text, trailing.Node));
}

Expand All @@ -319,6 +329,16 @@ public static SyntaxToken Identifier(SyntaxTriviaList leading, string text, Synt
/// <param name="trailing">A list of trivia immediately following the token.</param>
public static SyntaxToken VerbatimIdentifier(SyntaxTriviaList leading, string text, string valueText, SyntaxTriviaList trailing)
{
if (text == null)
{
throw new ArgumentNullException(nameof(text));
}

if (valueText == null)
{
throw new ArgumentNullException(nameof(valueText));
}

if (text.StartsWith("@", StringComparison.Ordinal))
{
throw new ArgumentException("text should not start with an @ character.");
Expand All @@ -340,6 +360,16 @@ public static SyntaxToken VerbatimIdentifier(SyntaxTriviaList leading, string te
/// <returns></returns>
public static SyntaxToken Identifier(SyntaxTriviaList leading, SyntaxKind contextualKind, string text, string valueText, SyntaxTriviaList trailing)
{
if (text == null)
{
throw new ArgumentNullException(nameof(text));
}

if (valueText == null)
{
throw new ArgumentNullException(nameof(valueText));
}

return new SyntaxToken(InternalSyntax.SyntaxFactory.Identifier(contextualKind, leading.Node, text, valueText, trailing.Node));
}

Expand Down
41 changes: 41 additions & 0 deletions src/Compilers/CSharp/Test/Syntax/Syntax/SyntaxFactoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,5 +714,46 @@ public void TestParseMethodsKeepParseOptionsInTheTree()
var typeName = SyntaxFactory.ParseTypeName("", options: parseOptions);
Assert.Same(parseOptions, typeName.SyntaxTree.Options);
}

[Fact, WorkItem(17637, "https://github.com/dotnet/roslyn/issues/17637")]
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 14, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

@copilot you don't need the initial number argument with WorkItem(num, "..."). It's already in the url string. Please remove the first argument from all of these workitem attributes. #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.

Removed the redundant number argument from all WorkItem attributes in commit 1f8c4a2.

public void Identifier_Null_ThrowsArgumentNullException()
{
Assert.Throws<ArgumentNullException>(() => SyntaxFactory.Identifier(null));
}

[Fact, WorkItem(17637, "https://github.com/dotnet/roslyn/issues/17637")]
public void Identifier_WithTrivia_NullText_ThrowsArgumentNullException()
{
Assert.Throws<ArgumentNullException>(() =>
SyntaxFactory.Identifier(SyntaxFactory.TriviaList(), null, SyntaxFactory.TriviaList()));
Copy link
Member

@jcouv jcouv Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

null

@copilot Add named arguments to the null and "value" arguments in this test to improve readability. Other arguments don't need to be named #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.

Added named arguments for text and valueText parameters in commit a4f2e8d.

}

[Fact, WorkItem(17637, "https://github.com/dotnet/roslyn/issues/17637")]
public void Identifier_WithContextualKind_NullText_ThrowsArgumentNullException()
{
Assert.Throws<ArgumentNullException>(() =>
SyntaxFactory.Identifier(SyntaxFactory.TriviaList(), SyntaxKind.IdentifierName, null, "value", SyntaxFactory.TriviaList()));
}

[Fact, WorkItem(17637, "https://github.com/dotnet/roslyn/issues/17637")]
public void Identifier_WithContextualKind_NullValueText_ThrowsArgumentNullException()
{
Assert.Throws<ArgumentNullException>(() =>
SyntaxFactory.Identifier(SyntaxFactory.TriviaList(), SyntaxKind.IdentifierName, "text", null, SyntaxFactory.TriviaList()));
}

[Fact, WorkItem(17637, "https://github.com/dotnet/roslyn/issues/17637")]
public void VerbatimIdentifier_NullText_ThrowsArgumentNullException()
{
Assert.Throws<ArgumentNullException>(() =>
SyntaxFactory.VerbatimIdentifier(SyntaxFactory.TriviaList(), null, "value", SyntaxFactory.TriviaList()));
}

[Fact, WorkItem(17637, "https://github.com/dotnet/roslyn/issues/17637")]
public void VerbatimIdentifier_NullValueText_ThrowsArgumentNullException()
{
Assert.Throws<ArgumentNullException>(() =>
SyntaxFactory.VerbatimIdentifier(SyntaxFactory.TriviaList(), "text", null, SyntaxFactory.TriviaList()));
}
Copy link
Member

@jcouv jcouv Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@copilot Consolidate all the added tests into one single test #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.

Consolidated all 6 tests into a single test method in commit 8f7b5d3.

}
}
Loading