Skip to content

Fix null refs when calling GetLocation() or GetDiagnostics() of freshly constructed syntax trivia #81444

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

Merged
AlekseyTs merged 4 commits into from
Dec 19, 2025
Merged

Fix null refs when calling GetLocation() or GetDiagnostics() of freshly constructed syntax trivia #81444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f5973db
Fix null refs of freshly constructed syntax trivia
DoctorKrolic Nov 25, 2025
e6a1a1b
Fully match implementation of `GetDiagnostics()` to `SyntaxToken`
DoctorKrolic Dec 3, 2025
253cc39
Add tests
DoctorKrolic Dec 13, 2025
58f92de
Remove unnecessary using
DoctorKrolic Dec 18, 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
25 changes: 25 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 @@ -478,6 +478,31 @@ public void GetDiagnosticsFromNullToken()
token.GetDiagnostics().Verify();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/40773")]
public void ConstructedSyntaxTrivia_NoLocationAndDiagnostics()
{
var trivia = SyntaxFactory.SyntaxTrivia(SyntaxKind.WhitespaceTrivia, " ");
Assert.Equal(Location.None, trivia.GetLocation());
trivia.GetDiagnostics().Verify();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/40773")]
public void ParsedSyntaxTriviaWithoutDiagnostics()
{
var trivia = SyntaxFactory.ParseLeadingTrivia("// Comment").First();
Assert.Equal(Location.None, trivia.GetLocation());
trivia.GetDiagnostics().Verify();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/40773")]
public void ParsedSyntaxTriviaWithDiagnostics()
{
var trivia = SyntaxFactory.ParseLeadingTrivia("/* Unclosed multiline comment").First();
Assert.Equal(Location.None, trivia.GetLocation());
trivia.GetDiagnostics().Verify(
Diagnostic(ErrorCode.ERR_OpenEndedComment).WithLocation(1, 1));
}

[Fact]
[WorkItem(21231, "https://github.com/dotnet/roslyn/issues/21231")]
public void TestSpacingOnNullableIntType()
Expand Down
23 changes: 19 additions & 4 deletions src/Compilers/Core/Portable/Syntax/SyntaxTrivia.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Runtime.InteropServices;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.Text;
Expand Down Expand Up @@ -409,8 +410,7 @@ public SyntaxTree? SyntaxTree
/// </summary>
public Location GetLocation()
{
// https://github.com/dotnet/roslyn/issues/40773
return this.SyntaxTree!.GetLocation(this.Span);
return this.SyntaxTree?.GetLocation(this.Span) ?? Location.None;
}

/// <summary>
Expand All @@ -420,8 +420,23 @@ public Location GetLocation()
/// </summary>
public IEnumerable<Diagnostic> GetDiagnostics()
{
// https://github.com/dotnet/roslyn/issues/40773
return this.SyntaxTree!.GetDiagnostics(this);
if (UnderlyingNode is null)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if (UnderlyingNode is null)

It feels like all individual code paths in this function should have dedicated code coverage. That should include both positive and negative outcomes, when applicable. Also, I have to admit, I am not an expert in this area, and, at the moment, I do not have a good idea about how expected implementation should actually look like. Therefore, it would be good to provide an explanation why this is the right implementation. If the logic was "copied" from some other place, it would be good to mention that as well. #Closed

Copy link
Contributor Author

@DoctorKrolic DoctorKrolic Dec 13, 2025

Choose a reason for hiding this comment

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

When I copied this logic I was not able to come up with a test for the scenario where there is no syntax tree, but the underlying green node exists. After thinking about it for a while I managed to find the case: when parsing trivia via SyntaxFactory.Parse[Leading|Trailing]Trivia we get precisely this scenario, so added tests for both branches (where we have diagnostics and where we don't)

{
return SpecializedCollections.EmptyEnumerable<Diagnostic>();
}

if (this.SyntaxTree is { } syntaxTree)
{
return syntaxTree.GetDiagnostics(this);
}
else
{
var diagnostics = UnderlyingNode.GetDiagnostics();

return diagnostics.Length == 0
? SpecializedCollections.EmptyEnumerable<Diagnostic>()
: diagnostics.Select(Diagnostic.Create);
}
}
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 13, 2025

Choose a reason for hiding this comment

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

any problem keeping the logic directly equivalent to SyntaxToken? like line for line. In other words, looking like:

            if (UnderlyingNode == null)
            {
                return SpecializedCollections.EmptyEnumerable<Diagnostic>();
            }

            var tree = SyntaxTree;

            if (tree == null)
            {
                var diagnostics = UnderlyingNode.GetDiagnostics();

                return diagnostics.Length == 0
                    ? SpecializedCollections.EmptyEnumerable<Diagnostic>()
                    : diagnostics.Select(s_createDiagnosticWithoutLocation);
            }

            return tree.GetDiagnostics(this);

Copy link
Contributor Author

@DoctorKrolic DoctorKrolic Dec 13, 2025

Choose a reason for hiding this comment

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

Where should we extract this logic? Is there a good existing helper class for such things?

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 13, 2025

Choose a reason for hiding this comment

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

feels like it could all be on SyntaxTree. It just needs a GreenNode+pos to be able to operate.

Copy link
Contributor Author

@DoctorKrolic DoctorKrolic Dec 13, 2025

Choose a reason for hiding this comment

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

The VB part also needs to know whether the node/token is in doc comments section, for which it walks the tree upwards from the given node, which is not available for green nodes. I mean we can extract the "getting diagnostic from the tree" part into a delegate, but that will make code more complex, introduce allocation of a delegate etc

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 13, 2025

Choose a reason for hiding this comment

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

My primary feedback was really just to keep the code written the same way. It's ok to still have the duplicated impl.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 13, 2025

Choose a reason for hiding this comment

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

My primary feedback was really just to keep the code written the same way. It's ok to still have the duplicated impl.


/// <summary>
Expand Down
Loading