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

Add analyzer RCS0062, put expression body on its own line (#1575) #1593

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cbersch
Copy link

@cbersch cbersch commented Dec 6, 2024

Fixes #1575

Add new analyzer to put an existing expression body on its own line.

@cbersch
Copy link
Author

cbersch commented Dec 6, 2024

@dotnet-policy-service agree

* Add option roslynator_expression_body_style_on_next_line to enable breaking of expression bodies on new line.
  The line position is determined by option roslynator_arrow_token_new_line.
@cbersch cbersch force-pushed the put-expression-body-on-its-own-line branch from 89144ec to 736c1a7 Compare December 9, 2024 07:53
@cbersch
Copy link
Author

cbersch commented Dec 9, 2024

@josefpihrt So now I tried to add bool option roslynator_expression_body_style_on_next_line.
The current implementation doesn't work, where I add a new line to the arrow token itself depending on the desired options.
I added one test, which currently fails.

I decided to go with the bool option instead of adding a third value for the expression_body_style, so that the RR0169 can be configured. Otherwise we would need a third refactoring in addition to RR0038 and RR0169 to "Convert block body to expression body on new line" to cover that.

Comment on lines +122 to +125
<Option Id="ExpressionBodyStyleOnNextLine">
<ValuePlaceholder>true|false</ValuePlaceholder>
<Description>Place expression body always on next/same line</Description>
</Option>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain why did you add this option? I think that this option is basically the same as checking if analyzer is enabled or not. If you want to decide whether to wrap line before => in RCS1016 (as an example) just check if RCS0062 is effective or not.

See extensions methods IsEffective for DiagnosticDescriptor.

Copy link
Author

Choose a reason for hiding this comment

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

Basically you are right, but I wouldn't know how to use that information in the CodeFixProvider.
I found usings of DiagnosticDescriptor.IsEffective only in analyzers. The respective information is then unambiguous via the created diagnostic.

But configuring the behavior of a single diagnostic (RCS1016) in the CodeFixProvider is always done via AnalyzerConfigOptions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can definitely get the information whether an analyzer is effective from CodeFixProvider. You need a SyntaxTree, which is property of a node and CompilationOptions which is a property of a Project:

DiagnosticRules.ConfigureAwait.IsEffective(
    node.SyntaxTree,
    context.Document.Project.CompilationOptions,
    cancellationToken);

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I'm still making my way through to understand what to get from where.

So that would work, but now I would have dependency problems:
Currently, my attempt for the (not yet working) line breaking is in Workspaces.Common.ConvertBlockBodyToExpressionBodyRefactoring, but the DiagnosticRules are defined in Formatting.Analyzers.

To know, how to refactor that, I would need to know, how the actual new line is inserted. My approach doesn't work, yet.
Probably I would need to call the SyntaxNode ConvertBlockBodyToExpressionBodyRefactoring.Refactor(SyntaxNode, AnalyzerConfigOptions) from somewhere else, then manipulated the new syntax node, and finally replace the node in the document.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you call IsEffective from code fix provider and then pass it to ConvertBlockBodyToExpressionBodyRefactoring as a parameter? Something like bool wrapLineBeforeArrowToken?

Copy link
Author

@cbersch cbersch Dec 15, 2024

Choose a reason for hiding this comment

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

That also doesn't work, because the RCS0062 DiagnosticRule is defined in Formatting.Analyzers, but the caller of ConvertBlockBodyToExpressionBodyRefactoring.RefactorAsync, which would then have to evaluate the bool, like UseBlockBodyOrExpressionBodyCodeFixProvider (RCS1016) or ConstructorDeclarationRefactoring, can't reference that project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll move DiagnosticRules to Common project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cbersch I moved DiagnosticRules and DiagnosticIdentifiers to Roslynator.Common so now it's accessible from everywhere. Please re-run generate_code.ps1.

Copy link
Author

Choose a reason for hiding this comment

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

Grest, thanks. I'll rework when I'm back from vacaton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Analyzer that checks the breaking behavior of expression bodies
2 participants