Skip to content

Commit 36841b1

Browse files
paulirwinclaude
andauthored
Implement LuceneDev4000-4002 NoInlining Analyzers & CodeFix (#1097) (apache#30)
* Implement LuceneDev4000-4002 NoInlining Analyzers & CodeFix (#1097) Adds three Roslyn analyzers under the new Performance category covering [MethodImpl(MethodImplOptions.NoInlining)] usage: - LuceneDev4000: Reports when NoInlining is applied to an interface or abstract method. The MethodImpl attribute is not inherited, so the attribute has no effect on the implementation. - LuceneDev4001: Reports when NoInlining is applied to an empty-bodied method. An empty body cannot appear above any frame in a stack trace, so preventing inlining provides no benefit. - LuceneDev4002: Reports when a method referenced by the 2-argument StackTraceHelper.DoesStackTraceContainMethod(className, methodName) overload is missing NoInlining. Without it, the JIT may inline the method out of the stack trace, silently breaking the check. A code fix is provided for 4000 and 4001 (remove the attribute). 4002 has no automated fix because the diagnostic is reported on a method declaration triggered by an invocation in another scope, which Roslyn treats as a non-local diagnostic and refuses to fix automatically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Split LuceneDev4002 into its own analyzer class LuceneDev4002 (StackTraceHelper-driven NoInlining requirement) is a distinct rule from 4000/4001 (NoInlining-as-no-op detection): it has a different trigger (invocation vs. method declaration) and no code fix. Extract it into its own analyzer + test class. Shared logic for recognising the [MethodImpl(MethodImplOptions.NoInlining)] attribute, empty bodies, and interface/abstract methods moves into NoInliningAttributeHelper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add Sample project entries for LuceneDev4000-4002 Demonstrates each NoInlining diagnostic firing in the sample project: 4000 on an interface and an abstract method, 4001 on an empty-bodied method, and 4002 on a method referenced by the 2-arg StackTraceHelper.DoesStackTraceContainMethod overload (with a local stub mirroring the real type so the sample compiles standalone). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Preserve leading comments and trim attribute indent in 4000/4001 fix The previous remove-attribute logic used SyntaxRemoveOptions.KeepNoTrivia which discarded any comments or blank lines that preceded the attribute, or KeepLeadingTrivia which left a stray indent on the next line. Replace with an approach that operates on the parent member declaration directly: strip the attribute list while moving its leading comments (minus the final whitespace block, which was just the attribute's own indent) onto the next member's leading trivia. Adds tests covering: leading comment preservation, removing one of several attributes inside a single attribute list, and removing one attribute list among multiple sibling lists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Report LuceneDev4002 at the call site instead of the target method Reporting on the target method declaration produced a non-local diagnostic — the analyzer was visiting an InvocationExpressionSyntax but raising the diagnostic on a syntax tree it had not visited. MSBuild ran a full compilation and surfaced the warning, but the IDE's per-file live analysis filtered it out, so the warning never appeared in the editor. Move the report to the DoesStackTraceContainMethod invocation. The diagnostic is now local, shows up in the IDE, and opens the door to a code fix at the call site. The message names the qualified target (e.g. 'Target.Merge') so it remains clear which method needs the attribute. Test span updated accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add code fix for LuceneDev4002 Adds [MethodImpl(MethodImplOptions.NoInlining)] to the target method referenced by a DoesStackTraceContainMethod call. The fix resolves the target from the call's nameof() (or string-literal) arguments, locates its declaration in source, and edits that document — even when it lives in a different file from the call. Adds 'using System.Runtime.CompilerServices;' to the target's compilation unit if missing, and prepends the new attribute list ahead of any existing attributes on the method. Promote NoInliningAttributeHelper from internal to public so the code fix project can reuse it. Adds tests covering: target with using already present, target without the using, and target with an existing attribute. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Match source line-ending convention in 4002 code fix The fix previously emitted '\n' line terminators unconditionally, which matched local Mac checkouts (LF) but failed on Linux CI where the checked-out source uses CRLF. Detect the existing line ending from the target tree's trivia and reuse it so the fixed output stays consistent with the surrounding file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Simplify 4002 code fix and rely on Formatter for layout Replaces hand-rolled trivia construction with a single Formatter pass. The new attribute list and using directive are built without trivia and formatted via Formatter.FormatAsync, with the workspace NewLine option explicitly set from the source file's existing line endings so the output doesn't mix CRLF and LF on platforms where Environment.NewLine disagrees with the file (e.g. CRLF source on Linux CI). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address Copilot review on LuceneDev4002 Use a symbol-based attribute check (IMethodSymbol.GetAttributes) instead of inspecting AttributeSyntax with a SemanticModel from the wrong tree. The previous code threw "Syntax node is not within syntax tree" whenever the target method lived in a different file from the call site and had any attribute. Symbol-based lookup also avoids Compilation.GetSemanticModel in the analyzer (which trips RS1030). Also: - Drop the Contains("NoInlining") string fallback in the syntax-based helper; the constant-value path already resolves MethodImplOptions.NoInlining and the fallback false-positives on identifiers like "NotNoInlining". - Replace the namespace walk in FindSourceTypeByName with Compilation.GetSymbolsWithName to leverage Roslyn's symbol index. - Update the analyzer's XML doc — the PR adds a code fix; the previous comment claimed there was none. - Add cross-document tests for both analyzer and code fix; the analyzer test reproduces the SemanticModel bug above. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 66f539c commit 36841b1

14 files changed

Lines changed: 2067 additions & 6 deletions
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
using System.Collections.Immutable;
20+
using System.Composition;
21+
using System.Linq;
22+
using System.Threading;
23+
using System.Threading.Tasks;
24+
using Lucene.Net.CodeAnalysis.Dev.Utility;
25+
using Microsoft.CodeAnalysis;
26+
using Microsoft.CodeAnalysis.CodeActions;
27+
using Microsoft.CodeAnalysis.CodeFixes;
28+
using Microsoft.CodeAnalysis.CSharp;
29+
using Microsoft.CodeAnalysis.CSharp.Syntax;
30+
31+
namespace Lucene.Net.CodeAnalysis.Dev.CodeFixes.LuceneDev4xxx
32+
{
33+
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(LuceneDev4000_4001_NoInliningOnNoOpCodeFixProvider)), Shared]
34+
public sealed class LuceneDev4000_4001_NoInliningOnNoOpCodeFixProvider : CodeFixProvider
35+
{
36+
private const string TitleRemoveAttribute = "Remove [MethodImpl(MethodImplOptions.NoInlining)]";
37+
38+
public override ImmutableArray<string> FixableDiagnosticIds =>
39+
ImmutableArray.Create(
40+
Descriptors.LuceneDev4000_NoInliningHasNoEffect.Id,
41+
Descriptors.LuceneDev4001_NoInliningOnEmptyMethod.Id);
42+
43+
public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
44+
45+
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
46+
{
47+
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
48+
if (root is null)
49+
return;
50+
51+
var diagnostic = context.Diagnostics[0];
52+
var node = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true);
53+
54+
var attribute = node as AttributeSyntax
55+
?? node.FirstAncestorOrSelf<AttributeSyntax>();
56+
if (attribute is null)
57+
return;
58+
59+
context.RegisterCodeFix(
60+
CodeAction.Create(
61+
TitleRemoveAttribute,
62+
ct => RemoveAttributeAsync(context.Document, attribute, ct),
63+
equivalenceKey: nameof(TitleRemoveAttribute) + diagnostic.Id),
64+
diagnostic);
65+
}
66+
67+
private static async Task<Document> RemoveAttributeAsync(
68+
Document document,
69+
AttributeSyntax attribute,
70+
CancellationToken cancellationToken)
71+
{
72+
var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
73+
if (root is null)
74+
return document;
75+
76+
if (attribute.Parent is not AttributeListSyntax attrList)
77+
return document;
78+
79+
SyntaxNode newRoot;
80+
if (attrList.Attributes.Count > 1)
81+
{
82+
// [Foo, MethodImpl(NoInlining), Bar] → [Foo, Bar]
83+
var newList = attrList.WithAttributes(attrList.Attributes.Remove(attribute));
84+
newRoot = root.ReplaceNode(attrList, newList);
85+
return document.WithSyntaxRoot(newRoot);
86+
}
87+
88+
// Removing the whole [ ... ] list.
89+
//
90+
// Trivia handling: the attribute list's leading trivia is typically
91+
// (newline)(indent)[comment(newline)(indent)]*. We want to keep any
92+
// comments (and the newline that ends each one) but drop the final
93+
// whitespace block — which is just the indentation for the now-removed
94+
// attribute. The token following the list already carries its own
95+
// newline+indent, so leaving that whitespace in would double-indent the
96+
// next line. We move the trimmed trivia onto the next token.
97+
var leading = attrList.GetLeadingTrivia();
98+
int trim = leading.Count;
99+
while (trim > 0 && leading[trim - 1].IsKind(SyntaxKind.WhitespaceTrivia))
100+
{
101+
trim--;
102+
}
103+
var triviaToKeep = SyntaxFactory.TriviaList(leading.Take(trim));
104+
105+
// Locate the parent that owns this attribute list. Use the parent's
106+
// AttributeLists collection (e.g. on MethodDeclarationSyntax) so that
107+
// removing the list and re-attaching trivia happens in a single step
108+
// and preserves indentation of the surrounding declaration.
109+
if (attrList.Parent is MemberDeclarationSyntax member)
110+
{
111+
var newAttrLists = member.AttributeLists.Remove(attrList);
112+
MemberDeclarationSyntax newMember = member.WithAttributeLists(newAttrLists);
113+
114+
// Prepend the trivia we want to keep (e.g. comments) to the new
115+
// first token of the member declaration.
116+
if (triviaToKeep.Count > 0)
117+
{
118+
var firstToken = newMember.GetFirstToken();
119+
var combined = triviaToKeep.AddRange(firstToken.LeadingTrivia);
120+
newMember = newMember.ReplaceToken(firstToken, firstToken.WithLeadingTrivia(combined));
121+
}
122+
123+
newRoot = root.ReplaceNode(member, newMember);
124+
return document.WithSyntaxRoot(newRoot);
125+
}
126+
127+
// Fallback: just remove the list, dropping its trivia.
128+
newRoot = root.RemoveNode(attrList, SyntaxRemoveOptions.KeepNoTrivia)!;
129+
return document.WithSyntaxRoot(newRoot);
130+
}
131+
}
132+
}

0 commit comments

Comments
 (0)