Skip to content

Conversation

@DoctorKrolic
Copy link
Contributor

@DoctorKrolic DoctorKrolic commented Oct 26, 2025

Redo of #74218
Fixes #73894

@DoctorKrolic DoctorKrolic requested a review from a team as a code owner October 26, 2025 13:59
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 26, 2025
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 27, 2025

Done with review pass (commit 1), tests were not looked at. It looks like the change affects scenarios that fall outside of the stated goal of this PR. I suggest making adjustments to affect only error scenarios with target typed new on the right. #Closed

@DoctorKrolic
Copy link
Contributor Author

This PR is based on the following semantics of ??= operator:

  • If left type is a reference type, the right-hand side should also be that reference type
  • If left type is nullable value type, the right-hand side should be an underlying value type of that nullable type

This semantics is independent on what shape the right-hand side is. Therefore, limiting this recovery to only new() scenarios seems like an overfitting to me. If you look at tests, I heavily verified other target-typed expressions to ensure they get assigned a type since they are most likely to benifit from this change in IDE scenarios even if specific bugs where not discovered yet. But even for non-target-typed scenarios this recovery makes sense since now compiler can insert additional conversions when necessary, which are gonna be present in the success scenarios anyway. Look at ErrorRecovery_ReferenceType_ImplicitConversion and ErrorRecovery_NullableValueType_ImplicitConversion for such examples

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 3, 2025

If left type is nullable value type, the right-hand side should be an underlying value type of that nullable type

I do not think this is accurate. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 3, 2025

Done with review pass (commit 2), tests were not looked at. Some of the previously made comments still look relevant. Also, an alternative way to suppress cascading diagnostics was suggested. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 19, 2025

@DoctorKrolic It looks like PR validation is failing #Closed

@DoctorKrolic DoctorKrolic requested a review from a team as a code owner November 20, 2025 18:41
Copy link
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 10)

@AlekseyTs AlekseyTs requested a review from a team November 21, 2025 20:10
@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For a second review on a community PR

1 similar comment
@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For a second review on a community PR

@AlekseyTs
Copy link
Contributor

@DoctorKrolic Does this fix #73894 or there is remaining work there? If there is a remaining work, could you please provide details in the issue?

@DoctorKrolic
Copy link
Contributor Author

It seems that scenario from the issue related to other compound assignment operators might not be valid. If so then this PR fully closes the issue. Asked for clarification from the issue's author

@DoctorKrolic
Copy link
Contributor Author

Ok, confirmed, other scenarios are not valid. Modified PR description to close the issue when it is merged

@AlekseyTs AlekseyTs merged commit fc8030f into dotnet:main Nov 25, 2025
29 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 25, 2025
@AlekseyTs
Copy link
Contributor

@DoctorKrolic Thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parameter names in target-typed new() expressions are missing among completions in compound assignment statements

3 participants