Skip to content

Commit

Permalink
Merge pull request #43205 from sharwell/slice-analysis
Browse files Browse the repository at this point in the history
Fix 'Use Index' and 'Use Range' analysis for projects with multiple definitions
  • Loading branch information
sharwell authored Apr 17, 2020
2 parents 3ea829b + 303b014 commit 8a39dd2
Show file tree
Hide file tree
Showing 9 changed files with 734 additions and 343 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

using System.Collections.Concurrent;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.Shared.Extensions;

namespace Microsoft.CodeAnalysis.CSharp.UseIndexOrRangeOperator
{
Expand All @@ -17,20 +19,21 @@ internal partial class CSharpUseIndexOperatorDiagnosticAnalyzer
private class InfoCache
{
/// <summary>
/// The System.Index type. Needed so that we only fixup code if we see the type
/// we're using has an indexer that takes an Index.
/// The <see cref="T:System.Index"/> type. Needed so that we only fixup code if we see the type
/// we're using has an indexer that takes an <see cref="T:System.Index"/>.
/// </summary>
[SuppressMessage("Documentation", "CA1200:Avoid using cref tags with a prefix", Justification = "Required to avoid ambiguous reference warnings.")]
public readonly INamedTypeSymbol IndexType;

/// <summary>
/// Mapping from a method like 'MyType.Get(int)' to the Length/Count property for
/// 'MyType' as well as the optional 'MyType.Get(System.Index)' member if it exists.
/// Mapping from a method like <c>MyType.Get(int)</c> to the <c>Length</c>/<c>Count</c> property for
/// <c>MyType</c> as well as the optional <c>MyType.Get(System.Index)</c> member if it exists.
/// </summary>
private readonly ConcurrentDictionary<IMethodSymbol, MemberInfo> _methodToMemberInfo =
new ConcurrentDictionary<IMethodSymbol, MemberInfo>();

public InfoCache(Compilation compilation)
=> IndexType = compilation.GetTypeByMetadataName("System.Index");
=> IndexType = compilation.GetBestTypeByMetadataName("System.Index");

public bool TryGetMemberInfo(IMethodSymbol methodSymbol, out MemberInfo memberInfo)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Threading;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
Expand All @@ -16,26 +17,29 @@ namespace Microsoft.CodeAnalysis.CSharp.UseIndexOrRangeOperator
using static Helpers;

/// <summary>
/// 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)`
/// <para>Analyzer that looks for code like:</para>
///
/// <list type="number">
/// <item><description><c>s[s.Length - n]</c> and offers to change that to <c>s[^n]</c></description></item>
/// <item><description></description><c>s.Get(s.Length - n)</c> and offers to change that to <c>s.Get(^n)</c></item>
/// </list>
///
/// 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.
/// <para>In order to do convert between indexers, the type must look 'indexable'. Meaning, it must
/// have an <see cref="int"/>-returning property called <c>Length</c> or <c>Count</c>, and it must have both an
/// <see cref="int"/>-indexer, and a <see cref="T:System.Index"/>-indexer. In order to convert between methods, the type
/// must have identical overloads except that one takes an <see cref="int"/>, and the other a <see cref="T:System.Index"/>.</para>
///
/// It is assumed that if the type follows this shape that it is well behaved and that this
/// <para>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.
/// could always limit the feature to only work on a whitelist of known safe types.</para>
///
/// 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.
/// <para>Note that this feature only works if the code literally has <c>expr1.Length - expr2</c>. If
/// code has this, and is calling into a method that takes either an <see cref="int"/> or a <see cref="T:System.Index"/>,
/// it feels very safe to assume this is well behaved and switching to <c>^expr2</c> is going to
/// preserve semantics.</para>
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
[SuppressMessage("Documentation", "CA1200:Avoid using cref tags with a prefix", Justification = "Required to avoid ambiguous reference warnings.")]
internal partial class CSharpUseIndexOperatorDiagnosticAnalyzer : AbstractBuiltInCodeStyleDiagnosticAnalyzer
{
public CSharpUseIndexOperatorDiagnosticAnalyzer()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Concurrent;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.Shared.Extensions;

Expand All @@ -20,15 +21,16 @@ internal partial class CSharpUseRangeOperatorDiagnosticAnalyzer
public class InfoCache
{
/// <summary>
/// The System.Range type. Needed so that we only fixup code if we see the type
/// we're using has an indexer that takes a Range.
/// The <see cref="T:System.Range"/> type. Needed so that we only fixup code if we see the type
/// we're using has an indexer that takes a <see cref="T:System.Range"/>.
/// </summary>
[SuppressMessage("Documentation", "CA1200:Avoid using cref tags with a prefix", Justification = "Required to avoid ambiguous reference warnings.")]
public readonly INamedTypeSymbol RangeType;
private readonly ConcurrentDictionary<IMethodSymbol, MemberInfo> _methodToMemberInfo;

public InfoCache(Compilation compilation)
{
RangeType = compilation.GetTypeByMetadataName("System.Range");
RangeType = compilation.GetBestTypeByMetadataName("System.Range");

_methodToMemberInfo = new ConcurrentDictionary<IMethodSymbol, MemberInfo>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Threading;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
Expand All @@ -17,19 +18,20 @@ namespace Microsoft.CodeAnalysis.CSharp.UseIndexOrRangeOperator
using static Helpers;

/// <summary>
/// Analyzer that looks for several variants of code like `s.Slice(start, end - start)` and
/// offers to update to `s[start..end]` or `s.Slice(start..end)`. In order to convert to the
/// <para>Analyzer that looks for several variants of code like <c>s.Slice(start, end - start)</c> and
/// offers to update to <c>s[start..end]</c> or <c>s.Slice(start..end)</c>. 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
/// an instance of the same type. It also needs a <c>Length</c>/<c>Count</c> property, as well as an indexer
/// that takes a <see cref="T: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.
/// <see cref="T:System.Range"/>.</para>
///
/// It is assumed that if the type follows this shape that it is well behaved and that this
/// <para>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.
/// could always limit the feature to only work on a whitelist of known safe types.</para>
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
[SuppressMessage("Documentation", "CA1200:Avoid using cref tags with a prefix", Justification = "Required to avoid ambiguous reference warnings.")]
internal partial class CSharpUseRangeOperatorDiagnosticAnalyzer : AbstractBuiltInCodeStyleDiagnosticAnalyzer
{
// public const string UseIndexer = nameof(UseIndexer);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,22 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;

namespace Microsoft.CodeAnalysis.CSharp.UseIndexOrRangeOperator
{
internal readonly struct MemberInfo
{
/// <summary>
/// The Length/Count property on the type. Must be public, non-static, no-parameter,
/// int32 returning.
/// The <c>Length</c>/<c>Count</c> property on the type. Must be public, non-static, no-parameter,
/// <see cref="int"/>-returning.
/// </summary>
public readonly IPropertySymbol LengthLikeProperty;

/// <summary>
/// Optional paired overload that takes a Range/Index parameter instead.
/// Optional paired overload that takes a <see cref="T:System.Range"/>/<see cref="T:System.Index"/> parameter instead.
/// </summary>
[SuppressMessage("Documentation", "CA1200:Avoid using cref tags with a prefix", Justification = "Required to avoid ambiguous reference warnings.")]
public readonly IMethodSymbol OverloadedMethodOpt;

public MemberInfo(
Expand Down
Loading

0 comments on commit 8a39dd2

Please sign in to comment.