Prefer expanded form for error reporting when params arguments are convertible #81276
Conversation
When a method has a params parameter and an argument would be convertible to the element type when expanded, don't report an error for that argument. The real error is with another argument that prevents the method from being applicable. This fixes issue #8925 where calling string.Join(args, ".") incorrectly reported "cannot convert from 'string' to 'params object?[]'" when the real issue was the first argument failing to convert to 'char'. Co-authored-by: CyrusNajmabadi <[email protected]>
|
|
||
| ParameterSymbol parameter = parameters[parm]; | ||
| bool isLastParameter = parameters.Length == parm + 1; // This is used to later decide if we need to try to unwrap a params collection | ||
|
|
There was a problem hiding this comment.
@copilot try another approach. INstead of trying to suppress reporting someething here, change it so that this argument is not in MemberAnalysisResult.BadArgumentsOpt in the first place. This means that during binding we should not consider this arg in error because we should have felt it matched the expanded parmeter array type.
There was a problem hiding this comment.
Changed the approach to modify form selection in PreferExpandedFormOverNormalForm rather than error reporting. The fix now prefers expanded form when the normal form has a bad argument for the params parameter but the expanded form doesn't, ensuring we report more accurate errors. Commit bf126f6.
Changed PreferExpandedFormOverNormalForm to prefer the expanded form when: - Both normal and expanded forms fail with BadArgumentConversion - The normal form has a bad argument for the last (params) parameter - The expanded form does NOT have a bad argument for that parameter This ensures that when a params argument would convert to the element type in expanded form, we report errors from the expanded form rather than the misleading "cannot convert to params T[]" error from normal form. Addresses feedback from @CyrusNajmabadi to handle this at the form selection level rather than at error reporting level. Co-authored-by: CyrusNajmabadi <[email protected]>
| // M(char c, params object[] args) called with M(string[], string) | ||
| // where string[] -> char is bad and string -> object[] is bad in normal form, | ||
| // but string -> object is good in expanded form. | ||
| if (haveBadArgumentForLastParameter(normalResult) && !haveBadArgumentForLastParameter(expandedResult)) |
There was a problem hiding this comment.
@copilot the 'if' statement above the 'if-statement' you added looks suspect. The one above this checks haveBadArgumentForLastParameter(expandedResult) and this one checks !haveBadArgumentForLastParameter(expandedResult). So effectively both optoins are checked. can we just simplify the above, and fully explain the situation.
Or, if the above 'if-statement' is actually incorrect and we only want to keep the new one, see if we can remove the old one if it doesn't make things worse.
There was a problem hiding this comment.
Simplified the logic by removing the redundant checks. Since the two if-statements covered all cases where haveBadArgumentForLastParameter(normalResult) is true (both when expanded has bad arguments and when it doesn't), we can just check if normal form has bad arguments for the last parameter and prefer expanded form in that case. Commit 50eb025.
Simplified the condition checks by removing redundant logic. Since we check both cases where expandedResult has bad arguments and where it doesn't, we can simply check if normalResult has bad arguments for the last parameter and prefer expanded form in that case, regardless of the expanded form's state. This makes the code clearer and addresses the observation that both conditions were effectively covering all cases where normalResult has bad arguments. Co-authored-by: CyrusNajmabadi <[email protected]>
| CreateCompilation(source).VerifyDiagnostics(); | ||
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/8925")] |
| // When a method has a params parameter and an argument would be convertible | ||
| // to the element type when the params is expanded, we should not report an error | ||
| // for that argument. The real error is with another argument that prevents | ||
| // the method from being applicable. |
There was a problem hiding this comment.
@copilot remove these actual source code comments. you can keep the comments in the string literal. #Closed
There was a problem hiding this comment.
Removed the C# comments outside the string literals from both test methods. The comments inside the test source code strings remain. Commit 4438555.
Changed the WorkItem attribute URLs in the test methods from issue #8925 to the correct issue #12933 which this PR fixes. Co-authored-by: CyrusNajmabadi <[email protected]>
Removed the C# comments outside the string literals in the test methods, keeping only the comments inside the test source code strings. Co-authored-by: CyrusNajmabadi <[email protected]>
Added a test case that verifies the fix also addresses issue #43920. The test confirms that when a params method fails due to a non-params argument, we don't report a misleading error for the params argument that would be valid if expanded. Co-authored-by: CyrusNajmabadi <[email protected]>
Added a test for issue #43920 which passes, confirming the fix addresses that issue as well. The test verifies that when Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| var source = """ | ||
| class Program | ||
| { | ||
| static void M(int x, params object[] args) { } |
There was a problem hiding this comment.
| { | ||
| // First argument fails: string -> int | ||
| // Second argument would succeed if expanded: string -> object | ||
| // We should only report the first error. |
There was a problem hiding this comment.
@copilot remove the comments from within the test literals. #Closed
There was a problem hiding this comment.
Removed all comments from within the test string literals in all four test methods and updated the diagnostic line numbers accordingly. Commit 4e72746.
Added a test case with the specific signature from issue #43920: A(string a, params string[] b). The test confirms that when called with A(1, "test"), only the first argument error is reported (int -> string), not a misleading error about the params argument. Co-authored-by: CyrusNajmabadi <[email protected]>
Removed all comments from within the test source code string literals and updated the expected line numbers in the diagnostic assertions to match the new line positions. Co-authored-by: CyrusNajmabadi <[email protected]>
|
@dotnet/roslyn-compiler this is ready for review. |
|
@jjonescz ptal :) |
Plan: Fix Overload Resolution Error Reporting for Params Collections
Summary of Changes
File: OverloadResolution.cs
Simplified
PreferExpandedFormOverNormalFormto remove redundant condition checks. The logic now simply checks ifhaveBadArgumentForLastParameter(normalResult)is true and prefers the expanded form in that case, which covers both scenarios:This makes the logic clearer while maintaining the same behavior: prefer expanded form when normal form has a bad argument for the params parameter, which ensures more accurate error messages.
File: OverloadResolutionTests.cs
ParamsErrorSuppression_Issue43920: Tests withparams object[]signatureParamsErrorSuppression_Issue43920_StringParams: Tests withparams string[]signature (the specific signature from the issue)Test Results:
Issues Fixed:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.