Skip to content

Check val escape for initializers of fields with ref-like type#60577

Merged
jcouv merged 5 commits intodotnet:release/dev17.2from
cston:60568
Apr 12, 2022
Merged

Check val escape for initializers of fields with ref-like type#60577
jcouv merged 5 commits intodotnet:release/dev17.2from
cston:60568

Conversation

@cston
Copy link
Copy Markdown
Contributor

@cston cston commented Apr 5, 2022

Fixes #60568

See AB#1520561

@ghost ghost added the Area-Compilers label Apr 5, 2022
@cston cston marked this pull request as ready for review April 5, 2022 04:22
@cston cston requested a review from a team as a code owner April 5, 2022 04:22
if (!boundInitializer.Value.HasAnyErrors)
{
var field = boundInitializer.Field;
if (field.Type.IsRefLikeType)
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.

It wasn't clear to me whether we should check field.Type.IsRefLikeType or field.ContainingType.IsRefLikeType or both.

Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson Apr 5, 2022

Choose a reason for hiding this comment

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

I think bad ref escapes can only occur into field when field.Type.IsRefLikeType (until we add ref fields at least.)

Maybe a scenario like field = M(ref staticField) where M returns Span would be worth testing. (though I guess the safe-to-escape of a static field is global? I can't seem to think of a scenario that should cause an error here besides stackalloc. Possibly some weird scenario involving pattern variables within the initializer.)

Copy link
Copy Markdown
Contributor Author

@cston cston Apr 6, 2022

Choose a reason for hiding this comment

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

As you mentioned, the safe-to-escape of a static field is global.

I've added an item to the ref fields test plan to consider ref struct field initializers when ref fields is supported.

var field = boundInitializer.Field;
if (field.Type.IsRefLikeType)
{
var value = parentBinder.ValidateEscape(boundInitializer.Value, ExternalScope, isByRef: false, diagnostics);
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Apr 5, 2022

Choose a reason for hiding this comment

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

ValidateEscape

We don't need to do the same check in BindScriptFieldInitializers? #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.

IsRefLikeType fields cannot be declared in a script since the containing type is not a ref struct. Added a test.

var field = boundInitializer.Field;
if (field.Type.IsRefLikeType)
{
var value = parentBinder.ValidateEscape(boundInitializer.Value, ExternalScope, isByRef: false, diagnostics);
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Apr 5, 2022

Choose a reason for hiding this comment

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

var

The type is not obvious. #Closed

@cston cston mentioned this pull request Apr 6, 2022
@cston cston changed the title Check val escape for struct field initializers Check val escape for initializers of fields with ref-like type Apr 6, 2022
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 (commit 5)

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