Improve error location of missing object.ToString() for concatenation

[DoctorKrolic]

Extracted from #71793 (suggested in #71793 (comment))

Currently if object.ToString() member is missing error is reported on the whole concatenation. It makes more sense to report in on the argument, which actually requested that member.

E.g. consider concatenation a + b + c where a is a string and b and c are not. If object.ToString() is missing without a chage of this PR error locations will be for b - a + b and for c - the whole a + b + c. This PR chages it so that errors are reported directly on b and c. Added 2 tests to show this behavior for 3 and 4 concatenation arguments

I know that this is a very minor change and I would not normally make it, but since such separation of changes was requested by a compiler team meber, here is it. The "fix" is just several charachters long. And regardless of whether this is accepted or not, a lot more tests of missing object.ToString() for concatenation are about to come in #71793

[AlekseyTs]

    private BoundExpression ConvertConcatExprToString(SyntaxNode syntax, BoundExpression expr)

I would like to suggest making a more general change instead:

• getting rid of the syntax parameter
• adding SyntaxNode syntax = expr.Syntax; at the beginning of the function. #Closed

---

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs:369 in 2d37bbc. [](commit_id = 2d37bbc, deletion_comment = False)

[AlekseyTs]

Done with review pass (commit 1).

[AlekseyTs]

Since this change is significantly less complicated (I would even say trivial), I think we should delay merging until after the other PR is merged.

[DoctorKrolic]

this change is significantly less complicated (I would even say trivial)

See, this is the reason I did it in the first place. This is a type of chage which brings barely noticable improvment, but for equally barely noticable effort

I think we should delay merging until after the other PR is merged

There is a slight problem here. I kinda trapped myself since tests, added in #71793 (specifically in a288d21) depend on this new error location. I would prefer to resolve a simple merge conflict rather than change all locations in those tests back and forth in different PRs.

[AlekseyTs]

See, this is the reason I did it in the first place. This is a type of chage which brings barely noticable improvment, but for equally barely noticable effort

Still not a reason to not follow a guideline and bundle it with an unrelated change. From a contributor aware of the guideline, I personally expect the contributor to follow it, i.e. not sending a PR with an unrelated change for review, rather than doing that and waiting if a request will be made to revert it.

[AlekseyTs]

I would prefer to resolve a simple merge conflict rather than change all locations in those tests back and forth in different PRs.

I primarily wanted to avoid a blanket merge from 'main' to the other PR. For whatever reason, the tool that we are using for code reviews might "go of the rails" after merges like that. I guess I would be comfortable if you merge the specific commit rather than the entire main. How does this sound?

[AlekseyTs]

LGTM (commit 2)

[AlekseyTs]

@dotnet/roslyn-compiler For the second review

[DoctorKrolic]

if you merge the specific commit rather than the entire main. How does this sound?

No problem with that. Sorry for creating this issue in the first place

[AlekseyTs]

@DoctorKrolic Thank you for the contribution. Note, I squashed the commits together while merging this PR, this is what we usually do.