Repository navigation
Handle cref type parameters in symbol lookup - #85840
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused compiler fix matches existing cref semantics and has strong regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Prevents cref completion crashes by excluding unsupported reduced-extension lookup for cref type parameters.
Changes:
- Guard cref type parameters during symbol lookup.
- Add compiler and completion regression coverage.
| File | Description |
|---|---|
CSharpSemanticModel.cs |
Skips reduced-extension lookup for cref type parameters. |
CrefTests.cs |
Tests classic and block extension lookup behavior. |
CrefCompletionProviderTests.cs |
Verifies completion descriptions no longer crash. |
|
whoops forgot to tag the issue in my PR: #85817 but I like your fix better |
| if ((object)container == null || container.Kind == SymbolKind.Namespace) | ||
| // Cref type parameters do not support the constraint checks required to reduce extensions. | ||
| if ((object)container == null || container.Kind == SymbolKind.Namespace || | ||
| container is TypeParameterSymbol { TypeParameterKind: TypeParameterKind.Cref }) |
There was a problem hiding this comment.
container is TypeParameterSymbol { TypeParameterKind: TypeParameterKind.Cref }
The change prevents a crash for the tested scenario. But I think that it is better to implement unimplemented APIs on CrefTypeParameterSymbol. For example, return ErrorTypeSymbol.UnknownResultType; works for the scenario.
Also, we can end up here with a generic type symbol constructed with such type parameter as a type argument. And quite possibly an attempt might be made to use one of the "unimplemented" APIs while classifying conversion for that type. For example, the following unit-test does:
[Fact]
[WorkItem("[https://github.com/dotnet/roslyn/issues/85788](https://github.com/dotnet/roslyn/issues/85788)")]
public void CrefTypeParameterExtensionMemberLookup1()
{
var source = """
interface I<out T>
{
public I<T> Copy1();
}
/// <see cref="I{U}.Copy1()"/>
class D { }
""";
var extensions = """
static class Extensions
{
extension(I<D> value)
{
public int Copy2() => 0;
}
}
""";
var compilation = CreateCompilation([source, extensions], parseOptions: TestOptions.RegularWithDocumentationComments);
compilation.VerifyEmitDiagnostics();
var cref = GetCrefSyntaxes(compilation).Single();
var model = compilation.GetSemanticModel(cref.SyntaxTree);
var method = (IMethodSymbol)model.GetSymbolInfo(cref).Symbol;
var type = (INamedTypeSymbol)method.ReturnType;
Assert.Equal(TypeParameterKind.Cref, ((ITypeParameterSymbol)(type.TypeArguments[0])).TypeParameterKind);
Assert.Empty(model.LookupSymbols(cref.SpanStart, type, "Copy2", includeReducedExtensionMethods: true));
}
There is no crash for this scenario, but there is an attempt to classify conversion for cref type parameter.
Bottom line, I think it will be much better to implement the APIs in some reasonable way. #Closed
|
Done with review pass (commit 1) |
# By Sam Harwell (331) and others # Via GitHub (414) and others * upstream/main: (1352 commits) Document known issue workarounds inline (#85896) Restore automatic HTML test results for local runs (#85892) Report actual errors when strong-name HRESULT assertions fail (#85894) Restore main insertions after the VS 18.12 snap (#85824) Add "edit criteria" for AI doc updates (#85893) Handle cref type parameters in symbol lookup (#85840) Fix build warnings (#85754) Improve our Copilot instructions for testing [main] Update dependencies from dotnet/arcade (#85885) Rewrite 'router' phrasing Address review feedback Add option to disable cref paste fix Prefer repository terminology in generated output (#85878) Use cref for workspace extension handler Remove unnecessary static diagnostic registration Unsafe evolution: use safety documentation for unnecessary unsafe (#85842) Add Razor compiler team to code owners Unsafe evolution: avoid constructor recursion (#85773) Add roslyn-ide to Razor code owners Fix unexpected System.ArrayTypeMismatchException for captured method call receivers (#85811) ... Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fd482dbd-c828-466b-87c6-6b2103be2089 # Conflicts: # src/Compilers/CSharp/Portable/Errors/ErrorCode.cs # src/Compilers/CSharp/Portable/Errors/MessageID.cs
Fixes #85788.
Microsoft Reviewers: Open in CodeFlow