Remove CS1701 and CS1702 warnings

[Youssef1313]

Very quick PR to fix #19640

[Youssef1313]

Draft for now. I'll debug the failing tests later :(
I'm okay in case someone wanted to complete this before me.

[AlekseyTs]

Are all use-site diagnostics on symbols errors now? As in VB. If so, we probably should rename all relevant APIs that use "UseSiteDiagnostic" to "UseSiteError" to reflect the fact. Some checks for use-site errors (vs. warnings), if any, could be simplified as well. Also, we should see if calculation/merging of use-site diagnostics could be optimized/simplified (get out early, etc.) now.

[Youssef1313]

@AlekseyTs Could this be done in a separate follow-up PR (and open an issue to track that)?

[AlekseyTs]

Could this be done in a separate follow-up PR (and open an issue to track that)?

I think that if we want to address the issue, we should do that without any need for an additional follow up. I do not see any urgency in stopping reporting the warnings. If we think that the issue is worth addressing, we should do that in its entirety.

[Youssef1313]

@AlekseyTs I took a look

roslyn/src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs

Lines 1048 to 1055 in ab23769

	nullabilityDiagnosticsBuilderOpt.Add(new TypeParameterDiagnosticInfo(typeParameter, new UseSiteInfo<AssemblySymbol>(new CSDiagnosticInfo(ErrorCode.WRN_NullabilityMismatchInTypeParameterNotNullConstraint, containingSymbol.ConstructedFrom(), typeParameter, typeArgument))));
	}
	if (typeParameter.HasReferenceTypeConstraint &&
	typeParameter.ReferenceTypeConstraintIsNullable == false &&
	typeArgument.GetValueNullableAnnotation().IsAnnotated())
	{
	nullabilityDiagnosticsBuilderOpt.Add(new TypeParameterDiagnosticInfo(typeParameter, new UseSiteInfo<AssemblySymbol>(new CSDiagnosticInfo(ErrorCode.WRN_NullabilityMismatchInTypeParameterReferenceTypeConstraint, containingSymbol.ConstructedFrom(), typeParameter, typeArgument))));

and if I understand correctly, we can't get simplify anything here. Am I correct?

[AlekseyTs]

and if I understand correctly, we can't get simplify anything here. Am I correct?

Are all use-site diagnostics on symbols errors now? As in VB.

I am primarily referring to the following APIs on Symbol:

        /// <summary>
        /// True if the symbol has a use-site diagnostic with error severity.
        /// </summary>
        internal bool HasUseSiteError
        {
            get
            {
                var info = GetUseSiteInfo();
                return info.DiagnosticInfo?.Severity == DiagnosticSeverity.Error;
            }
        }

        /// <summary>
        /// Returns diagnostic info that should be reported at the use site of the symbol, or default if there is none.
        /// </summary>
        internal virtual UseSiteInfo<AssemblySymbol> GetUseSiteInfo()
        {
            return default;
        }

By contrast VB has only:

        ''' <summary>
        ''' Returns dependencies and an error info for an error, if any, that should be reported at the use site of the symbol.
        ''' </summary>
        Friend Overridable Function GetUseSiteInfo() As UseSiteInfo(Of AssemblySymbol)
            Return Nothing
        End Function

If so, we probably should rename all relevant APIs that use "UseSiteDiagnostic" to "UseSiteError" to reflect the fact.

I was referring to legacy names for the APIs. So, I withdraw this suggestion.

Some checks for use-site errors (vs. warnings), if any, could be simplified as well.

Given the example where nullable warnings are added to the UseSiteInfo, we cannot make a general assumption that UseSiteInfo doesn't contain a warning. But it might be a valid assumption when we get it from symbol.

Also, we should see if calculation/merging of use-site diagnostics could be optimized/simplified (get out early, etc.) now.

I am primarily referring to the way we calculate value returned by Symbol.GetUseSiteInfo API. It is quite possible that there is nothing to optimize there, but it is worth checking, I think.

[AlekseyTs]

VerifyDiagnostics

It doesn't feel like simply removing the verification of diagnostics is appropriate. I think at least we should verify that there is no diagnostics.

[AlekseyTs]

VerifyDiagnostics

According to the comment above, it looks like the goal of the test was to verify that: "higher version should be preferred over lower version regardless of the order of the references". It also looks like the fact was verified indirectly by checking diagnostics. If we no longer can rely on this approach, we should find a different way to verify the outcome. It feels like we might want other tests that primarily were verifying presence of the warnings (at least all tests in this file) be adjusted in a similar way.

[jcouv]

Marking PR as draft for now, to make room in our review queue. Please undraft when ready for another look. Thanks