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

Fix reversed output in interpolation concatenation #55494

Merged
merged 5 commits into from
Aug 10, 2021

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Aug 7, 2021

Fixes #55493
Fixes #55461

Relates to feature/test plan: #51499

@Youssef1313 Youssef1313 requested a review from a team as a code owner August 7, 2021 13:41
Copy link
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.

Don't use a reverse on the parts arrays, reverse the builder after populating.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Aug 7, 2021

@333fred The builder is indeed reversed at the end of the method, but that wasn't enough (we were appending each part in-order, then reversing the final result - but we want to append each part reversed, then reverse the final result).

For the code to be reversing a builder instead of array, I'd have to create a new builder for each BoundInterpolatedString, or loop backwards on Parts and use Add for each part (instead of AddRange(Parts).

Alternatively, attempt to rewrite the code such that we don't need reversing at all (ie, walk from left to right - but I'm not sure how hard is that or if that's possible).

@333fred
Copy link
Member

333fred commented Aug 7, 2021

Left to right isn't feasible. If we need to add these in reverse order here as well, we should not use linq to do so.

@333fred
Copy link
Member

333fred commented Aug 7, 2021

I'm also concerned about flow analysis. Make sure we have a test like $"{a = null}{a.ToString()}" + $""

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

@jcouv ptal.

@jcouv jcouv self-assigned this Aug 8, 2021
@jcouv jcouv added the Feature - Interpolated String Improvements Interpolated string improvements label Aug 8, 2021
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 Thanks (iteration 4)

@jcouv jcouv enabled auto-merge (squash) August 9, 2021 20:25
@jcouv jcouv disabled auto-merge August 10, 2021 07:15
@jcouv
Copy link
Member

jcouv commented Aug 10, 2021

Looks like there is a legitimate failure: InterpolatedStringsAddedUnderObjectAddition3 in the spanish CI branch because of localization of "Object reference not set to an instance of an object.". Consider just printing the exception type name.

@jcouv jcouv merged commit 909e6dc into dotnet:main Aug 10, 2021
@ghost ghost added this to the Next milestone Aug 10, 2021
@jcouv
Copy link
Member

jcouv commented Aug 10, 2021

Thanks @Youssef1313

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.

Interpolated string regression? Binary addition of interpolated strings mangles the string
4 participants