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

Implements the initial set of span-safety checks. #18482

Merged
merged 2 commits into from
Apr 7, 2017

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Apr 5, 2017

These are mostly the checks dealing with stack-only nature of the Span. With minor differences they follow the same logic as the existing checks for types such as TypedReference.

==== Not in this change:
Defining and detecting span-like types is NYI. For now we just treat any type named "System.Span" and "System.ReadonlySpan" as span-like. This will change.

Some of the checks result in somewhat generic messages and happen at emit phase. That was ok when the errors were supposed to be rare.
Error clarity is not the goal of this change, but we will examine what errors should say and whether they should be moved to an earlier phase. (issue: #18483)

@VSadov
Copy link
Member Author

VSadov commented Apr 5, 2017

@OmarTawfik - please review. Thanks!

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

@VSadov My only comment is whether it makes sense to consider Span a restricted type, which they have a specific meaning in the compiler code base.
Should it be another API?
Type.IsRestrictedType()
Type.IsSpanLikeType()
In that sense, Spans are not restricted types to me, but rather their own special category. Otherwise, LGTM.

@VSadov
Copy link
Member Author

VSadov commented Apr 6, 2017

@OmarTawfik It is convenient that IsRestrictedType defers to IsSpanLikeType, since for nearly all purposes of this check spans are a subset of restricted types.
"Restricted Types" is basically "types that may contain a managed ref field" and spans are such types. I cannot think of a better name for that than "IsRestrictedType" though. That seems good enough for internal API with a specific purpose.

"IsSpanLikeType" is also not a public API, yet.
Perhaps it will have to become a public API and I am not sure the name is the best - IsSpanLike, IsByRefLike,...
I did not want to make that decision or make it public in this change.

These are mostly the checks dealing with stack-only nature of the Span. With minor difference they follow the same logic as the existing checks for types such as TypedReference.

==== Not in this change:
Defining and detecting span-like types is NYI. For now we just treat any type named "System.Span" and "System.ReadonlySpan" as span-like. This will change.

Some of the checks result in somewhat generaic messages and happen at emit phase. That was ok when the failures were supposed to be rare.
Error clarity is not the goal of this change, but we will examone what errors should say and whether they should be moved to an earlier phase.
@VSadov
Copy link
Member Author

VSadov commented Apr 6, 2017

@dotnet/roslyn-compiler - please take a look

This will be allowed only for span-like containers eventually, when we can declare those.
For now allow in any struct.
@VSadov
Copy link
Member Author

VSadov commented Apr 7, 2017

I need to merge this to test on the actual code in CoreFxLab, but will look at the PR if there are any further suggestions.

@VSadov VSadov merged commit 65839b1 into dotnet:features/readonly-ref Apr 7, 2017
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.

3 participants