Disallow converted string as default argument value #59806
Conversation
| @@ -624,12 +624,11 @@ private static void CheckParameterModifiers(BaseParameterSyntax parameter, Bindi | |||
| hasErrors = true; | |||
| } | |||
| else if (conversion.IsReference && | |||
| (parameterType.SpecialType == SpecialType.System_Object || parameterType.Kind == SymbolKind.DynamicType) && | |||
There was a problem hiding this comment.
Have you gone back in the history to find out why this line was added? This seems extremely deliberate, so I want to have a good understanding of why it was there in the first place.
There was a problem hiding this comment.
This line dates from Pilchie's "Hello World" commit.
I should have copied the info here from the bug thread, but the native compiler correctly produced an error:
test.cs(6,37): error CS1763: 'x' is of type 'System.Collections.Generic.IEnumerable<char>'. A default parameter value of a reference type other than string can only be initialized with null
There was a problem hiding this comment.
The whole block is from the Roslyn.Says("Hello World"); commit. Is there somewhere internal that should be searched as well?
There was a problem hiding this comment.
This line dates from Pilchie's "Hello World" commit.
I guessed so, but I think this is something I would like information from TFS on. Sorry 🙂.
There was a problem hiding this comment.
Ok, so @jcouv and I did some spelunking through many layers of TFS to find that this was added by Eric at least a decade ago. Unless @ericlippert remembers why this originally only checked if the parameter type was object and wants to chime in here, I'd say I'm good with the change, provided VS still builds. This is one I think we want a validation PR for.
There was a problem hiding this comment.
What is the native compiler behavior?
There was a problem hiding this comment.
What is the native compiler behavior?
@AlekseyTs Julien tested it in the bug itself. It was an error: #59789 (comment)
There was a problem hiding this comment.
Good spelunking. I definitely wrote that line of code, and I have no memory of why I added that extra check. I concur that it is unnecessary.
|
I think the change here brings us into conformance with the spec. Currently, the compiler does something certainly wrong here: it gives no error, and silently uses 'null' as the default argument at call sites. Unless something critical is uncovered, I think we should just fix it and bring ourselves into alignment with the native compiler. |
|
@tannergooding raised an interesting scenario in LDM that might be broken by this PR. SharpLab I think in general we don't want implicit default arguments to involve calling user-defined operators such as |
Notably I'm not sure this will break it. This is changing the ability to declare This is different from the relevant IL metadata being present and the compiler interpreting that as given. Would be nice/good to have a test validating things though. |
|
The compiler is supposed to check the argument used in the |
|
Oh my god. I just realized this optional parameter is not even at the end of the parameter list. What. And that we pretty much only disallow the attribute argument if conversion doesn't exist at all. SharpLab What have we done, Tanner.. |
|
Started val build. (instructions for later reference) |
|
The val build succeeded. It shows the correct PR number and commit SHA: |
|
It looks like we succeeded at creating a VS insertion, but you'll still have to click through and confirm that the insertion (i.e. pull request to VS) has passing checks. |
|
Let's sync tomorrow. Here's what I see for https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/382848 |
|
Reviewed the val build this morning, all the checks we care about are green so we are good to go there. |
ghost
modified the milestones:
| @@ -98,3 +98,11 @@ https://github.com/dotnet/roslyn/issues/57750 | |||
| public S() { Y = 0; } // ok | |||
| } | |||
| ``` | |||
|
|
|||
| 7. <a name="7"></a>Before Visual Studio 17.2, the C# compiler would accept incorrect default argument values involving a reference conversion of a string constant, and would emit `null` as the constant value instead of the default value specified in source. In Visual Studio 17.2, this becomes an error. See [roslyn#59806](https://github.com/dotnet/roslyn/pull/59806). | |||
There was a problem hiding this comment.
FYI @BillWagner @dotnet/docs. New C# breaking change incoming in 17.2
There was a problem hiding this comment.
docs change was merged yesterday: dotnet/docs#28498
Fixes #59789
See relevant section of the spec.
Note, this is aligning the behavior with that of
const, which makes sense. In our constant folding logic (FoldConstantConversion), onlynullremains a constant when subjected to a reference conversion. So we produce a diagnostic forconstscenario inGetAndValidateConstantValue.