Skip to content
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.

Sign up for GitHub

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

Disallow converted string as default argument value #59806

Merged
merged 3 commits into from
Mar 1, 2022
Merged

Disallow converted string as default argument value #59806

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fcb123
Disallow converted string as default argument value
jcouv Feb 28, 2022
4b7e72f
Document break
jcouv Mar 1, 2022
fca972e
typo
jcouv Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,12 +624,11 @@ internal static bool ReportDefaultParameterErrors(
hasErrors = true;
}
else if (conversion.IsReference &&
(parameterType.SpecialType == SpecialType.System_Object || parameterType.Kind == SymbolKind.DynamicType) &&
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole block is from the Roslyn.Says("Hello World"); commit. Is there somewhere internal that should be searched as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂.

Copy link
Member

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the native compiler behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the native compiler behavior?

@AlekseyTs Julien tested it in the bug itself. It was an error: #59789 (comment)

Copy link

Choose a reason for hiding this comment

The 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);
Expand Down
53 changes: 53 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NamedAndOptionalTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2490,5 +2490,58 @@ void M1(object obj = 1) // 1
// C(object obj = System.DayOfWeek.Monday) // 2
Diagnostic(ErrorCode.ERR_NotNullRefDefaultParameter, "obj").WithArguments("obj", "object").WithLocation(8, 14));
}

[Fact, WorkItem(59789, "https://github.com/dotnet/roslyn/issues/59789")]
public void DefaultValue_NonNullConvertedString()
{
var source = @"
using System.Collections.Generic;

class C
{
const IEnumerable<char> y = ""world""; // 1
const string y2 = ""world"";
const object y3 = ""world""; // 2
const dynamic y4 = ""world""; // 3

void M(IEnumerable<char> x = ""hello"") // 4
{
}

void M2(string x = ""hello"")
{
}

void M3(object x = ""hello"") // 5
{
}

void M4(dynamic x = ""hello"") // 6
{
}
}
";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (6,33): error CS0134: 'C.y' is of type 'IEnumerable<char>'. A const field of a reference type other than string can only be initialized with null.
// const IEnumerable<char> y = "world"; // 1
Diagnostic(ErrorCode.ERR_NotNullConstRefField, @"""world""").WithArguments("C.y", "System.Collections.Generic.IEnumerable<char>").WithLocation(6, 33),
// (8,23): error CS0134: 'C.y3' is of type 'object'. A const field of a reference type other than string can only be initialized with null.
// const object y3 = "world"; // 2
Diagnostic(ErrorCode.ERR_NotNullConstRefField, @"""world""").WithArguments("C.y3", "object").WithLocation(8, 23),
// (9,24): error CS0134: 'C.y4' is of type 'dynamic'. A const field of a reference type other than string can only be initialized with null.
// const dynamic y4 = "world"; // 3
Diagnostic(ErrorCode.ERR_NotNullConstRefField, @"""world""").WithArguments("C.y4", "dynamic").WithLocation(9, 24),
// (11,30): error CS1763: 'x' is of type 'IEnumerable<char>'. A default parameter value of a reference type other than string can only be initialized with null
// void M(IEnumerable<char> x = "hello") // 4
Diagnostic(ErrorCode.ERR_NotNullRefDefaultParameter, "x").WithArguments("x", "System.Collections.Generic.IEnumerable<char>").WithLocation(11, 30),
// (19,20): error CS1763: 'x' is of type 'object'. A default parameter value of a reference type other than string can only be initialized with null
// void M3(object x = "hello") // 5
Diagnostic(ErrorCode.ERR_NotNullRefDefaultParameter, "x").WithArguments("x", "object").WithLocation(19, 20),
// (23,21): error CS1763: 'x' is of type 'dynamic'. A default parameter value of a reference type other than string can only be initialized with null
// void M4(dynamic x = "hello") // 6
Diagnostic(ErrorCode.ERR_NotNullRefDefaultParameter, "x").WithArguments("x", "dynamic").WithLocation(23, 21)
);
}
}
}