From 5ba43c34f0ee5d3f05241523f232fb5e3bcb480f Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 21 Oct 2018 17:47:03 -0700 Subject: [PATCH] Simplify and document. --- ...dexOperatorDiagnosticAnalyzer.InfoCache.cs | 10 +++- ...SharpUseIndexOperatorDiagnosticAnalyzer.cs | 44 ++++++++++------- ...ngeOperatorDiagnosticAnalyzer.InfoCache.cs | 10 +++- ...SharpUseRangeOperatorDiagnosticAnalyzer.cs | 47 ++++++++----------- 4 files changed, 61 insertions(+), 50 deletions(-) diff --git a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.InfoCache.cs b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.InfoCache.cs index 1953e523670..0acd9ed95c0 100644 --- a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.InfoCache.cs +++ b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.InfoCache.cs @@ -38,9 +38,15 @@ public InfoCache(Compilation compilation) _methodToMemberInfo[indexer.GetMethod] = ComputeMemberInfo(indexer.GetMethod, requireIndexMember: false); } - public bool TryGetMemberInfo(IMethodSymbol intIndexingMethod, out MemberInfo memberInfo) + public bool TryGetMemberInfo(IMethodSymbol methodSymbol, out MemberInfo memberInfo) { - memberInfo = _methodToMemberInfo.GetOrAdd(intIndexingMethod, m => ComputeMemberInfo(m, requireIndexMember: true)); + if (!IsIntIndexingMethod(methodSymbol)) + { + memberInfo = default; + return false; + } + + memberInfo = _methodToMemberInfo.GetOrAdd(methodSymbol, m => ComputeMemberInfo(m, requireIndexMember: true)); return memberInfo.LengthLikeProperty != null; } diff --git a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.cs b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.cs index dae1f7b4330..39f5c7c1103 100644 --- a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.cs +++ b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseIndexOperatorDiagnosticAnalyzer.cs @@ -2,6 +2,7 @@ using System.Collections.Immutable; using System.Composition; +using System.Threading; using Microsoft.CodeAnalysis.CodeStyle; using Microsoft.CodeAnalysis.CSharp.CodeStyle; using Microsoft.CodeAnalysis.CSharp.Syntax; @@ -10,19 +11,27 @@ namespace Microsoft.CodeAnalysis.CSharp.UseIndexOrRangeOperator { - using System; - using System.Threading; using static Helpers; /// - /// Analyzer that looks for code like `s[s.Length - n]` and offers to change that to `s[^n]`. In - /// order to do this, the type must look 'indexable'. Meaning, it must have an int-returning - /// property called 'Length' or 'Count', and it must have both an int-indexer, and a - /// System.Index indexer. + /// Analyzer that looks for code like: + /// + /// 1) `s[s.Length - n]` and offers to change that to `s[^n]`. and. + /// 2) `s.Get(s.Length - n)` and offers to change that to `s.Get(^n)` + /// + /// In order to do convert between indexers, the type must look 'indexable'. Meaning, it must + /// have an int-returning property called 'Length' or 'Count', and it must have both an + /// int-indexer, and a System.Index-indexer. In order to convert between methods, the type + /// must have identical overloads except that one takes an int, and the other a System.Index. /// /// It is assumed that if the type follows this shape that it is well behaved and that this /// transformation will preserve semantics. If this assumption is not good in practice, we /// could always limit the feature to only work on a whitelist of known safe types. + /// + /// Note that this feature only works if the code literally has `expr1.Length - expr2`. If + /// code has this, and is calling into a method that takes either an int or a System.Index, + /// it feels very safe to assume this is well behaved and switching to `^expr2` is going to + /// preserve semantics. /// [DiagnosticAnalyzer(LanguageNames.CSharp), Shared] internal partial class CSharpUseIndexOperatorDiagnosticAnalyzer : AbstractCodeStyleDiagnosticAnalyzer @@ -38,9 +47,9 @@ protected override void InitializeWorker(AnalysisContext context) { context.RegisterCompilationStartAction(startContext => { - // We're going to be checking every property-reference in the compilation. Cache - // information we compute in this object so we don't have to continually recompute - // it. + // We're going to be checking every property-reference and invocation in the + // compilation. Cache information we compute in this object so we don't have to + // continually recompute it. var infoCache = new InfoCache(startContext.Compilation); context.RegisterOperationAction( c => AnalyzePropertyReference(c, infoCache), @@ -128,9 +137,9 @@ private void AnalyzeInvocation(OperationAnalysisContext context, InfoCache infoC return; } - // look for `s[s.Length - value]` and convert to `s[^val]` + // look for `s[s.Length - value]` or `s.Get(s.Length- value)`. - // Needs to have the one arg for `[s.Length - value]`, and that arg needs to be + // Needs to have the one arg for `s.Length - value`, and that arg needs to be // a subtraction. if (instance is null || arguments.Length != 1 || @@ -139,14 +148,13 @@ private void AnalyzeInvocation(OperationAnalysisContext context, InfoCache infoC return; } - // Ok, looks promising. We're indexing in with some subtraction expression. - // Examine the type this indexer is in to see if there's another meber that - // takes a System.Index that we can convert to. + // Ok, looks promising. We're indexing in with some subtraction expression. Examine the + // type this indexer is in to see if there's another member that takes a System.Index + // that we can convert to. // - // Also ensure that the left side of the subtraction : `s.Length - value` is - // actually getting the length off hte same instance we're indexing into. - if (!IsIntIndexingMethod(targetMethod) || - !infoCache.TryGetMemberInfo(targetMethod, out var memberInfo) || + // Also ensure that the left side of the subtraction : `s.Length - value` is actually + // getting the length off the same instance we're indexing into. + if (!infoCache.TryGetMemberInfo(targetMethod, out var memberInfo) || !IsInstanceLengthCheck(memberInfo.LengthLikeProperty, instance, subtraction.LeftOperand)) { return; diff --git a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.InfoCache.cs b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.InfoCache.cs index 8687eb82d28..2c5700d0582 100644 --- a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.InfoCache.cs +++ b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.InfoCache.cs @@ -45,9 +45,15 @@ private IMethodSymbol GetSliceLikeMethod(INamedTypeSymbol namedType) .Where(m => IsSliceLikeMethod(m)) .FirstOrDefault(); - public bool TryGetMemberInfo(IMethodSymbol sliceLikeMethod, out MemberInfo memberInfo) + public bool TryGetMemberInfo(IMethodSymbol method, out MemberInfo memberInfo) { - memberInfo = _methodToMemberInfo.GetOrAdd(sliceLikeMethod, m => ComputeMemberInfo(m, requireRangeMember: true)); + if (!IsSliceLikeMethod(method)) + { + memberInfo = default; + return false; + } + + memberInfo = _methodToMemberInfo.GetOrAdd(method, m => ComputeMemberInfo(m, requireRangeMember: true)); return memberInfo.LengthLikeProperty != null; } diff --git a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.cs b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.cs index 6d18198e1a0..46614dd14ae 100644 --- a/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.cs +++ b/src/Features/CSharp/Portable/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.cs @@ -15,9 +15,12 @@ namespace Microsoft.CodeAnalysis.CSharp.UseIndexOrRangeOperator /// /// Analyzer that looks for several variants of code like `s.Slice(start, end - start)` and - /// offers to update to `s[start..end]`. In order to convert, the type being called on needs a - /// slice-like method that takes two ints, and returns an instance of the same type. It also - /// needs a Length/Count property, as well as an indexer that takes a System.Range instance. + /// offers to update to `s[start..end]` or `s.Slice(start..end)`. In order to convert to the + /// indexer, the type being called on needs a slice-like method that takes two ints, and returns + /// an instance of the same type. It also needs a Length/Count property, as well as an indexer + /// that takes a System.Range instance. In order to convert between methods, there need to be + /// two overloads that are equivalent except that one takes two ints, and the other takes a + /// System.Range. /// /// It is assumed that if the type follows this shape that it is well behaved and that this /// transformation will preserve semantics. If this assumption is not good in practice, we @@ -83,25 +86,8 @@ protected override void InitializeWorker(AnalysisContext context) return; } - // See if the call is to something slice-like. - var targetMethod = invocation.TargetMethod; - if (!IsSliceLikeMethod(invocation.TargetMethod)) - { - return; - } - var sliceLikeMethod = targetMethod; - // See if this is a type we can use range-indexer for, and also if this is a call to the - // Slice-Like method we've found for that type. Use the InfoCache so that we can reuse - // any previously computed values for this type. - if (!infoCache.TryGetMemberInfo(sliceLikeMethod, out var memberInfo)) - { - return; - } - - // look for `s.Slice(start, end - start)` and convert to `s[Range]` - - // Needs to have the two args for `start` and `end - start` + // look for `s.Slice(e1, end - e2)` if (invocation.Instance is null || invocation.Instance.Syntax is null || invocation.Arguments.Length != 2) @@ -109,21 +95,26 @@ protected override void InitializeWorker(AnalysisContext context) return; } - // Arg2 needs to be a subtraction for: `end - start` - var arg2 = invocation.Arguments[1]; - if (!IsSubtraction(arg2, out var subtraction)) + // See if the call is to something slice-like. + var targetMethod = invocation.TargetMethod; + + // Second arg needs to be a subtraction for: `end - e2`. Once we've seen that we have + // that, try to see if we're calling into some sort of Slice method with a matching + // indexer or overload + if (!IsSubtraction(invocation.Arguments[1], out var subtraction) || + !infoCache.TryGetMemberInfo(targetMethod, out var memberInfo)) { return; } - // See if we have: (start, end - start). The start operation has to be the same as the - // right side of the subtraction. + // See if we have: (start, end - start). Specifically where the start operation it the + // same as the right side of the subtraction. var startOperation = invocation.Arguments[0].Value; if (CSharpSyntaxFactsService.Instance.AreEquivalent(startOperation.Syntax, subtraction.RightOperand.Syntax)) { context.ReportDiagnostic(CreateDiagnostic( - ComputedRange, option, invocationSyntax, sliceLikeMethod, + ComputedRange, option, invocationSyntax, targetMethod, memberInfo, startOperation, subtraction.LeftOperand)); return; } @@ -135,7 +126,7 @@ protected override void InitializeWorker(AnalysisContext context) IsInstanceLengthCheck(memberInfo.LengthLikeProperty, invocation.Instance, subtraction.LeftOperand)) { context.ReportDiagnostic(CreateDiagnostic( - ConstantRange, option, invocationSyntax, sliceLikeMethod, + ConstantRange, option, invocationSyntax, targetMethod, memberInfo, startOperation, subtraction.RightOperand)); return; } -- GitLab