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.

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

Implement interface tests #54310

Merged
merged 3 commits into from
Jun 23, 2021
Merged

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Jun 23, 2021

No description provided.

@"interface IInterface
{
[return: System.Runtime.CompilerServices.TupleElementNames(new[] { ""a"", ""b"" })]
[return: {|CS8138:System.Runtime.CompilerServices.TupleElementNames(new[] { ""a"", ""b"" })|}]
Copy link
Member Author

Choose a reason for hiding this comment

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

💭 It's not clear that this is an accurate representation.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ #54325

{
p public void Method()
p {|CS1585:public|} void Method()
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 In this case, the code fix is missing because CS0535 is not reported on the ITest identifier. If there is a different syntax which produces incomplete member errors but still produces CS0535, the results might be different than this test.

Options = { AllOptionsOff },

// 🐛 one value is generated with 0U instead of 0
CodeActionValidationMode = CodeActionValidationMode.None,
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 There are a few cases where validation needed to be disabled for this code fix. I'll file a follow-up issue to review them.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ #54329

@@ -5094,7 +5132,7 @@ interface I

class C : I
{
public void Goo([DefaultParameterValue(1), Optional] int x = 1, int[,] y = null)
public void Goo([{|CS1745:DefaultParameterValue|}(1), {|CS1745:Optional|}] int x = {|CS8017:1|}, int[,] y = null)
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 code generation is not correct. I'm guess the result we want is:

Suggested change
public void Goo([{|CS1745:DefaultParameterValue|}(1), {|CS1745:Optional|}] int x = {|CS8017:1|}, int[,] y = null)
public void Goo(int x = 1, int[,] y = null)

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ #54326

Comment on lines +8190 to +8191
// Specify the code action by equivalence key only to avoid trying to implement the interface explicitly with a second code fix pass.
CodeActionEquivalenceKey = "False;False;True:global::IInterface;Assembly1;Microsoft.CodeAnalysis.ImplementInterface.AbstractImplementInterfaceService+ImplementInterfaceCodeAction;",
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 The test library could be improved for this case (only require matching key/index if the key appears somewhere in the list of available code actions, plus require the first pass have a matching pair). I'll file a follow-up issue in dotnet/roslyn-sdk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +9509 to +9512
// /0/Test0.cs(7,11): error CS0535: 'C' does not implement interface member 'ITest.M1()'
DiagnosticResult.CompilerError("CS0535").WithLocation(0).WithArguments("C", "ITest.M1()"),
// /0/Test0.cs(9,16): error CS0539: 'C.M1()' in explicit interface declaration is not found among members of the interface that can be implemented
DiagnosticResult.CompilerError("CS0539").WithLocation(1).WithArguments("C.M1()"),
Copy link
Member Author

Choose a reason for hiding this comment

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

🐛 The generated code is missing a static modifier, which leads to errors. Will file a follow-up issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ #54327

ExpectedDiagnostics =
{
// /0/Test0.cs(9,33): error CS0112: A static member cannot be marked as 'abstract'
DiagnosticResult.CompilerError("CS0112").WithLocation(0).WithArguments("abstract"),
Copy link
Member Author

Choose a reason for hiding this comment

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

🐛 Static members cannot be generated abstractly. Will file a follow-up issue.

Copy link
Member

Choose a reason for hiding this comment

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

@sharwell I believe I already fixed this in a PR that's not yet merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ #54328

@sharwell sharwell changed the base branch from main to main-vs-deps June 23, 2021 06:45
@sharwell sharwell changed the base branch from main-vs-deps to main June 23, 2021 06:46
@sharwell sharwell marked this pull request as ready for review June 23, 2021 06:46
@sharwell sharwell requested a review from a team as a code owner June 23, 2021 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants