Fixing NullReferenceException inside ConvertToInterpolatedString#25225
Fixing NullReferenceException inside ConvertToInterpolatedString#25225DustinCampbell merged 2 commits intodotnet:masterfrom
Conversation
|
@dotnet-bot retest windows_release_vs-integration_prtest please |
|
Note: it's disabled because dynamic could have a custom + operator (it likely won't, but this is probably not worth investing in as most people don't use dynamic - and if they do, they already have to get used to IDE features not working). |
A good approach might actually be to offer the refactoring but show a warning. This is only a refactoring so false positives wouldn't be an issue. |
| method.ContainingType.SpecialType == SpecialType.System_String && | ||
| return semanticModel.GetSymbolInfo(expression, cancellationToken).Symbol is IMethodSymbol method && | ||
| method.MethodKind == MethodKind.BuiltinOperator && | ||
| method.ContainingType?.SpecialType == SpecialType.System_String && |
There was a problem hiding this comment.
@sharwell @jasonmalinowski This is rather interesting. We have a MethodSymbol without a null ContainingType. I'm pretty certain there will be a lot of codepaths in the IDE that will not be resilient to this sort of thing. The presumption was that fields/props/events/methods would all be contained in types (possibly the implicit script class for scripts).
There was a problem hiding this comment.
I'll take a look, that does seem wrong.
What's the expression used in semanticModel.GetSymbolInfo in this example?
Filed #25377
There was a problem hiding this comment.
it's a binary + expression involving dynamic
There was a problem hiding this comment.
Perhaps the compiler synthesizes methods for dynamic invocations? But then doesn't have an INamedTypeSymbol to place them on (because IDynamicTypeSymbol isn't an INamedTypeSymbol). (Just spitballing).
However, for this case, it would be nice if we could synthesize a type. Similar to how top level members in script code belong to an implicit named type representing the script class.
There was a problem hiding this comment.
Thanks for contributing this fix @Neme12. I'm going to ahead and merge this, regardless of the potential for a fix on the compiler side. I agree with @CyrusNajmabadi that ContainingType shouldn't be null. If the compiler fixes it, this null-check will just become a bit paranoid.
fixes #20943