Don't offer to use patterns when they won't work#60370
Don't offer to use patterns when they won't work#60370davidwengier merged 6 commits intodotnet:mainfrom
Conversation
src/Analyzers/CSharp/Analyzers/UsePatternCombinators/AnalyzedPattern.cs
Outdated
Show resolved
Hide resolved
Youssef1313
left a comment
There was a problem hiding this comment.
Is using SpeculativeSemanticModel better for this scenario? That way we'll be referring to the compiler back instead of trying to mimmic its logic.
I believe @CyrusNajmabadi has used speculation for the unnecessary cast feature, and it worked reallyy great.
...tures/CSharpTest/UsePatternCombinators/CSharpUsePatternCombinatorsDiagnosticAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
...tures/CSharpTest/UsePatternCombinators/CSharpUsePatternCombinatorsDiagnosticAnalyzerTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
I have another idea. Why not keep the analyzer the same, ie, report a diagnostic, and fix the CodeFixProvider to generate a valid code. It's always possible to refer to the type using a fully qualified name with global:: I think?
We can always produce the node with global:: + fully qualified name, then add WithAdditionalAnnotations(Simplifier.Annotation); which should simplify the name if possible.
|
Might just be me, but I wouldn't like a fix that uses I'm not sure this warrants the complexity. |
There was a problem hiding this comment.
Does it work for the following?
public class Goo
{
private struct X { }
private struct Y { }
private void M(object o)
{
const int X = 1;
const int Y = 2;
// Generics are also to be considered.
if (o is X? || o is Y?) // error if converted to o is X? or Y?
{
}
}
}I think there can be other edge cases we cannot see easily (the above may not be included, it could be currently working). That's why I think referring back to SpeculativeSemanticModel is the most accurate fix.
| var names = operation.SemanticModel.LookupSymbols(typeSyntax.SpanStart, name: GetLeftmostName(typeSyntax)); | ||
| if (names.Any(t => t is not INamespaceOrTypeSymbol)) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Edge case:
var @int = 1;
var @long = 2;
if (o is int || o is long)
{
}I think we will not produce a diagnostic while it's valid to convert this to or?
NOTE: The edge cases I'm trying to come up with doesn't mean I feel strongly they should be handled. I'm just pointing to them as they come to mind so that you can weigh on whether they should be handled or not, or add tests for them if they already work and I'm thinking they don't work.
I did try using that, but it didn't work. I've never used it before though so could very well be something I did wrong. |
|
@davidwengier This has worked for me: 3a708f6 (built on top of your branch) The added test was failing before 3a708f6. |
|
Thank you for the education @Youssef1313, I was way off!! |
|
@mychang666 Did you accidentally submit your comment early? |
|
Ping @dotnet/roslyn-ide and/or @CyrusNajmabadi for review |
| { | ||
| return null; | ||
| } | ||
| } |
|
Thanks for your help @Youssef1313 |
Fixes #57199