roslyn
roslyn copied to clipboard
dotnet
Bind and emit Primary Constructor
var src1 = @"
In general, why are you not using raw string literals, at least for new tests? #Closed
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:29 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
public void ConstructorSymbol_02([CombinatorialValues("class ", "struct")] string keyword)
"class" (no trailing space) #Closed
In reply to: 1346443971
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:404 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
var src1 = @"
In general, why are you not using raw string literals, at least for new tests?
Personal preference. I got used to tests looking a certain way and a different look distracts me. Especially that, in my opinion, they do not offer much advantage. I do not mind escaping strings and that doesn't bother me even a little. Importantly, I also prefer the repro code to start at the beginning of the line rather than been indented as the rest of the surrounding code.
In reply to: 1346436957
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:29 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
public void ConstructorSymbol_02([CombinatorialValues("class ", "struct")] string keyword)
"class" (no trailing space)
This makes strings the same length and aligns diagnostics position, when we have one. I don't think there is a requirement to remove the space, even when no diagnostics reported.
In reply to: 1346443971
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:404 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
CompileAndVerify(comp, expectedOutput: "42").VerifyDiagnostics(); ;
superfluous semicolon
In reply to: 1346555551
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:2794 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
public void AttributesOnPrimaryConstructorParameters_09_CallerMemberName([CombinatorialValues("class ", "struct")] string keyword)
Why 09 when the previous test is AttributesOnPrimaryConstructorParameters_02?
In reply to: 1346564013
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:2895 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
Assert.Equal(0, analyzer.FireCount3);
Consider adding comments to places where fire counts are zero. At least, I don't understand why is that the case. #Closed
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:3403 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
public void AttributesOnPrimaryConstructorParameters_09_CallerMemberName([CombinatorialValues("class ", "struct")] string keyword)
Why 09 when the previous test is AttributesOnPrimaryConstructorParameters_02?
Even though that might come as a surprise at first. There is no requirement for numbering of tests. There is no requirement for numbers to be contiguous either. I probably kept the number because that was the name of the test for records that I cloned.
In reply to: 1346564013
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:2895 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
Assert.Equal(0, analyzer.FireCount3);
Consider adding comments to places where fire counts are zero. At least, I don't understand why is that the case.
The analyzer tests are clones of same named (without suffix) tests for records and record structs (from RecordTests.cs and RecordStructTests.cs). The behavior is consistent with observed in those tests, adjusting for the fact that members are not generated
In reply to: 1346574403
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:3403 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
Assert.Equal("Base..ctor(System.Int32 X, System.Int32 Y)", model.GetSymbolInfo(baseWithargs).Symbol.ToTestDisplayString());
Is this testing a different GetSymbolInfo() overload to the line above? (It looks like both test GetSymbolInfo(SyntaxNode, CancellationToken).)
In reply to: 1347324742
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:909 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
Assert.Equal("Base..ctor(System.Int32 X, System.Int32 Y)", model.GetSymbolInfo(baseWithargs).Symbol.ToTestDisplayString());
Is this testing a different GetSymbolInfo()
You are correct. The original test for records has the same "problem".
In reply to: 1347324742
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:909 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
Assert.Equal("Base..ctor(System.Int32 X, System.Int32 Y)", model.GetSymbolInfo(baseWithargs).Symbol.ToTestDisplayString());
I think that we are missing a public API that we meant to test. I will add it, but preferably in a separate PR
In reply to: 1347335078
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:909 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
case "System.Runtime.CompilerServices.IsExternalInit":
Are these "IsExternalInit" cases hit?
In reply to: 1347534571
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:4781 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
Diagnostic(ErrorCode.WRN_BadXMLRef, "I1").WithArguments("I1").WithLocation(3, 49),
Is this warning expected? Is the cref not valid?
In reply to: 1347537920
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:5362 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
Diagnostic(ErrorCode.WRN_BadXMLRef, "I1").WithArguments("I1").WithLocation(3, 49),
Is this warning expected? Is the cref not valid?
Yes. See the same warning produced for a regular method below.
In reply to: 1347537920
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:5362 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
case "System.Runtime.CompilerServices.IsExternalInit":
Are these "IsExternalInit" cases hit?
Probably not
In reply to: 1347534571
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:4781 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
@cston
Assert.Equal("Base..ctor(System.Int32 X, System.Int32 Y)", model.GetSymbolInfo(baseWithargs).Symbol.ToTestDisplayString());
Is this testing a different
GetSymbolInfo()overload to the line above? (It looks like both testGetSymbolInfo(SyntaxNode, CancellationToken).)
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:909 in 47bf749. [](commit_id = 47bf749773b240a4f1873c5097655b4b13e6ebab, deletion_comment = False)
You are correct. The original test for records has the same "problem". I think that we are missing a public API that we meant to test. I will add it, but preferably in a separate PR
After taking a closer look. The API I considered as missing is actually present and is tested on the next line. It is true that this line is testing the same API as the previous line, but does it with an input of a different type. I think it is good to cover both input types long term. Given that, it looks like no follow up is necessary.