Do not suggest using implicit type for stackalloc initialization #22779

OmarTawfik · 2017-10-19T22:40:52Z

vso bug : https://devdiv.visualstudio.com/DevDiv/NET%20Developer%20Experience%20Productivity/_workitems/edit/517749

cc @sharwell @jcouv @dotnet/roslyn-ide

jcouv

LGTM Thanks

CyrusNajmabadi · 2017-10-19T23:40:21Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseImplicitTypeDiagnosticAnalyzer.cs

+                var initializer = variable.Initializer.Value;
+
+                // Discourage using "var pointer = stackalloc", after introducing C# 7.2, as it might be confusing to users expecting a Span<>
+                // Check descendant nodes as well, because Span-creating stackallocs can be nested in other nodes (like conditional operators and casts).


that seems odd... what if i had something like Type t = foo(... stackalloc ...);

This would still be a location where converting to 'var' woudl be ok, right? #Resolved

what if the stackalloc is inside a lambda that is on the right hand side? it seems weird that this would disable this feature at this level.

Such syntax is disabled. Please check #22046 #Closed

You can't have a stackalloc inside lambda on the RHS? #Pending

You can. I'll test that as well.

CyrusNajmabadi · 2017-10-19T23:40:41Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseImplicitTypeDiagnosticAnalyzer.cs

+                    return false;
+                }
+
+                if (AssignmentSupportsStylePreference(variable.Identifier, typeName, initializer, semanticModel, optionSet, cancellationToken))


i woudl keep the wrapping as it was before. #Resolved

sharwell · 2017-10-20T15:31:13Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseImplicitTypeDiagnosticAnalyzer.cs

-                        semanticModel, optionSet, cancellationToken))
+                var initializer = variable.Initializer.Value;
+
+                // Discourage using "var pointer = stackalloc", after introducing C# 7.2, as it might be confusing to users expecting a Span<>


❗️ This analyzer isn't the place to do this. The new C# 7.2 features do not affect the use of var from the perspective of this analyzer.

@sharwell can you please explain? the IDE should never suggest that particular pattern in that case.

sharwell · 2017-10-20T15:32:20Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseImplicitTypeDiagnosticAnalyzer.cs

+                // Discourage using "var pointer = stackalloc", after introducing C# 7.2, as it might be confusing to users expecting a Span<>
+                // Check descendant nodes as well, because Span-creating stackallocs can be nested in other nodes (like conditional operators and casts).
+                // https://github.com/dotnet/roslyn/issues/22768
+                if (initializer.DescendantNodesAndSelf().Any(node => node.IsKind(SyntaxKind.StackAllocArrayCreationExpression)))


❓ Why is this needed? We already have the following block which is intended to catch exactly this case. For example, no special casing is needed for IFormattable f = $"";.

roslyn/src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseImplicitTypeDiagnosticAnalyzer.cs

Lines 180 to 184 in 4b1c677

var conversion = semanticModel.GetConversion(expression, cancellationToken);

if (conversion.Exists && conversion.IsImplicit && !conversion.IsIdentity)

{

return false;

}

Because in cases where compiler can bind the node successfully without errors, it will be replaced with a BoundConvertedStackAllocExpression. That's what the semantic model will return.

sharwell · 2017-10-20T15:34:56Z

src/EditorFeatures/CSharpTest/Diagnostics/UseImplicitOrExplicitType/UseImplicitTypeTests.cs

+{
+    unsafe static void M()
+    {
+        [|int*|] x = stackalloc int [10];


❗️ For the ImplicitTypeEverywhere() options, this should report a diagnostic. Span<T> is not a special case, the same way string interpolation with IFormattable is not a special case.

CyrusNajmabadi · 2017-10-24T05:22:42Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpUseImplicitTypeDiagnosticAnalyzer.cs

+                if (!variableDeclaration.Type.IsKind(SyntaxKind.PointerType)
+                    && initializer
+                        .DescendantNodesAndSelf(descendIntoChildren: node => !node.IsAnyLambdaOrAnonymousMethod())
+                        .Any(node => node.IsKind(SyntaxKind.StackAllocArrayCreationExpression)))


FWIW, i would prefer really long expressions like this get broken out into either a helper method, or into statements. i.e.:

var isPointerType = ... var containsStackAlloc = ... if (!isPointerType && containsStackAlloc) { ...

It's just much more readable to me that way (IMO).

OmarTawfik · 2017-10-24T15:09:30Z

@sharwell any other comments?

OmarTawfik · 2017-10-30T21:05:28Z

@sharwell can you please take a look?

jinujoseph · 2017-10-31T17:03:36Z

@OmarTawfik , submitting this for shiproom barcheck...can you fill in the ask mode template .
Once approved you are re-target this to master branch.
Thanks for taking care of this

OmarTawfik · 2017-10-31T19:25:04Z

@jinujoseph filled the questions in the VSO bug. Please let me know if I should do anything else.

sharwell · 2017-10-31T19:38:12Z

@OmarTawfik this will need to be rebased so we can target master.

OmarTawfik · 2017-11-02T00:15:52Z

@sharwell it is not going to be a pretty/straightforward rebase :(
I'll move these changes to another PR against master if it gets approved for 15.5.

sharwell · 2017-11-02T11:42:02Z

@OmarTawfik I went ahead and did the rebase and changed the base branch of this PR to master. This will allow the tests to complete for the correct target so we're ready if approval comes through. If it doesn't get approved, we'll just switch the base branch back to 15.6 - a second rebase would not be required. 👍

jinujoseph · 2017-11-03T00:29:17Z

Pre-approved to merge via link

OmarTawfik added Area-IDE Bug Feature - Readonly References Readonly References labels Oct 19, 2017

OmarTawfik added this to the 15.later milestone Oct 19, 2017

OmarTawfik self-assigned this Oct 19, 2017

dnfclas added the cla-already-signed label Oct 19, 2017

OmarTawfik requested a review from sharwell October 19, 2017 22:45

jcouv approved these changes Oct 19, 2017

View reviewed changes

CyrusNajmabadi reviewed Oct 19, 2017

View reviewed changes

sharwell requested changes Oct 20, 2017

View reviewed changes

sharwell reviewed Oct 20, 2017

View reviewed changes

CyrusNajmabadi reviewed Oct 24, 2017

View reviewed changes

sharwell approved these changes Oct 30, 2017

View reviewed changes

jinujoseph added the Needs Shiproom Approval label Oct 31, 2017

sharwell changed the base branch from post-dev15.5-contrib to master October 31, 2017 19:37

sharwell changed the base branch from master to post-dev15.5-contrib October 31, 2017 19:37

OmarTawfik changed the base branch from post-dev15.5-contrib to master November 2, 2017 00:02

OmarTawfik changed the base branch from master to merges/master-to-post-dev15.5-contrib-20171031-070016 November 2, 2017 00:13

OmarTawfik changed the base branch from merges/master-to-post-dev15.5-contrib-20171031-070016 to post-dev15.5-contrib November 2, 2017 00:13

OmarTawfik added 3 commits November 2, 2017 06:40

Do not suggest using implicit type for stackalloc initialization

7f077aa

PR comments

fef6618

style comments

e7e9fa4

sharwell force-pushed the fix-22768-suggest-var-for-stackalloc branch from 4fa5c0d to e7e9fa4 Compare November 2, 2017 11:40

sharwell changed the base branch from post-dev15.5-contrib to master November 2, 2017 11:41

sharwell modified the milestones: 15.6, 15.5 Nov 2, 2017

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Nov 3, 2017

sharwell merged commit 089a2d3 into master Nov 3, 2017

sharwell deleted the fix-22768-suggest-var-for-stackalloc branch November 3, 2017 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not suggest using implicit type for stackalloc initialization #22779

Do not suggest using implicit type for stackalloc initialization #22779

OmarTawfik commented Oct 19, 2017 •

edited by jinujoseph

Loading

jcouv left a comment

CyrusNajmabadi Oct 19, 2017 •

edited by OmarTawfik

Loading

CyrusNajmabadi Oct 19, 2017

OmarTawfik Oct 20, 2017 •

edited

Loading

CyrusNajmabadi Oct 20, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Oct 23, 2017 •

edited

Loading

CyrusNajmabadi Oct 19, 2017 •

edited by OmarTawfik

Loading

sharwell Oct 20, 2017

OmarTawfik Oct 23, 2017

sharwell Oct 20, 2017

OmarTawfik Oct 23, 2017

sharwell Oct 20, 2017 •

edited

Loading

CyrusNajmabadi Oct 24, 2017

CyrusNajmabadi Oct 24, 2017

OmarTawfik commented Oct 24, 2017

OmarTawfik commented Oct 30, 2017

jinujoseph commented Oct 31, 2017

OmarTawfik commented Oct 31, 2017

sharwell commented Oct 31, 2017

OmarTawfik commented Nov 2, 2017

sharwell commented Nov 2, 2017 •

edited

Loading

jinujoseph commented Nov 3, 2017

	var conversion = semanticModel.GetConversion(expression, cancellationToken);
	if (conversion.Exists && conversion.IsImplicit && !conversion.IsIdentity)
	{
	return false;
	}

Do not suggest using implicit type for stackalloc initialization #22779

Do not suggest using implicit type for stackalloc initialization #22779

Conversation

OmarTawfik commented Oct 19, 2017 • edited by jinujoseph Loading

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi Oct 19, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 19, 2017

Choose a reason for hiding this comment

OmarTawfik Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 20, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 19, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

sharwell Oct 20, 2017

Choose a reason for hiding this comment

OmarTawfik Oct 23, 2017

Choose a reason for hiding this comment

sharwell Oct 20, 2017

Choose a reason for hiding this comment

OmarTawfik Oct 23, 2017

Choose a reason for hiding this comment

sharwell Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 24, 2017

Choose a reason for hiding this comment

CyrusNajmabadi Oct 24, 2017

Choose a reason for hiding this comment

OmarTawfik commented Oct 24, 2017

OmarTawfik commented Oct 30, 2017

jinujoseph commented Oct 31, 2017

OmarTawfik commented Oct 31, 2017

sharwell commented Oct 31, 2017

OmarTawfik commented Nov 2, 2017

sharwell commented Nov 2, 2017 • edited Loading

jinujoseph commented Nov 3, 2017

OmarTawfik commented Oct 19, 2017 •

edited by jinujoseph

Loading

CyrusNajmabadi Oct 19, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Oct 20, 2017 •

edited

Loading

CyrusNajmabadi Oct 20, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Oct 23, 2017 •

edited

Loading

CyrusNajmabadi Oct 19, 2017 •

edited by OmarTawfik

Loading

sharwell Oct 20, 2017 •

edited

Loading

sharwell commented Nov 2, 2017 •

edited

Loading