Skip to content

Check interpolated string arguments in Binder.GetValEscape()#63263

Merged
cston merged 4 commits intodotnet:mainfrom
cston:63262
Aug 9, 2022
Merged

Check interpolated string arguments in Binder.GetValEscape()#63263
cston merged 4 commits intodotnet:mainfrom
cston:63262

Conversation

@cston
Copy link
Copy Markdown
Contributor

@cston cston commented Aug 8, 2022

Fixes #63262

@cston cston requested a review from a team as a code owner August 8, 2022 17:39
@ghost ghost added the Area-Compilers label Aug 8, 2022
Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass (commit 1). /cc @stephentoub, presumably DefaultInterpolatedStringHandler will need to react to this breaking change.

@RikkiGibson RikkiGibson self-assigned this Aug 8, 2022
RikkiGibson
RikkiGibson previously approved these changes Aug 8, 2022
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM modulo the review comments.

{
Span<char> s = stackalloc char[10];
CustomHandler h2 = new CustomHandler(0, 1);
h2.AppendFormatted(s); // 1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't understand why the interpolated string version had no errors but the "explicit" version has this error. Could you please clarify?

Copy link
Copy Markdown
Contributor Author

@cston cston Aug 8, 2022

Choose a reason for hiding this comment

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

My understanding is the interpolated string version is considered as creating a CustomHandler instance as if the new CustomHandler() and the interpolated string arguments were combined as an object initializer. In F1(), where the interpolated string argument s has safe-to-escape of the current method, the resulting CustomHandler instance will have safe-to-escape of the current method as well. In F2(), the argument s is not included in the CustomHandler object initializer, so the resulting instance is safe-to-escape of the calling method.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding is the interpolated string version is considered as creating a CustomHandler instance as if the new CustomHandler() and the interpolated string arguments were combined as an object initializer.

I would expect these scenarios to have the same behavior.

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.

I would expect these scenarios to have the same behavior.

If we make the CustomHandler in the first example explicit, the two examples are essentially:

CustomHandler h1 = $"{s}";
M(h1);

CustomHandler h2 = new CustomHandler(0, 1);
h2.AppendFormatted(s);
M(h2);

The difference between the two is the initializers: the initializer for h1 limits the scope to the current method; the initializer for h2 does not limit the scope.

Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass (commit 2). The current behavior is not what I'd expect from the feature.

@RikkiGibson RikkiGibson dismissed their stale review August 8, 2022 20:55

needs review after upcoming revisions

@cston cston merged commit 91aa521 into dotnet:main Aug 9, 2022
@cston cston deleted the 63262 branch August 9, 2022 21:34
@ghost ghost added this to the Next milestone Aug 9, 2022
@dibarbet dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022
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.

Unexpected escape analysis behavior for interpolated string with variable of local scope

4 participants