FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

C#: Fix compiler warning of possible null de-reference. by michaelnebel · Pull Request #14693 · github/codeql · GitHub

/ codeql Public

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

Merged
michaelnebel merged 2 commits into
github:mainfrom
michaelnebel:csharp/fixcompilerwarning
Nov 8, 2023
Merged

michaelnebel merged 2 commits into
github:mainfrom
michaelnebel:csharp/fixcompilerwarning

Conversation

Copy link
Copy Markdown
Contributor

No description provided.

github-actions Bot added the C# label Nov 6, 2023
michaelnebel added the no-change-note-required This PR does not need a change note label Nov 6, 2023
michaelnebel marked this pull request as ready for review November 6, 2023 15:08
michaelnebel requested a review from a team as a code owner November 6, 2023 15:08
/// <typeparam name="TSymbol">The type of the symbol.</typeparam>
public abstract class CachedEntity<TSymbol> : CachedEntity where TSymbol : notnull
{
[NotNull]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

michaelnebel Nov 8, 2023
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

michaelnebel force-pushed the csharp/fixcompilerwarning branch from d1777de to 0cf00eb Compare November 8, 2023 09:59
michaelnebel merged commit 795e32c into github:main Nov 8, 2023
michaelnebel deleted the csharp/fixcompilerwarning branch November 8, 2023 11:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL