Skip to content

Commit 9da8ef8

Browse files
authored
Merge pull request #970 from polyadic/analyzer-prefer-named-function
Add analyzer λ1017: prefer the named function over the lambda
2 parents a1b3d6f + 911b1ed commit 9da8ef8

21 files changed

Lines changed: 1035 additions & 43 deletions

File tree

‎Documentation/src/SUMMARY.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,10 @@
5151
* [λ1008](./analyzer-rules/λ1008.md)
5252
* [λ1009](./analyzer-rules/λ1009.md)
5353
* [λ1010](./analyzer-rules/λ1010.md)
54-
* [λ1015](./analyzer-rules/λ1015.md)
5554
* [λ1014](./analyzer-rules/λ1014.md)
55+
* [λ1015](./analyzer-rules/λ1015.md)
5656
* [λ1016](./analyzer-rules/λ1016.md)
57+
* [λ1017](./analyzer-rules/λ1017.md)
5758
* [λ1101](./analyzer-rules/λ1101.md)
5859

5960
[Changelog](changelog.md)

‎Documentation/src/analyzer-rules/analyzer-rules.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ contains a rule.
5050
| [λ1014](./λ1014.md) | Warning | Throw expression in an eagerly evaluated fallback |
5151
| [λ1015](./λ1015.md) | Info | Fallback is computed eagerly |
5252
| [λ1016](./λ1016.md) | Warning | Pass the fallback value directly |
53+
| [λ1017](./λ1017.md) | Warning | Prefer the named function over the lambda |
5354
| [λ1101](./λ1101.md) | Warning | Assert can be simplified |
5455

