Teach pattern-matching analyzer to detect and fix additional cases by alrz · Pull Request #19907 · dotnet/roslyn

alrz · 2017-05-31T19:16:26Z

Fixes #17744

null check in while in addition to if
inline type check: if ((x = o as T) != null)

/cc @CyrusNajmabadi

Pilchie · 2017-06-05T16:15:25Z

Thanks for the contribution - tagging @dotnet/roslyn-ide for review.

CyrusNajmabadi · 2017-06-05T18:55:46Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

CyrusNajmabadi · 2017-06-05T18:57:39Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

CyrusNajmabadi · 2017-06-05T18:59:06Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

Should be TryFind since this can fail. Maybe say "TryFind..InContainingBlock" as well. That would help explain why we need to hand write this instead of just using the ILocalSymbol and getting the declaration from that.

CyrusNajmabadi · 2017-06-05T18:59:29Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckCodeFixProvider.cs

How can this happen?

It can't, just to satisfy the compiler. is there an ExceptionUtilities.Unreachable accessible to use?

Yes. It's called "ExceptionUtilities.Unreachable" :D

I think that's not accessible in feature project. If not could I add it?

It should be accessible. We link that file into the Workspaces layer IIRC. Let me check.

It shoudl be available:

CyrusNajmabadi · 2017-06-05T18:59:48Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckCodeFixProvider.cs

i woudl personally prefer ifOrWhileStatement

CyrusNajmabadi · 2017-06-05T19:00:44Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

ifOrWhileStatement

CyrusNajmabadi · 2017-06-05T19:01:35Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

it might be nice to keep this around, so other calls you make could access this directly. your call.

CyrusNajmabadi · 2017-06-05T19:03:46Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

shoudl be declarationStatement. "declaration" makes me think it has type VariableDeclaration.

CyrusNajmabadi · 2017-06-05T19:04:45Z

src/EditorFeatures/CSharpTest/UsePatternMatching/CSharpAsAndNullCheckTests.cs

We'll need to figure out if we need a new option here. Or if we need to update the option we show to the user here. I think it's ok to have a single option, but we likely need to call out these cases in the preview. What do you think @Pilchie ?

I personally don't think we need a new option here, since this isn't available on the command line yet.

@kuhlenh how do you feel about the preview question?

CyrusNajmabadi · 2017-06-05T19:06:03Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

It's unclear to me why so much logic changed in this method. Can you explain what your new code does that's different from teh old way of checking def assignment? Thanks!

two reasons: in the old code if assigned was false we didn't run the loop body, so we could break out of loop early. and the other reason is that, unlike if, pattern var is scoped only inside while block.

CyrusNajmabadi · 2017-06-05T19:07:18Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

declarationStatement.

CyrusNajmabadi · 2017-06-05T19:07:31Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

Needs comments.

CyrusNajmabadi · 2017-06-05T19:07:57Z

This is awesome. Mostly nits. Haven't reviewed the def assignment stuff yet. Waiting on you for explanation first.

alrz · 2017-06-05T19:23:08Z

Other cases I want this to handle are:

non-leftmost null checks

var x = e as T;
if (condition && x != null && x.Foo)
// gives
if (condition && e is T x && x.Foo)

negative null checks

var x = e as T;
if (x == null) return;
// gives
if (!(e is T x)) return;

null check in ternary

var x = e as T;
return x != null ? x.Foo : ...;
// gives
return e is T x ? x.Foo : ...;

These all should be able to mix e.g. ternary+negative+inline, while+positive+non-leftmost etc.

Before I do anything about it (probably in another PR?) I'd like to hear your thoughts on those. Thanks.

CyrusNajmabadi · 2017-06-05T19:23:48Z

Can you give examples of each of those?

CyrusNajmabadi · 2017-06-05T19:24:31Z

The more i think about this, the more i'm ok with all of this. I think the general feature we're offering is "use the new 'is Type T' pattern matching construct". So wherever reasonable we can see something that can translate into that pattern, we should offer it.

CyrusNajmabadi · 2017-06-05T19:27:23Z

(and yes, another PR would be good)

alrz · 2017-06-05T19:31:06Z

Updated my previous comment to include some examples.

CyrusNajmabadi · 2017-06-05T19:34:20Z

