Skip to content

C#: Fix compiler warning of possible null de-reference. #14693

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
michaelnebel merged 2 commits into from
Nov 8, 2023
Merged

C#: Fix compiler warning of possible null de-reference. #14693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69e8621
C#: Fix compiler warning of possible null de-reference.
michaelnebel Nov 6, 2023
0cf00eb
C#: Re-introduce null check in cached entity hash code calculation.
michaelnebel Nov 8, 2023
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
2 changes: 1 addition & 1 deletion csharp/extractor/Semmle.Extraction.CSharp/Entities/Attribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ private void ExtractArguments(TextWriter trapFile)
// The current argument is not named
// so the previous ones were also not named
// so the child index matches the parameter index.
isParamsParameter = Symbol?.AttributeConstructor?.Parameters[childIndex].IsParams == true;
isParamsParameter = Symbol.AttributeConstructor?.Parameters[childIndex].IsParams == true;
argSyntax = ctorArguments[childIndex];
}

Expand Down
2 changes: 1 addition & 1 deletion csharp/extractor/Semmle.Extraction.CSharp/Entities/NonGeneratedSourceLocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public override void Populate(TextWriter trapFile)
Position.Span.Start.Line + 1, Position.Span.Start.Character + 1,
Position.Span.End.Line + 1, Position.Span.End.Character);

var mapped = Symbol!.GetMappedLineSpan();
var mapped = Symbol.GetMappedLineSpan();
if (mapped.HasMappedPath && mapped.IsValid)
{
var mappedLoc = Create(Context, Location.Create(mapped.Path, default, mapped.Span));
Expand Down
2 changes: 2 additions & 0 deletions csharp/extractor/Semmle.Extraction/Entities/Base/CachedEntity`1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Diagnostics.CodeAnalysis;
using System.IO;
using Microsoft.CodeAnalysis;

Expand Down Expand Up @@ -30,6 +31,7 @@ protected CachedEntity(Context context) : base(context)
/// <typeparam name="TSymbol">The type of the symbol.</typeparam>
public abstract class CachedEntity<TSymbol> : CachedEntity where TSymbol : notnull
{
[NotNull]
Copy link
Copy Markdown
Contributor

@tamasvajk tamasvajk Nov 7, 2023

Choose a reason for hiding this comment

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

There are a couple of #nullable disable warnings scattered around the code that relate to cached entities, such as in Compilation, Type, VarargsParam. It might be worth checking if these preprocessor directives can be also removed, or if any of the classes benefit from the Symbol not being marked as not null.

Copy link
Copy Markdown
Contributor Author

@michaelnebel michaelnebel Nov 7, 2023

Choose a reason for hiding this comment

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

Oh no! Good that you bring this up! Then the change as it is might not work! I will look into it.

Copy link
Copy Markdown
Contributor Author

@michaelnebel michaelnebel Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I just took a look at the inheritance hierarchy for CachedEntity.
There are many places where "null" is passed specifically as the Symbol and then most likely all the methods that rely on Symbol are overridden.
If it should really be fixed, then we probably need to distinguish between cached entities and nullable cached entities, which requires a larger re-write.

I think I will re-introduce the null check in the GetHashCode method to make sure that nothing gets broken (but in principle this method should always be overwritten in case Symbol could be null).

None of the disable nullable warnings can be removed as the Symbol can be (or is actually) null.
However, I think it is still a good idea to tag the property as "not null" as all the "correct" usages of the class shouldn't suffer from the hacks where the warning is disabled.

public TSymbol Symbol { get; }

protected CachedEntity(Context context, TSymbol symbol) : base(context)
Expand Down