Skip to content

Commit 663eed7

Browse files
paulirwinclaude
andcommitted
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>
1 parent 81cce2f commit 663eed7

5 files changed

Lines changed: 154 additions & 74 deletions

File tree

src/Lucene.Net.CodeAnalysis.Dev.CodeFixes/LuceneDev4xxx/LuceneDev4002_StackTraceHelperNoInliningCodeFixProvider.cs

Lines changed: 10 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
* limitations under the License.
1717
*/
1818

19-
using System.Collections.Generic;
2019
using System.Collections.Immutable;
2120
using System.Composition;
2221
using System.Linq;
@@ -106,14 +105,14 @@ private static async Task<Solution> AddNoInliningToTargetAsync(
106105
if (member.MethodKind != MethodKind.Ordinary)
107106
continue;
108107

108+
if (NoInliningAttributeHelper.HasNoInliningAttribute(member, methodImplAttrSymbol))
109+
continue;
110+
109111
foreach (var declRef in member.DeclaringSyntaxReferences)
110112
{
111113
if (declRef.GetSyntax(cancellationToken) is not MethodDeclarationSyntax methodDecl)
112114
continue;
113115

114-
var declSemantic = compilation.GetSemanticModel(methodDecl.SyntaxTree);
115-
if (NoInliningAttributeHelper.FindNoInliningAttribute(methodDecl, declSemantic, methodImplAttrSymbol) is not null)
116-
continue;
117116
if (NoInliningAttributeHelper.HasEmptyBody(methodDecl))
118117
continue;
119118
if (NoInliningAttributeHelper.IsInterfaceOrAbstractMethod(methodDecl))
@@ -267,40 +266,17 @@ private static (string? Name, INamedTypeSymbol? TypeFromNameof) ResolveClassRefe
267266

268267
private static INamedTypeSymbol? FindSourceTypeByName(Compilation compilation, string typeName)
269268
{
270-
foreach (var type in EnumerateAllTypes(compilation.Assembly.GlobalNamespace))
271-
{
272-
if (type.Name == typeName)
273-
return type;
274-
}
275-
return null;
276-
}
277-
278-
private static IEnumerable<INamedTypeSymbol> EnumerateAllTypes(INamespaceSymbol ns)
279-
{
280-
foreach (var member in ns.GetMembers())
269+
// Use Roslyn's symbol-name index instead of walking every namespace.
270+
// Restrict to the source assembly so we don't match metadata types.
271+
foreach (var symbol in compilation.GetSymbolsWithName(n => n == typeName, SymbolFilter.Type))
281272
{
282-
if (member is INamedTypeSymbol type)
273+
if (symbol is INamedTypeSymbol type
274+
&& SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, compilation.Assembly))
283275
{
284-
yield return type;
285-
foreach (var nested in EnumerateNestedTypes(type))
286-
yield return nested;
287-
}
288-
else if (member is INamespaceSymbol child)
289-
{
290-
foreach (var t in EnumerateAllTypes(child))
291-
yield return t;
276+
return type;
292277
}
293278
}
294-
}
295-
296-
private static IEnumerable<INamedTypeSymbol> EnumerateNestedTypes(INamedTypeSymbol type)
297-
{
298-
foreach (var nested in type.GetTypeMembers())
299-
{
300-
yield return nested;
301-
foreach (var deeper in EnumerateNestedTypes(nested))
302-
yield return deeper;
303-
}
279+
return null;
304280
}
305281
}
306282
}

src/Lucene.Net.CodeAnalysis.Dev/LuceneDev4xxx/LuceneDev4002_StackTraceHelperNoInliningAnalyzer.cs

Lines changed: 12 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
* limitations under the License.
1717
*/
1818

19-
using System.Collections.Generic;
2019
using System.Collections.Immutable;
2120
using System.Linq;
2221
using Lucene.Net.CodeAnalysis.Dev.Utility;
@@ -32,10 +31,8 @@ namespace Lucene.Net.CodeAnalysis.Dev.LuceneDev4xxx
3231
/// StackTraceHelper.DoesStackTraceContainMethod(className, methodName) overload
3332
/// that lack [MethodImpl(MethodImplOptions.NoInlining)]. Without it the JIT may
3433
/// inline the method out of the stack trace, silently breaking the check.
35-
///
36-
/// This analyzer has no code fix: the diagnostic is reported on the referenced
37-
/// method declaration but is triggered by a separate invocation, which Roslyn
38-
/// treats as a non-local diagnostic and does not allow code fixes for.
34+
/// The diagnostic is reported on the invocation so the IDE surfaces it as a
35+
/// local diagnostic and a code fix can apply the attribute to the target method.
3936
/// </summary>
4037
[DiagnosticAnalyzer(LanguageNames.CSharp)]
4138
public sealed class LuceneDev4002_StackTraceHelperNoInliningAnalyzer : DiagnosticAnalyzer
@@ -110,14 +107,14 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext ctx, INamedTypeS
110107
if (member.MethodKind != MethodKind.Ordinary)
111108
continue;
112109

