Implement LuceneDev4000-4002 NoInlining Analyzers & CodeFix (#1097)#30
Open
paulirwin wants to merge 9 commits intoapache:mainfrom
Open
Implement LuceneDev4000-4002 NoInlining Analyzers & CodeFix (#1097)#30paulirwin wants to merge 9 commits intoapache:mainfrom
paulirwin wants to merge 9 commits intoapache:mainfrom
Conversation
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>
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>
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>
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>
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>
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>
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>
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>
There was a problem hiding this comment.
Pull request overview
Adds new LuceneDev4xxx “Performance” analyzers and code fixes to enforce correct [MethodImpl(MethodImplOptions.NoInlining)] usage, including enforcing NoInlining for methods referenced by StackTraceHelper.DoesStackTraceContainMethod(className, methodName).
Changes:
- Introduces analyzers LuceneDev4000/4001 (no-op
NoInliningcases) and LuceneDev4002 (missingNoInliningfor StackTraceHelper-referenced methods), plus shared helper logic. - Adds code fix providers: remove ineffective
NoInlining(4000/4001) and addNoInlining(4002) including adding the requiredusing. - Adds descriptors/resources, samples, analyzer release notes, and NUnit tests for analyzers + code fixes.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Lucene.Net.CodeAnalysis.Dev.Tests/LuceneDev4xxx/TestLuceneDev4002_StackTraceHelperNoInliningAnalyzer.cs | Adds analyzer tests for LuceneDev4002 behavior. |
| tests/Lucene.Net.CodeAnalysis.Dev.Tests/LuceneDev4xxx/TestLuceneDev4000_4001_NoInliningOnNoOpAnalyzer.cs | Adds analyzer tests for LuceneDev4000/4001 behavior. |
| tests/Lucene.Net.CodeAnalysis.Dev.CodeFixes.Tests/LuceneDev4xxx/TestLuceneDev4002_StackTraceHelperNoInliningCodeFixProvider.cs | Adds code-fix tests for adding NoInlining + optional using. |
| tests/Lucene.Net.CodeAnalysis.Dev.CodeFixes.Tests/LuceneDev4xxx/TestLuceneDev4000_4001_NoInliningOnNoOpCodeFixProvider.cs | Adds code-fix tests for removing NoInlining while preserving trivia/sibling attributes. |
| src/Lucene.Net.CodeAnalysis.Dev/Utility/Descriptors.LuceneDev4xxx.cs | Adds descriptors for 4000–4002. |
| src/Lucene.Net.CodeAnalysis.Dev/Resources.resx | Adds localized strings for 4000–4002 diagnostics. |
| src/Lucene.Net.CodeAnalysis.Dev/LuceneDev4xxx/NoInliningAttributeHelper.cs | Adds shared helper for detecting NoInlining, empty bodies, and abstract/interface declarations. |
| src/Lucene.Net.CodeAnalysis.Dev/LuceneDev4xxx/LuceneDev4002_StackTraceHelperNoInliningAnalyzer.cs | Adds analyzer for StackTraceHelper 2-arg overload call sites. |
| src/Lucene.Net.CodeAnalysis.Dev/LuceneDev4xxx/LuceneDev4000_4001_NoInliningOnNoOpAnalyzer.cs | Adds analyzer for ineffective NoInlining usage on declarations. |
| src/Lucene.Net.CodeAnalysis.Dev/AnalyzerReleases.Unshipped.md | Registers new rule IDs 4000–4002 in the unshipped list. |
| src/Lucene.Net.CodeAnalysis.Dev.Sample/LuceneDev4xxx/LuceneDev4002_StackTraceHelperNoInliningSample.cs | Adds sample usage demonstrating LuceneDev4002. |
| src/Lucene.Net.CodeAnalysis.Dev.Sample/LuceneDev4xxx/LuceneDev4000_4001_NoInliningOnNoOpSample.cs | Adds sample usage demonstrating LuceneDev4000/4001. |
| src/Lucene.Net.CodeAnalysis.Dev.CodeFixes/LuceneDev4xxx/LuceneDev4002_StackTraceHelperNoInliningCodeFixProvider.cs | Adds code fix to apply NoInlining to the referenced method (and add using). |
| src/Lucene.Net.CodeAnalysis.Dev.CodeFixes/LuceneDev4xxx/LuceneDev4000_4001_NoInliningOnNoOpCodeFixProvider.cs | Adds code fix to remove only the offending attribute and preserve trivia/siblings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements three Roslyn analyzers in the new Performance category around
[MethodImpl(MethodImplOptions.NoInlining)]usage, addressing apache/lucenenet#1097.NoInliningshould not be used on interface or abstract methods.MethodImplis not inherited, so the attribute has no effect on the implementation.NoInliningshould not be used on empty-bodied methods. An empty body cannot appear above any frame in a stack trace, so preventing inlining provides no benefit and only harms perf.StackTraceHelper.DoesStackTraceContainMethod(className, methodName)overload should be marked[MethodImpl(MethodImplOptions.NoInlining)]when the body is non-empty. Without it, the JIT may inline the method out of the stack trace, silently breaking the check.Code fixes
[MethodImpl(NoInlining)]attribute, preserving any leading comments and removing only the attribute (not its siblings) when multiple attributes are present.[MethodImpl(MethodImplOptions.NoInlining)]to the target method (resolved from the call'snameof()or string-literal arguments), even when the method lives in a different file. Addsusing System.Runtime.CompilerServices;to the target's compilation unit if missing.Class layout
LuceneDev4000_4001_NoInliningOnNoOpAnalyzer+ matching code fix — handles 4000/4001 (NoInlining is a no-op on the declaration).LuceneDev4002_StackTraceHelperNoInliningAnalyzer+ matching code fix — handles 4002. The diagnostic is reported on theDoesStackTraceContainMethodinvocation (not the target method declaration) so that it shows up in IDE live analysis; reporting on the declaration produced a non-local diagnostic that MSBuild surfaced but the IDE filtered out.NoInliningAttributeHelperholds the shared attribute / body / abstract checks used by both analyzers and the 4002 fix.