roslyn: IOperation missing IsExpression / IsStatement
Version Used: master
Recently IsExpression and IsStatement were removed from IOperation in https://github.com/dotnet/roslyn/commit/a289e370526a0051e83f8c75a6e1844d34b3498e. I’ve been trying to upgrade my existing analyzer pack, which contains about 40 analyzers, half of them using IOperation.
I have not yet found a way to fix an existing analyzer that counts the number of statements in methods. A few weeks ago I was struggling with IThrowExpression (a replacement/merge of IThrowStatement), but had to revert back to syntax to differentiate between expression and statement form. Now that the distinction no longer exists, I would need to revert back to syntax for all possible statements in the language.
Another case is an analyzer that checks for block scope in if statements, where I have do deal with the merge to IConditionalOperation. The expression form should be filtered out by this analyzer.
So my request is to bring back IsExpression and IsStatement in some form in the 15.5 timeframe. If that’s not possible, any advise on how to proceed would be welcome.
Tagging @mavasani
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Comments: 26 (26 by maintainers)
Thanks @bkoelman for digging through this! I will take a look at your implementation in detail later today.
Regardless, given the non-trivial nature of this detection logic, I believe we should expose
IOperation.IsExpressionandIOperation.IsStatementproperties in Roslyn. They are trivial to implement in our operation factory/generator and trivial to validate in the test operation dumper for each operation node based on syntax checks.THanks for the response. My feedback to the individual points.
Looking at teh use cases… i’m not sure why you need this. I mean, your code already goes back to syntax depending on if things are a statement or not. i.e.: https://github.com/bkoelman/CSharpGuidelinesAnalyzer/blob/97c8bca9201bd8c57da2b3525ec3b2f8427103b5/src/CSharpGuidelinesAnalyzer/CSharpGuidelinesAnalyzer/Extensions/OperationExtensions.cs#L220-L224
If you’re going to use the syntax anyways, then why not just do: “operation.Syntax is StatementSyntax”?
Similarly, i’m very wary of use cases like this: AvoidMemberWithManyStatementsAnalyzer
Basically, that code would could an if-statement as a statement and count that against the user. But if the user instead used a
conditional ?:expression, it wouldn’t count against them. But certainly each are just as complex as they both involve branching and whatnot.Indeed, it seems like a better way to determine complexity would be to precisely stick with the operations, and not the weak statement/expression analysis. That way the code correctly understand that my code is still as complex, even if i do a bunch of cute things to build a huge expression instead of having a bunch of individual statements.
As before, i’m actually more skeptical that this is valuable. The idea is to move people away from the language’s idea of ‘statements/expressions’. If you want that, i would actually go back to the syntax as that really is going to be accurate and in line with the individual languages.
–
Note: i’m not hugely against this. As these could just be props that could change independently, and could be different per language, or per context. However, it feels oogy to me, and i’m def at the point where i do question if it’s the right thing to do.
THanks!