110+
if (NoInliningAttributeHelper.HasNoInliningAttribute(member, methodImplAttrSymbol))
111+
continue;
112+
113113
foreach (var declRef in member.DeclaringSyntaxReferences)
114114
{
115115
if (declRef.GetSyntax(ctx.CancellationToken) is not MethodDeclarationSyntax methodDecl)
116116
continue;
117117

118-
if (NoInliningAttributeHelper.FindNoInliningAttribute(methodDecl, ctx.SemanticModel, methodImplAttrSymbol) is not null)
119-
continue;
120-
121118
if (NoInliningAttributeHelper.HasEmptyBody(methodDecl))
122119
continue;
123120

@@ -192,40 +189,17 @@ private static (string? Name, INamedTypeSymbol? TypeFromNameof) ResolveClassRefe
192189

193190
private static INamedTypeSymbol? FindSourceTypeByName(Compilation compilation, string typeName)
194191
{
195-
foreach (var type in EnumerateAllTypes(compilation.Assembly.GlobalNamespace))
196-
{
197-
if (type.Name == typeName)
198-
return type;
199-
}
200-
return null;
201-
}
202-
203-
private static IEnumerable<INamedTypeSymbol> EnumerateAllTypes(INamespaceSymbol ns)
204-
{
205-
foreach (var member in ns.GetMembers())
192+
// Use Roslyn's symbol-name index instead of walking every namespace.
193+
// Restrict to the source assembly so we don't match metadata types.
194+
foreach (var symbol in compilation.GetSymbolsWithName(n => n == typeName, SymbolFilter.Type))
206195
{
207-
if (member is INamedTypeSymbol type)
208-
{
209-
yield return type;
210-
foreach (var nested in EnumerateNestedTypes(type))
211-
yield return nested;
212-
}
213-
else if (member is INamespaceSymbol child)
196+
if (symbol is INamedTypeSymbol type
197+
&& SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, compilation.Assembly))
214198
{
215-
foreach (var t in EnumerateAllTypes(child))
216-
yield return t;
199+
return type;
217200
}
218201
}
219-
}
220-
221-
private static IEnumerable<INamedTypeSymbol> EnumerateNestedTypes(INamedTypeSymbol type)
222-
{
223-
foreach (var nested in type.GetTypeMembers())
224-
{
225-
yield return nested;
226-
foreach (var deeper in EnumerateNestedTypes(nested))
227-
yield return deeper;
228-
}
202+
return null;
229203
}
230204
}
231205
}

