StackTraceHelper Code Analysis Rules

[paulirwin]

Is there an existing issue for this?

• I have searched the existing issues

Task description

StackTraceHelper is a public type, so we should make these Roslyn analyzer rules part of our public analyzers. Alternatively, if we feel like this type should not be public, we can move these to our dev analyzers instead. This type does not exist in Lucene.

• For StackTraceHelper.DoesStackTraceContainMethod(string className, string methodName), warn if the className argument does not use the nameof operator, and likewise for methodName. There are some legitimate uses of this that can be suppressed, but it should be done deliberately.
• Similar to the one above, error if the className and methodName are constant expressions (like nameof) and the method does not exist on the type. This will help prevent accidentally referencing a method that would result in the call always being false.
• Warn if methods referenced by these calls (via constant expressions like nameof) are not marked [MethodImpl(MethodImplOptions.NoInlining)] if the method body is not empty (overlaps with NoInlining Dev Analyzer Rules #1097)

[NightOwl888]

Thanks. I wasn't aware that StackTraceHelper was public and we definitely shouldn't be encouraging users to write tests that check the stack trace, so let's mark it internal.

Of course, if we can come up with an alternative to checking the stack trace for places where Lucene used it, suggestions are welcome. Sometimes the method we are looking for is several levels up the stack from where we are looking for it.

Using the nameof() operator assumes that we know the specific method that is supposed to be calling us, but generally we don't. We only have the strings that we got from Lucene. We could take a best guess, but that would be misleading if we guess wrong. Of course, now that we have running tests, we can probably pause or do logging and work it out in the more trivial cases, but it is going to take more time to do than suppressing the warning.

Since we have dropped support for .NET Standard 1.x which had a lot of code to work around the lack of stack trace support, maybe we should eliminate the StackTraceHelper and put the translated code inline. It would improve performance in some cases, for example in TestConcurrentMergeScheduler, Lucene has:

Putting the stack trace code inline would allow us to check the stack for both strings at once and exit as soon as both are found.

But as you can see, these are some really common method names. Without knowing specifically which one Lucene is scanning for, we must put [MethodImpl(MethodImplOptions.NoInlining)] on all Flush methods to ensure the test will pass. I think in this case we looked up which specific Dispose() method required no inlining, since it would be impractical and cause performance issues if we did that to all of them.