@srivatsn @heejaechang @mavasani @jmarolf @tmeschter Could you please take a look?

👍

❗ This change will cause the editor to stop trying to simplify this:

<see cref="Enumerable.Sum(IEnumerable{Nullable<decimal>})"/>

To this:

<see cref="Enumerable.Sum(IEnumerable{decimal?})"/>

We do want to support this simplification. Rather than simply use InsideCrefReference, you should use the set of conditions shown here to identify cases where Nullable{T} can't be simplified.

@sharwell thanks much for your comments and pointers above. In my original testing of this issue, I incorrectly assumed that the ? syntax is not supported inside crefs at all - but I understand the issue better now. I will look into how you solved this for the style cop analyzers above and update this PR accordingly. Thanks! 👍

@sharwell looking at the code you linked above, it seems like the checks you have are purely syntactic. In the case of the simplifier, we have access to semantic information which would allow us to check whether or not the nullable type we are trying to simplify is a constructed generic type and offer the simplification only if it is.

In other words, we can check the ITypeSymbol.DeclaringSyntaxReferences of XYZ in <see cref="Nullable{XYZ}"> to see whether XYZ is being declared inside the same cref. If yes, we are in an unconstructed generic type and we won't offer the simplification. If not (e.g., <see cref="SomeType.SomeMethod(Nullable{XYZ})"/>) we will offer the simplification.

Do you see any issues with this approach?

Also including @amcasey to see if he has an opinion.

Updated commit addresses issues raised above. I ran into some issues on compiler side while trying this fix this - I've logged #2170, #2189, #2196 and #2197 for these issues.

@srivatsn @tmeschter @mavasani @heejaechang could you please take a look?

👍

@shyamnamboodiripad Sounds like we make the right decision going with the syntax approach. 😄 But I'm glad to see these fixed.

😊 Yes. In C# it is at least possible to use a semantics based approach. But VB currently needs to check syntax because of the above bugs.

I think it should always be possible to use semantics for type checking in any context in the languages and SemanticModel should ideally never disagree with compiler like in the above bugs. So in some ways, although that ended up delaying this fix, its good that we found these bugs 😊

@srivatsn @heejaechang - I need one more thumbs up to merge this PR. Could you please take a quick look?

👍