src/Lucene.Net.CodeAnalysis.Dev/LuceneDev4xxx/NoInliningAttributeHelper.cs

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ namespace Lucene.Net.CodeAnalysis.Dev.LuceneDev4xxx
2525
{
2626
public static class NoInliningAttributeHelper
2727
{
28+
private const int NoInlining = 0x0008;
29+
2830
public static AttributeSyntax? FindNoInliningAttribute(
2931
MethodDeclarationSyntax methodDecl,
3032
SemanticModel semantic,
@@ -46,6 +48,27 @@ public static class NoInliningAttributeHelper
4648
return null;
4749
}
4850

51+
// Symbol-based variant: works across syntax trees and avoids
52+
// Compilation.GetSemanticModel (which would trip RS1030 in an analyzer).
53+
public static bool HasNoInliningAttribute(
54+
IMethodSymbol method,
55+
INamedTypeSymbol methodImplAttrSymbol)
56+
{
57+
foreach (var attr in method.GetAttributes())
58+
{
59+
if (!SymbolEqualityComparer.Default.Equals(attr.AttributeClass, methodImplAttrSymbol))
60+
continue;
61+
62+
if (attr.ConstructorArguments.Length == 0)
63+
continue;
64+
65+
var first = attr.ConstructorArguments[0];
66+
if (first.Value is int intValue && (intValue & NoInlining) == NoInlining)
67+
return true;
68+
}
69+
return false;
70+
}
71+
4972
private static bool AttributeSpecifiesNoInlining(AttributeSyntax attr, SemanticModel semantic)
5073
{
5174
if (attr.ArgumentList is null || attr.ArgumentList.Arguments.Count == 0)
@@ -61,11 +84,10 @@ private static bool AttributeSpecifiesNoInlining(AttributeSyntax attr, SemanticM
6184
var constant = semantic.GetConstantValue(firstPositional.Expression);
6285
if (constant.HasValue && constant.Value is int intValue)
6386
{
64-
const int NoInlining = 0x0008;
6587
return (intValue & NoInlining) == NoInlining;
6688
}
6789

68-
return firstPositional.Expression.ToString().Contains("NoInlining");
90+
return false;
6991
}
7092

7193
public static bool IsInterfaceOrAbstractMethod(MethodDeclarationSyntax methodDecl)

tests/Lucene.Net.CodeAnalysis.Dev.CodeFixes.Tests/LuceneDev4xxx/TestLuceneDev4002_StackTraceHelperNoInliningCodeFixProvider.cs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,64 @@ public void Check()
165165
await test.RunAsync();
166166
}
167167

168+
[Test]
169+
public async Task Fix_AddsAttributeAndUsing_InTargetDocument_WhenTargetIsInDifferentFile()
170+
{
171+
// Target lives in /0/Test0.cs, Caller in /0/Test1.cs. The code fix must
172+
// edit the target's document (adding the attribute and the using) rather
173+
// than the caller's.
174+
var targetSource = @"
175+
public class Target
176+
{
177+
public void Merge()
178+
{
179+
var x = 1;
180+
}
181+
}
182+
";
183+
184+
var callerSource = @"
185+
public class Caller
186+
{
187+
public void Check()
188+
{
189+
if (Lucene.Net.Support.ExceptionHandling.StackTraceHelper.DoesStackTraceContainMethod(nameof(Target), nameof(Target.Merge)))
190+
{
191+
}
192+
}
193+
}" + StackTraceHelperStub;
194+
195+
var fixedTargetSource = @"using System.Runtime.CompilerServices;
196+
197+
public class Target
198+
{
199+
[MethodImpl(MethodImplOptions.NoInlining)]
200+
public void Merge()
201+
{
202+
var x = 1;
203+
}
204+
}
205+
";
206+
207+
var expected = new DiagnosticResult(Descriptors.LuceneDev4002_MissingNoInlining)
208+
.WithSeverity(DiagnosticSeverity.Warning)
209+
.WithLocation("/0/Test1.cs", line: 6, column: 13)
210+
.WithArguments("Target.Merge");
211+
212+
var test = new InjectableCodeFixTest(
213+
() => new LuceneDev4002_StackTraceHelperNoInliningAnalyzer(),
214+
() => new LuceneDev4002_StackTraceHelperNoInliningCodeFixProvider())
215+
{
216+
ExpectedDiagnostics = { expected }
217+
};
218+
test.TestState.Sources.Add(targetSource);
219+
test.TestState.Sources.Add(callerSource);
220+
test.FixedState.Sources.Add(fixedTargetSource);
221+
test.FixedState.Sources.Add(callerSource);
222+
223+
await test.RunAsync();
224+
}
225+
168226
[Test]
169227
public async Task Fix_PreservesExistingAttributeOnTarget()
170228
{

tests/Lucene.Net.CodeAnalysis.Dev.Tests/LuceneDev4xxx/TestLuceneDev4002_StackTraceHelperNoInliningAnalyzer.cs

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,56 @@ public void Check()
145145
await test.RunAsync();
146146
}
147147

148+
[Test]
149+
public async Task LuceneDev4002_Reports_When_TargetMethod_Is_In_Different_Document()
150+
{
151+
// The Target type and the Caller live in separate source files. The analyzer
152+
// must build a SemanticModel against the Target's syntax tree before
153+
// inspecting attributes — using the caller's SemanticModel against a node
154+
// from a different tree throws ArgumentException. Target carries an
155+
// unrelated attribute so the attribute-walk path actually executes.
156+
var targetSource = @"
157+
using System;
158+
159+
[AttributeUsage(AttributeTargets.Method)]
160+
public class FooAttribute : Attribute { }
161+
162+
public class Target
163+
{
164+
[Foo]
165+
public void Merge()
166+
{
167+
var x = 1;
168+
}
169+
}
170+
";
171+
172+
var callerSource = @"
173+
public class Caller
174+
{
175+
public void Check()
176+
{
177+
if (Lucene.Net.Support.ExceptionHandling.StackTraceHelper.DoesStackTraceContainMethod(nameof(Target), nameof(Target.Merge)))
178+
{
179+
}
180+
}
181+
}" + StackTraceHelperStub;
182+
183+
var expected = new DiagnosticResult(Descriptors.LuceneDev4002_MissingNoInlining)
184+
.WithSeverity(DiagnosticSeverity.Warning)
185+
.WithLocation("/0/Test1.cs", line: 6, column: 13)
186+
.WithArguments("Target.Merge");
187+
188+
var test = new InjectableCSharpAnalyzerTest(() => new LuceneDev4002_StackTraceHelperNoInliningAnalyzer())
189+
{
190+
ExpectedDiagnostics = { expected }
191+
};
192+
test.TestState.Sources.Add(targetSource);
193+
test.TestState.Sources.Add(callerSource);
194+
195+
await test.RunAsync();
196+
}
197+
148198
[Test]
149199
public async Task LuceneDev4002_NoDiagnostic_For_SingleArgOverload()
150200
{

0 commit comments

Comments
 (0)