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

CSharpSyntaxGenerator does not support adding interface members with a default implementation #53605

Open
jkoritzinsky opened this issue May 21, 2021 · 4 comments
Labels
Area-IDE Concept-API This issue involves adding, removing, clarification, or modification of an API.
Milestone

Comments

@jkoritzinsky
Copy link
Member

jkoritzinsky commented May 21, 2021

Version Used: 3.9.0-3.final. The failing code is in main at

case SyntaxKind.MethodDeclaration:
return ((MethodDeclarationSyntax)member)
.WithModifiers(default)
.WithSemicolonToken(SyntaxFactory.Token(SyntaxKind.SemicolonToken))
.WithBody(null);
case SyntaxKind.PropertyDeclaration:
var property = (PropertyDeclarationSyntax)member;
return property
.WithModifiers(default)
.WithAccessorList(WithoutBodies(property.AccessorList));
case SyntaxKind.IndexerDeclaration:
var indexer = (IndexerDeclarationSyntax)member;
return indexer
.WithModifiers(default)
.WithAccessorList(WithoutBodies(indexer.AccessorList));
// convert event into field event
case SyntaxKind.EventDeclaration:
var ev = (EventDeclarationSyntax)member;
return this.EventDeclaration(
ev.Identifier.ValueText,
ev.Type,
Accessibility.NotApplicable,
DeclarationModifiers.None);

Steps to Reproduce:

  1. Given a SyntaxGenerator named generator as part of a code-fix running in C#, an IMethodSymbol named method, and a SyntaxNode named declaration that represents an interface, run the following snippet:
SyntaxNode methodDecl = generator.MethodDeclaration(method);
methodDecl = generator.WithModifiers(methodDecl, generator.GetModifiers(declaration).WithIsAbstract(false));
methodDecl = generator.WithStatements(methodDecl, generator.ThrowStatement(generator.NullLiteralExpression()));
declaration = generator.AddMembers(declaration, methodDecl);

Expected Behavior:

The generated code in declaration will have a method based on method that has the following body:

{
    throw null;
}

Actual Behavior:

The generated code will have an interface member declaration with no body.

Based on the code in Roslyn linked above, this bug will be hit for properties, indexers, and event declarations as well.

This is blocking providing the code fixes as part of dotnet/runtime#41529

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 21, 2021
@CyrusNajmabadi CyrusNajmabadi changed the title CSharpSourceGenerator does not support adding interface members with a default implementation CSharpSyntaxGenerator does not support adding interface members with a default implementation May 22, 2021
@CyrusNajmabadi
Copy link
Member

This is about SyntaxGenerator not SourceGenerator.

@jkoritzinsky
Copy link
Member Author

Sorry about that. Fixed the content of the issue since you already fixed the title.

jkoritzinsky added a commit to jkoritzinsky/roslyn-analyzers that referenced this issue May 24, 2021
@jinujoseph jinujoseph added Concept-Continuous Improvement Concept-API This issue involves adding, removing, clarification, or modification of an API. and removed untriaged Issues and PRs which have not yet been triaged by a lead Concept-Continuous Improvement labels Jun 2, 2021
@jinujoseph jinujoseph added this to the Backlog milestone Jun 2, 2021
@jinujoseph
Copy link
Contributor

Assigning to Cyrus to review the API

@CyrusNajmabadi
Copy link
Member

This is challenging as we've always supported generating interface members by taking a normal member and just adding it to an interface. We toss things like the bodies in this case. So it's very likely that code exists that depends on that behavior. So it could be extremely problematic to change that now.

It's not clear what a viable approach would be here except maybe to allow AddMembers to specify some new behaviors here that can be opted into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Concept-API This issue involves adding, removing, clarification, or modification of an API.
Projects
None yet
Development

No branches or pull requests

3 participants