Skip to content

Produce correct diagnostics on implicit operator conversions for stackalloc expressions#26248

Merged
OmarTawfik merged 2 commits intodotnet:masterfrom
OmarTawfik:fix-26195-stackalloc-conversion-crash
May 9, 2018
Merged

Produce correct diagnostics on implicit operator conversions for stackalloc expressions#26248
OmarTawfik merged 2 commits intodotnet:masterfrom
OmarTawfik:fix-26195-stackalloc-conversion-crash

Conversation

@OmarTawfik
Copy link
Copy Markdown
Contributor

@OmarTawfik OmarTawfik commented Apr 19, 2018

Fixes #26195
cc @dotnet/roslyn-compiler for reivew.

@OmarTawfik OmarTawfik added Bug Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 19, 2018
@OmarTawfik OmarTawfik added this to the 15.8 milestone Apr 19, 2018
@OmarTawfik OmarTawfik self-assigned this Apr 19, 2018
@OmarTawfik OmarTawfik requested a review from a team as a code owner April 19, 2018 00:16
@OmarTawfik OmarTawfik removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 19, 2018
Copy link
Copy Markdown
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented Apr 23, 2018

@jaredpar for approval. #Closed

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Apr 23, 2018

Choose a reason for hiding this comment

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

Test2 [](start = 33, length = 5)

Do we have tests for scenarios when this is "Test" instead of "Test2"? #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Please check the tests at the start of this file for semantic model, and tests at the end of CodeGenTests.cs for emitting the conversion.


In reply to: 183469924 [](ancestors = 183469924)

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Apr 23, 2018

Choose a reason for hiding this comment

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

Test2 [](start = 33, length = 5)

Do we have tests for scenarios when this is "Test" instead of "Test2"? #Closed

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Apr 23, 2018

Choose a reason for hiding this comment

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

ConversionKind.StackAllocToPointerType [](start = 21, length = 38)

ConversionKind.StackAllocToPointerType [](start = 21, length = 38)

It is not obvious why is this correct fix. Some explanation and perhaps a quote from relevant portion of language specification might help. #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is no language spec as this is a C# 7.2 feature.
This is the up to date document: https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.2/span-safety.md

About why is that the correct fix, I'll add a comment to explain.


In reply to: 183471406 [](ancestors = 183471406)

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Apr 23, 2018

Done with review pass (iteration 1) #Closed

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 8, 2018

@AlekseyTs comments addressed.
Had to rebase because of recent project system changes in 15.8, but the PR is small enough that it won't cause a problem in reviewing older comments. #Closed

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 2)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 8, 2018

@jcouv for ask mode approval. #Closed

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 9, 2018

Approved. Thanks

@OmarTawfik OmarTawfik merged commit 8abcaf2 into dotnet:master May 9, 2018
@OmarTawfik OmarTawfik deleted the fix-26195-stackalloc-conversion-crash branch May 9, 2018 07:13
333fred added a commit that referenced this pull request May 14, 2018
* dotnet/master: (732 commits)
  Disable procdump on Spanish runs
  use named args.
  Exclude System.Diagnostics.DiagnosticSource from PortableFacades
  Exclude System.Net.Http from PortableFacades
  Align the implementations of IAssemblySymbol.GivesAccessTo (#26727)
  Fixed exception message in GetSemanticModel (#26659)
  Implement IDiscardSymbol.Equals and GetHashCode (#26720)
  Produce correct diagnostics on implicit operator conversions for stackalloc expressions (#26248)
  Remove special case handling of CreateDiagnosticProviderAndFixer throwing an exception
  Add verbose logging to DevDivInsertionFiles if you want it
  Exclude Microsoft.Composition as something we build a dependency map for
  Fix the message for Make Field Readonly
  Update LangVersion to 7.3 (#26698)
  fix extension deployment and add explainatory comment
  Add TaskExtensions to compiler package
  Add System.Diagnostics.DiagnosticSource to SignToolData.json
  Include missing facade: System.Diagnostics.DiagnosticSource
  Use default expressions.
  Remove forwarding method.
  Revert project chnage.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants