-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Disallow converted string as default argument value #59806
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -624,12 +624,11 @@ internal static bool ReportDefaultParameterErrors( | |
| hasErrors = true; | ||
| } | ||
| else if (conversion.IsReference && | ||
| (parameterType.SpecialType == SpecialType.System_Object || parameterType.Kind == SymbolKind.DynamicType) && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line dates from Pilchie's "Hello World" commit.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole block is from the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guessed so, but I think this is something I would like information from TFS on. Sorry 🙂.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the native compiler behavior?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@AlekseyTs Julien tested it in the bug itself. It was an error: #59789 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| (object)defaultExpression.Type != null && | ||
| defaultExpression.Type.SpecialType == SpecialType.System_String || | ||
| conversion.IsBoxing) | ||
| { | ||
| // We don't allow object x = "hello", object x = 123, dynamic x = "hello", etc. | ||
| // We don't allow object x = "hello", object x = 123, dynamic x = "hello", IEnumerable<char> x = "hello", etc. | ||
| // error CS1763: '{0}' is of type '{1}'. A default parameter value of a reference type other than string can only be initialized with null | ||
| diagnostics.Add(ErrorCode.ERR_NotNullRefDefaultParameter, parameterSyntax.Identifier.GetLocation(), | ||
| parameterSyntax.Identifier.ValueText, parameterType); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @BillWagner @dotnet/docs. New C# breaking change incoming in 17.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs change was merged yesterday: dotnet/docs#28498