non-leftmost null checks

Will this work with definite assignment?

CyrusNajmabadi · 2017-06-05T19:35:05Z

negative null checks

This is iffy. Because some people don't like the if (!( form. We may need a separate option for this. "Use negative pattern matching".

Note: i would personally like this.

CyrusNajmabadi · 2017-06-05T19:35:52Z

null check in ternary

If it's just "return ternary" then yes please.

CyrusNajmabadi · 2017-06-05T19:36:37Z

Seperate PR for those though. ideally one PR per new addition.

Pilchie · 2017-06-28T00:31:24Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

Nit: Generally would prefer all comments to be "Sentence case."

CyrusNajmabadi · 2017-06-28T02:32:07Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

bad indentation.

CyrusNajmabadi · 2017-06-28T02:32:57Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

Tagging @mavasani . It seems unfortunate that we will get tons of callbacks for a file and we'll have to do this check and bail out for every node we get called back on. It would be really nice if we could have some sort of conditional way to say "don't even call into us for this syntax tree."

CyrusNajmabadi · 2017-06-28T02:33:34Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

@mavasani RegisterSyntaxNodeAction could take a SyntaxTree predicate that we could potentially use to avoid calling this action if the SyntaxTree didn't match.

CyrusNajmabadi · 2017-06-28T02:35:25Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

fyi, i'm a bit on the fence about this. In general we'd like to do some quick up front checks that don't involve expensive typechecks. That way we can be as low overhead as possible since we're going to be called on every if-statement in the world. so doing IsParentKind(Block) (and doing all the other syntactic checks first) before doing type-tests is generally preferred.

I agree with you but I think it's not just this instance, for example OfType on all nodes to find identifiers or local declarations should be updated as well.

I propose we add bool IsKind<T>(this SyntaxNode, SyntaxKind, out T) where T : SyntaxNode.

I wish there was a way to get the corresponding SyntaxKind from the syntax node type

CyrusNajmabadi · 2017-06-28T02:37:11Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

assignment.

try to wrap at 100col for comments.

CyrusNajmabadi · 2017-06-28T02:37:51Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

might want to comment/give-example why this is ok.

This is exactly what the old code does (though not so obviously).

Nevertheless, I think CheckDefnitiveAssignment is oversimplified. what if the statement is branched, etc? Could we use control flow analysis instead?

CyrusNajmabadi · 2017-06-28T02:38:29Z

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

it's not clear to me why this is appropriate. do you have an example?

It's not. when we return true here we won't suggest the fix.

alrz · 2017-07-08T20:22:16Z

@CyrusNajmabadi I've addressed your comments, Had a few questions. Can you please take a look? thanks.

CyrusNajmabadi

I like the new extension. If you're feeling up to it, feel free to update the rest of our code to use it :)

alrz · 2017-07-10T03:29:24Z

We could even have overloads for node types then you dont need to pass the syntax kind,

static bool Is(this SyntaxNode node, out LocalVarDeclStatement result)
// etc

if(node.Is(out LocalVarDeclStatement decl))

We could have it for abstract classes as well. Maybe the syntax generator could produce these helpers.

Is that too many overloads?

CyrusNajmabadi · 2017-07-10T04:00:35Z

Can't tell if serious. How would that overload work?

alrz · 2017-07-10T04:22:13Z

The generated code would look like this:

static bool Is(this SyntaxNode node, out LocalDeclarationStatementSyntax result)
  => node.IsKind(SyntaxKind.LocalDeclarationStatement, out result);

static bool Is(this SyntaxNode node, out EnumDeclarationSyntax result)
  => node.IsKind(SyntaxKind.EnumDeclaration, out result);

I'm not sure if we should do this for abstract classes? e.g.

static bool Is(this SyntaxNode node, out TypeDeclarationSyntax result) {
  => node.IsKind(ClassDeclaration, InterfaceDeclaration, StructDeclaration, out result);

CyrusNajmabadi · 2017-07-10T15:40:04Z

Ah, i see. I missed the part where this would be autogenerated. I would be fine with you making such a helper. But not right now :)

alrz · 2017-07-11T17:09:44Z

Ok I got it here in this gist, will open a PR at a later time.

