Fix imports not being removed from MAS files. by CyrusNajmabadi · Pull Request #44230 · dotnet/roslyn

CyrusNajmabadi · 2020-05-13T22:06:21Z

Also does a reasonable effort of emitting #nullable enable/disable directives in MAS files when it contains signatures wiht types that are annotated/oblivious.

CyrusNajmabadi · 2020-05-13T22:06:38Z

tagging @dotnet/roslyn-ide

CyrusNajmabadi · 2020-05-13T22:09:24Z

src/Features/CSharp/Portable/MetadataAsSource/FormattingRule.cs

+using Microsoft.CodeAnalysis.Formatting.Rules;
+using Roslyn.Utilities;
+
+namespace Microsoft.CodeAnalysis.CSharp.MetadataAsSource


this is justa move.

CyrusNajmabadi · 2020-05-13T22:09:39Z

src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs

                new CSharpParenthesizedPatternReducer(),
                new CSharpDefaultExpressionReducer());

-        private class FormattingRule : AbstractMetadataFormattingRule


this type was moved to a sibling file.

src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs

jasonmalinowski · 2020-05-13T22:55:13Z

@CyrusNajmabadi Does this also close #30327?

JoeRobich · 2020-05-14T17:14:57Z

@CyrusNajmabadi So the extra using directives were being added because we were always adding Attribute namespaces to the list of usings, and the .NET 5 assemblies now have nullable annotations?

CyrusNajmabadi · 2020-05-14T17:36:10Z

@CyrusNajmabadi Does this also close #30327?

Yup!

CyrusNajmabadi · 2020-05-14T17:40:16Z

@CyrusNajmabadi So the extra using directives were being added because we were always adding Attribute namespaces to the list of usings, and the .NET 5 assemblies now have nullable annotations?

That was one problem. But the primary problem was that we basically turned off the step to then clean up those imports. So any added imports that turned out to be unnecessary just were not removed. Nullable just exacerbated this as you might have a ton of those in code which just exploded this sort of thing.

jasonmalinowski · 2020-05-14T19:28:41Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

-
-[NullableContextAttribute(1)]
-public interface [|C|]<[NullableAttribute(2)] T>
+public interface [|C|]<T>


Should this have gotten a #nullable enable added?

not that i can tell. @jcouv we dont' need #nullable enable here right? we don't have an annotated reference type afaict.

Tagging @cston. I'm surprised that we emit a NullableAttribute(2) (means ?) on an unconstrained T in the first place (sharplab)

The attribute value represents the lack of a notnull constraint - see examples.

src/Workspaces/Core/Portable/CodeGeneration/NullableSyntaxAnnotation.cs

...Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs

src/Workspaces/CSharp/Portable/CodeGeneration/AttributeGenerator.cs

jasonmalinowski · 2020-05-14T21:09:38Z

src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs

+                    // if we hit a type, and we're currently disabled, then switch us back to enabled for that type.
+                    // This ensures whenever we walk into a type-decl, we're always in the enabled-state.


Is that to simply the algorithm since we're doing this as a bottom-up rewrite so we can't really know where the type might end up?

that's exactly it. it also keeps things stateless so we don't need to keep track of everything as we walk.

src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs

jasonmalinowski · 2020-05-14T22:05:11Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

+{{
+    public TestType();
+
+#nullable disable


When looking at the implementation at first this seemed a bit strange to tag it this way but I have to say I think I actually like it -- it means there's always a #nullable enable at the top which is easy to see, and the special cases are called out specifically.

yup. that seemed really sane and sensible. it's the coding pattern i would prefer to see tbh :)

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs

…SourceService.cs

ghost

Auto-approval

jcouv · 2020-05-14T23:39:15Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

+        {
+        }
+
+    #nullable disable


nit: formatting

jcouv · 2020-05-14T23:41:19Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

+    public TestType();
+
+    public void [|M1|](dynamic s);
+}}";


Consider including a nasty case (where best effort fails):

List<string #nullable enable > M( #nullable disable string parameter) => throw null;

Also, the case we discussed yesterday where members omitted by MAS still affect the output.

CyrusNajmabadi added 3 commits May 13, 2020 12:21

initial

bc0b404

Generate #nullable

3164a4f

Add tests

7cd7226

CyrusNajmabadi requested a review from a team as a code owner May 13, 2020 22:06

CyrusNajmabadi requested a review from jcouv May 13, 2020 22:06

CyrusNajmabadi commented May 13, 2020

View reviewed changes

CyrusNajmabadi requested a review from jasonmalinowski May 13, 2020 22:11

CyrusNajmabadi commented May 13, 2020

View reviewed changes

src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs Show resolved Hide resolved

remove using

5674b62

jasonmalinowski approved these changes May 14, 2020

View reviewed changes

CyrusNajmabadi commented May 14, 2020

View reviewed changes

src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 2 commits May 14, 2020 15:22

Update src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAs…

f8ecb16

…SourceService.cs

Merge remote-tracking branch 'upstream/master' into nullableMAS

f3fc268

CyrusNajmabadi added the auto-merge label May 14, 2020

ghost approved these changes May 14, 2020

View reviewed changes

CyrusNajmabadi added 7 commits May 14, 2020 15:35

Add comment

45b6c2e

Simplify

6601df1

Filter out dynamic attribute.

f21590d

Fix nullable ref type parameters.

9183eb7

Add tests

6c8c3a0

Add test for nested type

02c58e0

Whitespace

d9c2d61

jcouv reviewed May 14, 2020

View reviewed changes

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

{

}

#nullable disable

Copy link

Member

jcouv May 14, 2020

Choose a reason for hiding this comment

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

nit: formatting

jcouv reviewed May 14, 2020

View reviewed changes

CyrusNajmabadi added 8 commits May 14, 2020 21:51

Fix test

f8de656

lint

ad2a2e1

lint

240bf6b

Merge remote-tracking branch 'upstream/master' into nullableMAS

08b7d85

Remove param

ab0f7ac

Fix

bca9b9f

lint

759c6cd

Merge remote-tracking branch 'upstream/master' into nullableMAS

31abe0c

CyrusNajmabadi merged commit f8ed9e7 into dotnet:master May 16, 2020

ghost added this to the Next milestone May 16, 2020

CyrusNajmabadi deleted the nullableMAS branch May 16, 2020 01:38

JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020

nxtn mentioned this pull request May 26, 2020

Generate constructor not taking into account NRT for type parameters #43273

Closed

		// if we hit a type, and we're currently disabled, then switch us back to enabled for that type.
		// This ensures whenever we walk into a type-decl, we're always in the enabled-state.

Conversation

CyrusNajmabadi commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasonmalinowski commented May 13, 2020

Uh oh!

JoeRobich commented May 14, 2020

Uh oh!

CyrusNajmabadi commented May 14, 2020

Uh oh!

CyrusNajmabadi commented May 14, 2020

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 14, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CyrusNajmabadi commented May 13, 2020 •

edited

Loading

jcouv May 14, 2020 •

edited

Loading