Create AddNonNullTypes code fixer by jcouv · Pull Request #30098 · dotnet/roslyn

jcouv · 2018-09-22T05:16:58Z

This is a first take. The goal is to give users a convenient path into nullable feature (start using nullable-related syntax, we offer to turn the feature on).

Some things that are not handled yet:

if you already have [NonNullTypes(false)] then we should flip it to true instead
we should also offer to add [module: NonNullTypes] (but we'll need to decide which file to put it in, or whether to inject it via msbuild/GenerateAssemblyInfo instead)
should we add using System.Runtime.CompilerServices; instead?

(Filed follow-up issue #30099)

Therzok · 2018-09-22T17:18:22Z

src/Features/CSharp/Portable/CSharpFeaturesResources.resx

Typo'ed NonNull #Resolved

Thanks! Fixed #Resolved

CyrusNajmabadi · 2018-09-22T18:19:31Z

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs

why not? #Closed

I don't think that's very useful. Most likely you should use a module-level attribute instead for such cases.
Also, I suspect it's not trivial to implement properly.
The cost/benefit combination makes it low priority at the moment (we're trying to address blockers before preview 1). #Closed

I'll leave it to you to decide the right design. However:

Also, I suspect it's not trivial to implement properly.

This should be very trivial. Basically, you derive from SyntaxEditorBasedCodeFixProvider, and implement FixAll. Your fixall will wlak all your diagnostics and get the unique set of TypeDecls to fix. Then it will just walk all of those and add attributes like you do today.

#Closed

Yes, that'd be the core, but what about FixAll across documents (partial type scenarios)?
SyntaxEditorBasedCodeFixProvider seems to work on a document basis. #Closed

Hrmm... i'm a bit skeptical that that's an important enough scenario to care about. It seems fine to just place the attribute on the differnet parts :)

That said, if this is important enogh for you, an easy fix is to simply pass along the FixAllState object (or the FixAllScope) with SyntaxEditorBasedFixAllProvider.FixAllAsync. Then, if it's not 'document' you simply only fix the TypeDecl if it is considered the 'first part' of the partial type. #Closed

If we went with the brute force approach, the fixer would produce errors in cases involving partial types (the attribute can't be specified multiple times).

If we try a more tricky approach, I'm concerned that we would produce fewer attributes than required to fix the warnings: the scenario is that you have a partial class where the 'first part' doesn't involve any nullable notations (thus no warnings), and some other part does.
Maybe there is a way to share state across documents when doing such a FixAll? That would solve the problem.

Just to give options labels:

no FixAll

brute-force FixAll (per document, handle type nesting)

partial-aware FixAll (prefer 'first part', per document, handle type nesting)

cross-document FixAll #Closed

Or just fix this part:

(the attribute can't be specified multiple times).

I personally get super frustrated with these types of attributes because now i have to deal with that :) #Closed

the scenario is that you have a partial class where the 'first part' doesn't involve any nullable notations (thus no warnings), and some other part does.

Gotcha. An interesting case to be sure. It does feel like we could expose a more flexibly entrypoint in SyntaxEditorBasedCodeFixProvider. One where you got to handle all the diagnostics at once, instead of us handling each document one at a time, having you only fixup that document. #Closed

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs

CyrusNajmabadi · 2018-09-22T18:20:03Z

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs

this keeps syntax trees alive. preferable to not do that. #Closed

Fixed. Also fixed HideBase which I had modeled after. #Closed

jcouv · 2018-09-24T21:35:23Z

@dotnet/roslyn-ide for review. Thanks #Resolved

jcouv · 2018-09-26T16:52:21Z

@dotnet/roslyn-ide for review. Thanks #Resolved

jcouv · 2018-09-28T14:32:55Z

@dotnet/roslyn-ide for review (simple fixer). Thanks #Resolved

jasonmalinowski

Looks like there's some missing tests and one possible functional bug. Great start though.

jasonmalinowski · 2018-09-28T20:15:58Z

src/EditorFeatures/CSharpTest/Diagnostics/AddNonNullTypes/AddNonNullTypesTests.cs

So this is a bug then? It seems we should have this assert the correct behavior and then skip the test with a Skip attribute, rather than assert "wrong" behavior? #Resolved

Skipping tests risks that we forget unskipping.
The comment is there to clarify that the expectation is "wrong". #Resolved

Skipped tests that aren't unskipped we can tool to catch automatically, this we can't. Please just use a skip and have it assert the right thing. #Resolved

The "right thing" is not yet decided, as explained in the comment and the linked issue. Two options:

not trigger

toggle to true

I don't think this PR should depend on figuring out the design for some future work.

jasonmalinowski · 2018-09-28T20:16:40Z

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs

"NonNullTYpes" has a typo. #Resolved

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs

jasonmalinowski · 2018-09-28T20:21:25Z

src/Features/CSharp/Portable/CodeFixes/HideBase/HideBaseCodeFixProvider.cs

Should be indented a bit farther. #Resolved

jcouv · 2018-10-02T23:55:26Z

@jasonmalinowski All feedback addressed, except the request to skip a test (because the "right" behavior is pending some design). #Resolved

CyrusNajmabadi · 2018-10-03T01:23:25Z

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs

+            = ImmutableArray.Create(CS8632, CS8629);
+
+        public CSharpAddNonNullTypesCodeFixProvider()
+            : base(supportsFixAll: false)


CyrusNajmabadi · 2018-10-03T01:24:27Z

src/Features/CSharp/Portable/CSharpFeaturesResources.resx

    <value>Hide base member</value>
  </data>
+  <data name="Add_NonNullTypes_attribute" xml:space="preserve">
+    <value>Add NonNullTypes attribute</value>


Nit: would probably look better as Add [NonNullTypes] attribute #Pending

CyrusNajmabadi · 2018-10-03T01:24:46Z

src/Features/CSharp/Portable/CodeFixes/HideBase/HideBaseCodeFixProvider.cs

+using System.Threading;
+using System;
+using Microsoft.CodeAnalysis.CodeActions;
+using Microsoft.CodeAnalysis.Editing;


ordering. #Pending

CyrusNajmabadi · 2018-10-03T01:25:06Z

src/Features/CSharp/Portable/CodeFixes/HideBase/HideBaseCodeFixProvider.cs

+        private class MyCodeAction : CodeAction.DocumentChangeAction
+        {
+            public MyCodeAction(Func<CancellationToken, Task<Document>> createChangedDocument)
+                : base(CSharpFeaturesResources.Hide_base_member, createChangedDocument)


dumb question... why was this rolled into this PR? #Resolved

Earlier in this PR, you pointed out bad pattern (holding onto objects) which I had picked up from that fixer. #Resolved

CyrusNajmabadi · 2018-10-03T01:26:50Z

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs

+            if (containingType == null)
+            {
+                return;
+            }


this is somewhat busted in that we have already registered the fix, but now it doesn't do anything in this case. Unless we're guaranteeing that this diagnostic will always be reported inside a type-decl, then we need to change tings slightly. specifically, the RegisterCodeFixesAsync method needs to check if we're in a containing-type first, before registering teh fix.

CyrusNajmabadi · 2018-10-03T01:27:36Z

src/Features/CSharp/Portable/CodeFixes/HideBase/HideBaseCodeFixProvider.cs

@@ -42,7 +51,25 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
                return;


same issue here. but this seems to be a different feature. so it's buggy that feature was written that way.

jcouv · 2018-10-05T18:41:51Z

I'm going to hold-off this PR, as we're going to switch to a directive method to turn the feature on.

CyrusNajmabadi · 2018-10-05T18:52:21Z

Directive approach seems far saner. does this mean no concerns about attribute circularity?

jcouv · 2018-10-05T18:57:40Z

@CyrusNajmabadi Yes. Cycles are a key motivation for switching to directive.
Also, there is the scenario of controlling nullability in aliases: using alias = List<string>;

CyrusNajmabadi · 2018-10-05T19:01:11Z

Loveit!

jcouv · 2019-02-23T01:28:00Z

Closing because we don't use the NonNullTypes attribute any longer. We use #nullable enable or a project-level setting. I'll add a fixer once the project-level setting is available via project-system.

jcouv added Area-IDE PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. Feature - Nullable Reference Types Nullable Reference Types labels Sep 22, 2018

jcouv added this to the 16.0 milestone Sep 22, 2018

jcouv self-assigned this Sep 22, 2018

jcouv requested a review from a team as a code owner September 22, 2018 05:16

jcouv force-pushed the add-nnt branch 2 times, most recently from ef2dcad to 0d8d37e Compare September 22, 2018 15:45

Therzok reviewed Sep 22, 2018

View reviewed changes

CyrusNajmabadi reviewed Sep 22, 2018

View reviewed changes

src/Features/CSharp/Portable/AddNonNullTypes/CSharpAddNonNullTypesCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 22, 2018

View reviewed changes

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 22, 2018

jinujoseph assigned dpoeschl Sep 28, 2018

jinujoseph added the Feature Request label Sep 28, 2018

jasonmalinowski reviewed Sep 28, 2018

View reviewed changes

jasonmalinowski approved these changes Oct 2, 2018

View reviewed changes

jcouv and others added 7 commits October 2, 2018 18:01

Code fix to add NonNullTypes attribute

1e2cbf7

Also trigger on suppression operator

3773fbc

Fix typo

7778071

Don't hold onto syntax tree

9240004

HideBase shouldn't hold onto SyntaxTree

705f90a

Address PR feedback from Jason

ada629b

Updating xlf files

8dee1e1

jcouv force-pushed the add-nnt branch from 90c19e0 to 8dee1e1 Compare October 3, 2018 01:12

CyrusNajmabadi reviewed Oct 3, 2018

View reviewed changes

jcouv added Blocked PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Oct 5, 2018

cston mentioned this pull request Oct 12, 2018

[Work items] Nullable reference types #22152

Closed

jcouv closed this Feb 23, 2019

		@@ -42,7 +51,25 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
		return;

Conversation

jcouv commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Therzok Sep 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 22, 2018 • 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!

CyrusNajmabadi Sep 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 22, 2018 • 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!

CyrusNajmabadi Sep 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 22, 2018 • 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!

CyrusNajmabadi Sep 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 24, 2018 • 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!

CyrusNajmabadi Sep 24, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Sep 24, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi Sep 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 22, 2018 • 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!

jcouv commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Sep 28, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 29, 2018 • 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!

jasonmalinowski Oct 1, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv commented Sep 22, 2018 •

edited

Loading

Therzok Sep 22, 2018 •

edited by jcouv

Loading

jcouv Sep 22, 2018 •

edited

Loading

CyrusNajmabadi Sep 22, 2018 •

edited by jcouv

Loading

jcouv Sep 22, 2018 •

edited

Loading

CyrusNajmabadi Sep 22, 2018 •

edited by jcouv

Loading

jcouv Sep 22, 2018 •

edited

Loading

CyrusNajmabadi Sep 22, 2018 •

edited by jcouv

Loading

jcouv Sep 24, 2018 •

edited

Loading

CyrusNajmabadi Sep 24, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Sep 24, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Sep 22, 2018 •

edited by jcouv

Loading

jcouv Sep 22, 2018 •

edited

Loading

jcouv commented Sep 24, 2018 •

edited

Loading

jcouv commented Sep 26, 2018 •

edited

Loading

jcouv commented Sep 28, 2018 •

edited

Loading

jasonmalinowski Sep 28, 2018 •

edited by jcouv

Loading

jcouv Sep 29, 2018 •

edited

Loading

jasonmalinowski Oct 1, 2018 •

edited by jcouv

Loading

jasonmalinowski Sep 28, 2018 •

edited by jcouv

Loading

jasonmalinowski Sep 28, 2018 •

edited by jcouv

Loading

jcouv commented Oct 2, 2018 •

edited

Loading

CyrusNajmabadi Oct 3, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Oct 3, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Oct 3, 2018 •

edited by jcouv

Loading

jcouv Oct 3, 2018 •

edited

Loading