5556
## Removed rules
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
# λ1017: Prefer the named function over the lambda
2+
A lambda that does what one of the functions in `Funcky.Functional` does should be replaced by that function.
3+
4+
## Cause
5+
A lambda is written where a named function from `Funcky.Functional` says the same:
6+
7+
| Lambda | Named function |
8+
|--------|----------------|
9+
| `x => x`, `x => { return x; }` | `Identity` |
10+
| `() => true`, `_ => true`, `(_, _) => true`, … | `True` |
11+
| `() => false`, `_ => false`, `(_, _) => false`, … | `False` |
12+
| `() => { }`, `_ => { }`, `(_, _) => { }`, … | `NoOperation` |
13+
14+
The rule applies to any delegate-typed position: arguments of LINQ and Funcky methods, variable initializers, assignments and return values.
15+
16+
Lambdas that are not exactly one of these are left alone: `x => (long)x` converts its parameter, `_ => Flag` returns a constant
17+
that may change, `async _ => { }` returns a task.
18+
19+
The rule only reports when the method group binds the same way as the lambda. `items.Where(_ => true)` is not reported because
20+
`Where` has an overload with an index parameter and `True` has overloads with one and two parameters, which makes `Where(True)`
21+
ambiguous. Lambdas inside expression trees are not reported either, because a method group cannot be converted to an expression tree.
22+
23+
## Reason for rule
24+
The named function states the intent at a glance: `Where(True)` reads as "keep everything", `Switch(none: NoOperation, …)`
25+
as "do nothing in that case". The lambda has to be read to arrive at the same conclusion, and a one-character typo turns
26+
`x => x` into something else entirely.
27+
28+
Since C# 11 the compiler caches the delegate created from a static method group, so there is no runtime cost compared to a lambda.
29+
Before C# 11 the method group allocates a delegate on each evaluation while a non-capturing lambda is cached once,
30+
so on older language versions the rule reports an info instead of a warning.
31+
32+
## How to fix violations
33+
Replace the lambda with the named function. A code fix is available; it adds `using static Funcky.Functional;` when the name
34+
is not yet in scope and uses `Functional.Identity` when the bare name is taken by another member.
35+
36+
## Examples
37+
38+
### Disallowed
39+
```cs
40+
static IEnumerable<int> Example(IEnumerable<Option<int>> options)
41+
=> options.SelectMany(option => option.Select(x => x));
42+
43+
static bool Example(Option<int> option)
44+
=> option.Match(none: false, some: _ => true);
45+
46+
static void Example(Option<string> option)
47+
=> option.Switch(none: () => { }, some: Console.WriteLine);
48+
```
49+
50+
### Allowed
51+
```cs
52+
static IEnumerable<int> Example(IEnumerable<Option<int>> options)
53+
=> options.SelectMany(option => option.Select(Identity));
54+
55+
static bool Example(Option<int> option)
56+
=> option.Match(none: false, some: True);
57+
58+
static void Example(Option<string> option)
59+
=> option.Switch(none: NoOperation, some: Console.WriteLine);
60+
61+
// Where(True) would be ambiguous, so the lambda stays.
62+
static IEnumerable<int> Example(IEnumerable<int> items)
63+
=> items.Where(_ => true);
64+
```
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
using System.Collections.Immutable;
2+
using System.Composition;
3+
using Microsoft.CodeAnalysis;
4+
using Microsoft.CodeAnalysis.CodeActions;
5+
using Microsoft.CodeAnalysis.CodeFixes;
6+
using Microsoft.CodeAnalysis.CSharp;
7+
using Microsoft.CodeAnalysis.CSharp.Syntax;
8+
using Microsoft.CodeAnalysis.Simplification;
9+
using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;
10+
11+
namespace Funcky.Analyzers.Functions;
12+
13+
[Shared]
14+
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(PreferNamedFunctionCodeFix))]
15+
public sealed class PreferNamedFunctionCodeFix : CodeFixProvider
16+
{
17+
private const string FunctionalNamespace = "Funcky";
18+
19+
private const string FunctionalTypeName = "Functional";
20+
21+
public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(PreferNamedFunctionAnalyzer.DiagnosticId);
22+
23+
// The analyzer only reports a lambda when the call also binds identically with every other reported lambda of the
24+
// same call replaced, so the batch fixer cannot combine individually safe replacements into a broken call.
25+
public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
26+
27+
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
28+
{
29+
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
30+
31+
foreach (var diagnostic in context.Diagnostics)
32+
{
33+
if (root?.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true) is AnonymousFunctionExpressionSyntax anonymousFunction
34+
&& diagnostic.Properties.TryGetValue(PreferNamedFunctionAnalyzer.FunctionNameProperty, out var functionName)
35+
&& functionName is not null)
36+
{
37+
context.RegisterCodeFix(
38+
CodeAction.Create(
39+
$"Replace with '{functionName}'",
40+
cancellationToken => ReplaceWithNamedFunctionAsync(context.Document, anonymousFunction, functionName, cancellationToken),
41+
nameof(PreferNamedFunctionCodeFix)),
42+
diagnostic);
43+
}
44+
}
45+
}
46+
47+
private static async Task<Document> ReplaceWithNamedFunctionAsync(Document document, AnonymousFunctionExpressionSyntax anonymousFunction, string functionName, CancellationToken cancellationToken)
48+
{
49+
if (await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) is not { } root
50+
|| await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false) is not { } semanticModel)
51+
{
52+
return document;
53+
}
54+
55+
var (replacement, addUsingStatic) = ReplacementExpression(semanticModel, root, anonymousFunction, functionName);
56+
return document.WithSyntaxRoot(Replace(semanticModel, root, anonymousFunction, replacement.WithTriviaFrom(anonymousFunction), addUsingStatic));
57+
}
58+
59+
/// <summary>The bare name is used when it already binds to <c>Funcky.Functional</c>, or when it binds to nothing and a <c>using static</c>
60+
/// can be added without disturbing the rest of the file. Otherwise the qualified name is used.</summary>
61+
private static (ExpressionSyntax Replacement, bool AddUsingStatic) ReplacementExpression(SemanticModel semanticModel, SyntaxNode root, AnonymousFunctionExpressionSyntax anonymousFunction, string functionName)
62+
{
63+
var symbolInfo = semanticModel.GetSpeculativeSymbolInfo(anonymousFunction.SpanStart, IdentifierName(functionName), SpeculativeBindingOption.BindAsExpression);
64+
var symbols = symbolInfo.Symbol is { } symbol ? ImmutableArray.Create(symbol) : symbolInfo.CandidateSymbols;
65+
66+
return symbols switch
67+
{
68+
{ IsEmpty: true } when !ImportWouldClash(semanticModel, root, anonymousFunction) => (IdentifierName(functionName), AddUsingStatic: true),
69+
{ IsEmpty: false } when symbols.All(IsFunctionalMember) => (IdentifierName(functionName), AddUsingStatic: false),
70+
_ => (ParseExpression($"global::{FunctionalNamespace}.{FunctionalTypeName}.{functionName}").WithAdditionalAnnotations(Simplifier.Annotation), AddUsingStatic: false),
71+
};
72+
}
73+
74+
// `using static Funcky.Functional` imports every member of the type. A bare name elsewhere in the file that matches one of
75+
// them, say a `True()` from another static import, would become ambiguous, so the file is scanned for such names first.
76+
private static bool ImportWouldClash(SemanticModel semanticModel, SyntaxNode root, AnonymousFunctionExpressionSyntax anonymousFunction)
77+
{
78+
if (semanticModel.Compilation.GetFunctionalType() is not { } functionalType)
79+
{
80+
return true;
81+
}
82+
83+
var importedNames = functionalType.GetMembers().Where(member => member.IsStatic && member.DeclaredAccessibility == Accessibility.Public).Select(member => member.Name).ToImmutableHashSet();
84+
85+
return root
86+
.DescendantNodes(node => node != anonymousFunction)
87+
.OfType<IdentifierNameSyntax>()
88+
.Any(identifier => importedNames.Contains(identifier.Identifier.ValueText) && !HasExplicitReceiver(identifier));
89+
}
90+
91+
private static bool HasExplicitReceiver(IdentifierNameSyntax identifier)
92+
=> (identifier.Parent is MemberAccessExpressionSyntax { Name: var name } && name == identifier)
93+
|| (identifier.Parent is QualifiedNameSyntax { Right: var right } && right == identifier)
94+
|| identifier.Parent is MemberBindingExpressionSyntax;
95+
96+
private static bool IsFunctionalMember(ISymbol symbol)
97+
=> symbol.ContainingType is { Name: FunctionalTypeName, ContainingNamespace: { Name: FunctionalNamespace, ContainingNamespace.IsGlobalNamespace: true } };
98+
99+
/// <summary>The <c>using static</c> goes next to the existing usings: into the enclosing namespace when the usings are there, otherwise at the top of the file.</summary>
100+
private static SyntaxNode Replace(SemanticModel semanticModel, SyntaxNode root, AnonymousFunctionExpressionSyntax anonymousFunction, ExpressionSyntax replacement, bool addUsingStatic)
101+
=> (addUsingStatic, anonymousFunction.FirstAncestorOrSelf<BaseNamespaceDeclarationSyntax>(namespaceDeclaration => namespaceDeclaration.Usings.Count > 0)) switch
102+
{
103+
(false, _) => root.ReplaceNode(anonymousFunction, replacement),
104+
(true, { } namespaceDeclaration) => root.ReplaceNodes(
105+
new SyntaxNode[] { anonymousFunction, namespaceDeclaration },
106+
(original, rewritten) => original == anonymousFunction
107+
? replacement
108+
: WithUsingStatic((BaseNamespaceDeclarationSyntax)rewritten, semanticModel, namespaceDeclaration.Usings[0].SpanStart)),
109+
(true, null) => WithUsingStatic((CompilationUnitSyntax)root.ReplaceNode(anonymousFunction, replacement), semanticModel, position: 0),
110+
};
111+
112+
/// <summary>Inserts the directive after the last using that is not inside a <c>#if</c> region, so that it is not swallowed by the
113+
/// condition; when every using is conditional it goes in front of them all. It takes the indentation and line ending of its neighbour.
114+
/// The directive is parsed rather than built from tokens so that it carries no elastic trivia, which the formatter would otherwise
115+
/// normalize together with the surrounding line endings.</summary>
116+
private static TContainer WithUsingStatic<TContainer>(TContainer container, SemanticModel semanticModel, int position)
117+
where TContainer : SyntaxNode
118+
{
119+
var usings = container switch
120+
{
121+
CompilationUnitSyntax compilationUnit => compilationUnit.Usings,
122+
BaseNamespaceDeclarationSyntax namespaceDeclaration => namespaceDeclaration.Usings,
123+
_ => default,
124+
};
125+
var usingDirective = ParseCompilationUnit($"using static {QualifiedFunctionalName(semanticModel, position)};").Usings.Single();
126+
127+
return (usings.LastOrDefault(usingDirective => !IsInsideConditionalRegion(usingDirective)), usings.FirstOrDefault()) switch
128+
{
129+
({ } anchor, _) => container.InsertNodesAfter(anchor, [usingDirective.WithLeadingTrivia(FormattingOnly(anchor.GetLeadingTrivia())).WithTrailingTrivia(FormattingOnly(anchor.GetTrailingTrivia()))]),
130+
(null, { } first) => container.InsertNodesBefore(first, [usingDirective.WithTrailingTrivia(CarriageReturnLineFeed)]),
131+
(null, null) => container switch
132+
{
133+
CompilationUnitSyntax compilationUnit => (TContainer)(SyntaxNode)compilationUnit.AddUsings(usingDirective.WithTrailingTrivia(CarriageReturnLineFeed, CarriageReturnLineFeed)),
134+
BaseNamespaceDeclarationSyntax namespaceDeclaration => (TContainer)(SyntaxNode)namespaceDeclaration.AddUsings(usingDirective.WithTrailingTrivia(CarriageReturnLineFeed, CarriageReturnLineFeed)),
135+
_ => container,
136+
},
137+
};
138+
}
139+
140+
// Inside a namespace that contains its own `Funcky` namespace a relative `Funcky.Functional` would bind to that one.
141+
private static string QualifiedFunctionalName(SemanticModel semanticModel, int position)
142+
=> semanticModel.LookupNamespacesAndTypes(position, name: FunctionalNamespace).Any(symbol => symbol is not INamespaceSymbol { ContainingNamespace.IsGlobalNamespace: true })
143+
? $"global::{FunctionalNamespace}.{FunctionalTypeName}"
144+
: $"{FunctionalNamespace}.{FunctionalTypeName}";
145+
146+
// Counting the directives before the using: more `#if` than `#endif` means it sits inside a region.
147+
private static bool IsInsideConditionalRegion(UsingDirectiveSyntax usingDirective)
148+
=> (usingDirective.Ancestors().LastOrDefault() ?? usingDirective)
149+
.DescendantNodes(descendIntoTrivia: true)
150+
.OfType<DirectiveTriviaSyntax>()
151+
.Where(directive => directive.SpanStart < usingDirective.SpanStart)
152+
.Sum(directive => directive switch
153+
{
154+
IfDirectiveTriviaSyntax => 1,
155+
EndIfDirectiveTriviaSyntax => -1,
156+
_ => 0,
157+
}) > 0;
158+
159+
private static SyntaxTriviaList FormattingOnly(SyntaxTriviaList trivia)
160+
=> TriviaList(trivia.Where(t => t.IsKind(SyntaxKind.WhitespaceTrivia) || t.IsKind(SyntaxKind.EndOfLineTrivia)));
161+
}

0 commit comments

Comments
 (0)