From eb718aec44a994424ce6157a3a3ddb57d1d1dd65 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 2 Jun 2019 12:52:10 -0700 Subject: [PATCH] Don't offer 'convert if to condition' if it crosses a directive --- .../UseConditionalExpressionForReturnTests.cs | 20 ++++++++++++++ ...ConditionalExpressionDiagnosticAnalyzer.cs | 3 +-- ...seConditionalExpressionForReturnHelpers.cs | 3 +-- .../UseConditionalExpressionHelpers.cs | 27 ++++++++++--------- .../Extensions/SyntaxTokenExtensions.cs | 17 +++++++++--- .../ISyntaxFactsServiceExtensions.cs | 4 +++ 6 files changed, 54 insertions(+), 20 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/UseConditionalExpression/UseConditionalExpressionForReturnTests.cs b/src/EditorFeatures/CSharpTest/UseConditionalExpression/UseConditionalExpressionForReturnTests.cs index d434972507e..4ef7ea8b947 100644 --- a/src/EditorFeatures/CSharpTest/UseConditionalExpression/UseConditionalExpressionForReturnTests.cs +++ b/src/EditorFeatures/CSharpTest/UseConditionalExpression/UseConditionalExpressionForReturnTests.cs @@ -821,6 +821,26 @@ IEnumerable M(int a) { yield return a != 0; } +}"); + } + + [WorkItem(36117, "https://github.com/dotnet/roslyn/issues/36117")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseConditionalExpression)] + public async Task TestMissingWhenCrossingPreprocessorDirective() + { + await TestMissingInRegularAndScriptAsync( +@" +class C +{ + int M() + { + bool check = true; +#if DEBUG + [||]if (check) + return 3; +#endif + return 2; + } }"); } } diff --git a/src/Features/Core/Portable/UseConditionalExpression/AbstractUseConditionalExpressionDiagnosticAnalyzer.cs b/src/Features/Core/Portable/UseConditionalExpression/AbstractUseConditionalExpressionDiagnosticAnalyzer.cs index 9fd6d1c1671..caefac9e5f1 100644 --- a/src/Features/Core/Portable/UseConditionalExpression/AbstractUseConditionalExpressionDiagnosticAnalyzer.cs +++ b/src/Features/Core/Portable/UseConditionalExpression/AbstractUseConditionalExpressionDiagnosticAnalyzer.cs @@ -39,8 +39,7 @@ protected sealed override void InitializeWorker(AnalysisContext context) private void AnalyzeOperation(OperationAnalysisContext context) { var ifOperation = (IConditionalOperation)context.Operation; - var ifStatement = ifOperation.Syntax as TIfStatementSyntax; - if (ifStatement == null) + if (!(ifOperation.Syntax is TIfStatementSyntax ifStatement)) { return; } diff --git a/src/Features/Core/Portable/UseConditionalExpression/ForReturn/UseConditionalExpressionForReturnHelpers.cs b/src/Features/Core/Portable/UseConditionalExpression/ForReturn/UseConditionalExpressionForReturnHelpers.cs index ab21baded58..1beab838316 100644 --- a/src/Features/Core/Portable/UseConditionalExpression/ForReturn/UseConditionalExpressionForReturnHelpers.cs +++ b/src/Features/Core/Portable/UseConditionalExpression/ForReturn/UseConditionalExpressionForReturnHelpers.cs @@ -35,8 +35,7 @@ internal static class UseConditionalExpressionForReturnHelpers if (falseStatement == null) { - var parentBlock = ifOperation.Parent as IBlockOperation; - if (parentBlock == null) + if (!(ifOperation.Parent is IBlockOperation parentBlock)) { return false; } diff --git a/src/Features/Core/Portable/UseConditionalExpression/UseConditionalExpressionHelpers.cs b/src/Features/Core/Portable/UseConditionalExpression/UseConditionalExpressionHelpers.cs index 46c82c121eb..d3b10ac9802 100644 --- a/src/Features/Core/Portable/UseConditionalExpression/UseConditionalExpressionHelpers.cs +++ b/src/Features/Core/Portable/UseConditionalExpression/UseConditionalExpressionHelpers.cs @@ -1,17 +1,8 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; -using System.Collections.Generic; -using System.Collections.Immutable; -using System.Linq; -using System.Threading; -using System.Threading.Tasks; using Microsoft.CodeAnalysis.Editing; -using Microsoft.CodeAnalysis.Formatting; -using Microsoft.CodeAnalysis.Formatting.Rules; using Microsoft.CodeAnalysis.LanguageServices; using Microsoft.CodeAnalysis.Operations; -using Microsoft.CodeAnalysis.Shared.Extensions; namespace Microsoft.CodeAnalysis.UseConditionalExpression { @@ -23,9 +14,21 @@ internal static class UseConditionalExpressionHelpers ISyntaxFactsService syntaxFacts, IConditionalOperation ifOperation, IOperation whenTrue, IOperation whenFalse) { - // Will likely screw things up if the if directive spans any preprocessor directives. - // So do not offer for now. - if (syntaxFacts.SpansPreprocessorDirective(ifOperation.Syntax)) + // Will likely screw things up if the if directive spans any preprocessor directives. So + // do not offer for now. Note: we pass in both the node for the ifOperation and the + // whenFalse portion. The whenFalse portion isn't necessary under the ifOperation. For + // example in: + // + // ```c# + // #if DEBUG + // if (check) + // return 3; + // #endif + // return 2; + // ``` + // + // In this case, we want to see that this cross the `#endif` + if (syntaxFacts.SpansPreprocessorDirective(ifOperation.Syntax, whenFalse.Syntax)) { return false; } diff --git a/src/Workspaces/CSharp/Portable/Extensions/SyntaxTokenExtensions.cs b/src/Workspaces/CSharp/Portable/Extensions/SyntaxTokenExtensions.cs index df01a82df20..5be38c036fe 100644 --- a/src/Workspaces/CSharp/Portable/Extensions/SyntaxTokenExtensions.cs +++ b/src/Workspaces/CSharp/Portable/Extensions/SyntaxTokenExtensions.cs @@ -70,6 +70,13 @@ public static bool IsFirstTokenOnLine(this SyntaxToken token, SourceText text) return tokenLine > previousTokenLine; } + /// + /// Determines if there is preprocessor trivia *between* any of the + /// provided. The will be deduped and then ordered by position. + /// Specifically, the first token will not have it's leading trivia checked, and the last + /// token will not have it's trailing trivia checked. All other trivia will be checked to + /// see if it contains a preprocessor directive. + /// public static bool SpansPreprocessorDirective(this IEnumerable tokens) { // we want to check all leading trivia of all tokens (except the @@ -79,7 +86,11 @@ public static bool SpansPreprocessorDirective(this IEnumerable toke var first = true; var previousToken = default(SyntaxToken); - foreach (var token in tokens) + // Allow duplicate nodes/tokens to be passed in. Also, allow the nodes/tokens + // to not be in any particular order when passed in. + var orderedTokens = tokens.Distinct().OrderBy(t => t.SpanStart); + + foreach (var token in orderedTokens) { if (first) { @@ -103,9 +114,7 @@ public static bool SpansPreprocessorDirective(this IEnumerable toke } private static bool SpansPreprocessorDirective(SyntaxTriviaList list) - { - return list.Any(t => t.GetStructure() is DirectiveTriviaSyntax); - } + => list.Any(t => t.GetStructure() is DirectiveTriviaSyntax); /// /// Retrieves all trivia after this token, including it's trailing trivia and diff --git a/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsServiceExtensions.cs b/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsServiceExtensions.cs index 88ead394556..f502a7a17db 100644 --- a/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsServiceExtensions.cs +++ b/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsServiceExtensions.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis.Shared.Extensions; @@ -119,6 +120,9 @@ private static bool IsWordOrNumber(this ISyntaxFactsService syntaxFacts, SyntaxT public static bool SpansPreprocessorDirective(this ISyntaxFactsService service, SyntaxNode node) => service.SpansPreprocessorDirective(SpecializedCollections.SingletonEnumerable(node)); + public static bool SpansPreprocessorDirective(this ISyntaxFactsService service, params SyntaxNode[] nodes) + => service.SpansPreprocessorDirective(nodes); + public static bool IsWhitespaceOrEndOfLineTrivia(this ISyntaxFactsService syntaxFacts, SyntaxTrivia trivia) => syntaxFacts.IsWhitespaceTrivia(trivia) || syntaxFacts.IsEndOfLineTrivia(trivia); -- GitLab