Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check generic construction diagnostics for stackalloc to span conversions #25131

Merged
merged 2 commits into from
Mar 6, 2018
Merged

Check generic construction diagnostics for stackalloc to span conversions #25131

merged 2 commits into from
Mar 6, 2018

Conversation

OmarTawfik
Copy link
Contributor

Closes #25086
Closes #25038

There is a bug in C# 15.5, where during the conversion of a stackalloc expression to a Span<T> object creation, the type parameter construction and constraints matching were not checked during binding.

Fixing this for 15.7 is a breaking change, because an expression of stackalloc T [count] would be legal even if T was not legal to initialize Span<T> with. We would previously emit IL even if it would cause run time exceptions. do believe that the scope is small enough for us to take the breaking change though.

@OmarTawfik OmarTawfik added this to the 15.7 milestone Feb 28, 2018
@OmarTawfik OmarTawfik self-assigned this Feb 28, 2018
@OmarTawfik OmarTawfik requested a review from a team as a code owner February 28, 2018 21:25
@OmarTawfik
Copy link
Contributor Author

OmarTawfik commented Feb 28, 2018

cc @dotnet/roslyn-compiler for review

Copy link
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:

@OmarTawfik
Copy link
Contributor Author

cc @AlekseyTs as you first raised this in a different PR.

@alrz
Copy link
Member

alrz commented Mar 1, 2018

@OmarTawfik May we sync features/stackalloc-init with dev15.7.x after this goes in?

@OmarTawfik
Copy link
Contributor Author

@alrz. Yes. The owner of the feature branch can update it from 15.7.x once approved and merged.

Copy link
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. Please get compat council to OK the break before merging.

@VSadov
Copy link
Member

VSadov commented Mar 1, 2018

Yes. We should check with compat council.

It is highly unlikely though that this breaks any functional code. Allowing Span in stackalloc would easily result in a GC holes and heap corruption.

Copy link
Member

@VSadov VSadov 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
Contributor Author

@jaredpar for ask mode approval.

@OmarTawfik
Copy link
Contributor Author

@jaredpar can you please take a look?

@jaredpar
Copy link
Member

jaredpar commented Mar 6, 2018

approved

@jaredpar jaredpar merged commit ca95348 into dotnet:dev15.7.x Mar 6, 2018
@OmarTawfik OmarTawfik deleted the type-of-span-stackalloc branch March 6, 2018 23:48
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.

6 participants