Fix analyzer failure where incorrect SemanticModel is used #91764
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @hoyosjs Issue DetailsGo get the appropriate semantic model when we happen to end up with a different SyntaxTree for the implementation of a property we are recursively analyzing. This is quite slow, but isn't very common. Fixes #90356
|
| @@ -293,6 +293,12 @@ private static INamedTypeSymbol[][] DecomposePropertySymbolForIsSupportedGroups_ | |||
| if (propertyDefiningSyntax is PropertyDeclarationSyntax propertyDeclaration | |||
| && propertyDeclaration.ExpressionBody is ArrowExpressionClauseSyntax arrowExpression) | |||
| { | |||
| if (model.SyntaxTree != arrowExpression.SyntaxTree) | |||
There was a problem hiding this comment.
I think this problem is actually caused by the DeclaringSyntaxReferences[0] call above. I think the only way you can get into this situation (property syntax is in a different syntax tree) is if you have a partial property with multiple definitions. When you grab [0] you might not be looking at the right one. I think you may want to iterate through the syntax references and match up the appropriate tree.
This broadly assumes that I know what this method is doing.
Can you broadly describe what's going on here?
There was a problem hiding this comment.
So, what this code is doing is finding a statement like...
if (SomeProperty)
{
Sse3.DoSomethingIntrinsic
}
and trying to check to see if SomeProperty is implemented by a property accessor like...
public bool SomeProperty => Sse3.IsSupported;
So, what we're doing here is a bit of interprocedural analysis. When this analyzer was written, it so happened that just using whatever SemanticModel we had lying around worked just fine, but at some point this started detecting that the SemanticModel wasn't associated with the syntax tree which implemented the property. Given that we only get to this logic when we've already found an ArrowExpressionClauseSyntax, I don't think this is a partial property problem, but perhaps there was some refactoring so that we had a new case where the property was implemented via a different SyntaxTree. (I don't know why we have multiple syntax trees, in Roslyn, I expect you know more about this than I do.)
There was a problem hiding this comment.
I see, so the problem is that you're trying to jump from definition to definition, and using the same semantic model, which may not be attached to the same tree.
In that case, this can work, but inter-procedural analaysis in Roslyn is always pretty fishy. It kind of sounds like you want something like the linker/AOT capability analyzer -- SomeProperty provides some capability, and only inside the check is that capability valid.
We haven't yet generalized this analysis to define arbitrary capabilities and checks, but this is on the table for .NET 9. + @sbomer
So, with this existing code... I think it's fine. Nothing jumps out at me as fundamentally wrong with this approach. However, only looking at the first Declaration is still a bug. Properties can be partial, meaning they can have multiple declarations, and you need to check them all. You should enumerate each one and run this logic for all of them. Also, there's the special-casing of ArrowExpression which is nasty, but that's a whole separate problem (you probably want the IOperation tree instead of the syntax tree).
There was a problem hiding this comment.
Yeah, I'm not saying any of this analysis is pretty... or clean. Just that it works for System.Private.CoreLib, which is all that is necessary (This is a System.Private.CoreLib specific analyzer). I don't see what there is to fix around the ArrowExpression and only the first declaration. Could you provide an example of how we could have a property with multiple definitions which is implemented via an arrow expression? I'm not aware of any way to write that. The logic here requires all relevant properties to be implemented via arrow expressions, just to simplify the analysis.
There was a problem hiding this comment.
provide an example
Sorry, I assumed dotnet/csharplang#6420 was going to make it into C# 12, but it looks like it didn't make the cut. So partial properties are not yet legal.
If the feature does make it into a future release, the code would look like https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMBGAsAKHQGYACABwgCcAXKCAGxKxIGESBvAk7p0imuo2AB7YYwCyATwAKlYWQCmNSRxIBzBdQDcJAL5cexJgBYS4gBQBKDge7789gkf60GTTKxv4evclVdCohIycorKJAC8AHwk1JQArgpaBLpAA
You could potentially use the for loop for future proofing, but it won't make a difference right now.
Go get the appropriate semantic model when we happen to end up with a different SyntaxTree for the implementation of a property we are recursively analyzing. This is quite slow, but isn't very common.
Fixes #90356