Pilchie · 2017-07-25T00:07:47Z

@CyrusNajmabadi Anything more you are waiting on here? If not, any reason not to merge this?

CyrusNajmabadi · 2017-07-25T00:28:35Z

I've signed off :)

Pilchie · 2017-07-25T00:29:44Z

retest this please

Pilchie · 2017-07-25T00:29:58Z

Test results were fairly stale.

- null check in `while` in addition to `if` - inline type check e.g. `if ((o as T) != null)`

CyrusNajmabadi · 2017-07-25T18:33:51Z

Thanks!

dnfclas added the cla-already-signed label May 31, 2017

alrz force-pushed the smart-as branch from 08333bb to 92ea806 Compare May 31, 2017 19:36

alrz changed the title ~~Teach pattern matching analyzer to detect and fix additional cases~~ Teach pattern-matching analyzer to detect and fix additional cases May 31, 2017

alrz force-pushed the smart-as branch 2 times, most recently from 579bee5 to a067452 Compare June 1, 2017 10:52

Pilchie added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jun 5, 2017

CyrusNajmabadi reviewed Jun 5, 2017

View reviewed changes

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs Outdated

Copy link

Contributor

CyrusNajmabadi Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index++ :)

CyrusNajmabadi reviewed Jun 5, 2017

View reviewed changes

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs Outdated

Copy link

Contributor

CyrusNajmabadi Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index-- :)

CyrusNajmabadi reviewed Jun 5, 2017

View reviewed changes

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs Outdated

Copy link

Contributor

CyrusNajmabadi Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ifOrWhileStatement

CyrusNajmabadi reviewed Jun 5, 2017

View reviewed changes

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs Outdated

Copy link

Contributor

CyrusNajmabadi Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declarationStatement.

CyrusNajmabadi reviewed Jun 5, 2017

View reviewed changes

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs Outdated

Copy link

Contributor

CyrusNajmabadi Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs comments.

alrz mentioned this pull request Jun 27, 2017

Support negative null-checks when we offer a fix to use pattern-matching #20246

Merged

2 tasks

Pilchie approved these changes Jun 28, 2017

View reviewed changes

CyrusNajmabadi reviewed Jun 28, 2017

View reviewed changes

src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs Outdated

Copy link

Contributor

CyrusNajmabadi Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad indentation.

CyrusNajmabadi reviewed Jun 28, 2017

View reviewed changes

alrz force-pushed the smart-as branch from f817249 to cfee653 Compare June 28, 2017 17:54

shyamnamboodiripad changed the base branch from dev15.6 to master June 29, 2017 22:17

CyrusNajmabadi approved these changes Jul 10, 2017

View reviewed changes

Teach pattern-matching analyzer to detect and fix additional cases

380b223

- null check in `while` in addition to `if` - inline type check e.g. `if ((o as T) != null)`

alrz mentioned this pull request Jul 25, 2017

Suggestion to use pattern matching breaks compilation #21097

Closed

alrz force-pushed the smart-as branch from 74b2c78 to 380b223 Compare July 25, 2017 16:23

CyrusNajmabadi merged commit 9e68d4c into dotnet:master Jul 25, 2017

alrz deleted the smart-as branch July 25, 2017 19:26

sharwell added Community The pull request was submitted by a contributor who is not a Microsoft employee. and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 31, 2017

Conversation

alrz commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pilchie commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jun 5, 2017

Uh oh!

alrz commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jun 5, 2017

Uh oh!

CyrusNajmabadi commented Jun 5, 2017

Uh oh!

CyrusNajmabadi commented Jun 5, 2017

Uh oh!

alrz commented Jun 5, 2017

Uh oh!

CyrusNajmabadi commented Jun 5, 2017

Uh oh!

CyrusNajmabadi commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jun 5, 2017

Uh oh!

CyrusNajmabadi commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz commented May 31, 2017 •

edited

Loading

alrz Jun 5, 2017 •

edited

Loading

alrz commented Jun 5, 2017 •

edited

Loading

CyrusNajmabadi commented Jun 5, 2017 •

edited

Loading

alrz commented Jul 10, 2017 •

edited

Loading

alrz commented Jul 10, 2017 •

edited

Loading