Skip to content

Conversation

@michaelnebel
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the C# label Nov 6, 2023
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Nov 6, 2023
@michaelnebel michaelnebel marked this pull request as ready for review November 6, 2023 15:08
@michaelnebel 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
Contributor

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
Contributor Author

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
Contributor Author

@michaelnebel michaelnebel Nov 8, 2023

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.

@michaelnebel michaelnebel force-pushed the csharp/fixcompilerwarning branch from d1777de to 0cf00eb Compare November 8, 2023 09:59
@michaelnebel michaelnebel merged commit 795e32c into github:main Nov 8, 2023
@michaelnebel michaelnebel deleted the csharp/fixcompilerwarning branch November 8, 2023 11:04
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