Adjust IsDiagnosticsHidden + MethodDesc comments

- Include additional stubs in IsDiagnosticsHidden (conceptually makes sense but no concrete bug was verified fixed by this)
- Added clarifying comments on some MethodDesc APIs related to TransientIL and diagnostics.

Co-authored-by: Copilot <[email protected]>
Co-authored-by: Jan Kotas <[email protected]>

Author: 3 people

src/coreclr/vm/method.inl

@@ -109,6 +109,9 @@ inline PTR_DynamicMethodDesc MethodDesc::AsDynamicMethodDesc()
     return dac_cast<PTR_DynamicMethodDesc>(this);
 }
 
+// NOTE: Not all methods that create IL at runtime are considered dynamic methods. This only returns TRUE
+// for methods represented by DynamicMethodDesc. Transient IL (see TryGenerateTransientILImplementation) also
+// generates IL at runtime but returns FALSE here.
 inline bool MethodDesc::IsDynamicMethod()
 {
     LIMITED_METHOD_CONTRACT;
@@ -121,16 +124,35 @@ inline bool MethodDesc::IsLCGMethod()
     return ((mcDynamic == GetClassification()) && dac_cast<PTR_DynamicMethodDesc>(this)->IsLCGMethod());
 }
 
+// NOTE: This method only detects the subset of ILStubs that are internally represented using DynamicMethodDesc.
+// There are other methods compiled from IL generated at runtime via ILStubLinker that still return FALSE here.
+// See TryGenerateTransientILImplementation.
 inline bool MethodDesc::IsILStub()
 {
     WRAPPER_NO_CONTRACT;
     return ((mcDynamic == GetClassification()) && dac_cast<PTR_DynamicMethodDesc>(this)->IsILStub());
 }
 
+// This method is intended to identify methods that aren't shown in diagnostic introspection (stacktraces,
+// code viewing, stepping, etc). Partly this is a user experience consideration to preserve the
+// abstraction users would expect based on source code and assembly contents. Partly it is also a technical
+// limitation that many parts of diagnostics don't know how to work with methods that aren't backed by
+// metadata and IL in a module.
 inline bool MethodDesc::IsDiagnosticsHidden()
 {
+    // Although good user experience can be subjective these are guidelines:
+    // - We don't want stacktraces to show extra frames when the IL only shows a single call was made. If our runtime stackwalker
+    //   is going to include multiple frames pointing at different MethodDescs then all but one of them should be hidden.
+    // - In most cases when multiple MethodDescs occur for the same IL call, one of them is a MethodDesc that
+    //   compiled original IL defined in the module (for its default code version at least). That is the one we want to show
+    //   and the rest should be hidden.
+    // - In other cases the user defines methods in their source code but provides no implementation (for
+    //   example interop calls, Array methods, Delegate.Invoke, or UnsafeAccessor). Ideally a filtered stacktrace would include exactly
+    //   one frame for these calls as well but we haven't always done this consistently. For calls that redirect to another managed method users
+    //   tolerate if the runtime-implemented frame is missing because they can still see the managed target method.
+
     WRAPPER_NO_CONTRACT;
-    return IsILStub() || IsAsyncThunkMethod();
+    return IsILStub() || IsAsyncThunkMethod() || IsWrapperStub();
 }
 
 inline BOOL MethodDesc::IsQCall()

src/coreclr/vm/prestub.cpp

@@ -1057,6 +1057,9 @@ bool MethodDesc::TryGenerateTransientILImplementation(DynamicResolver** resolver
 {
     STANDARD_VM_CONTRACT;
 
+    // When adding new methods implemented by Transient IL, consider if MethodDesc::IsDiagnosticsHidden() needs to be
+    // updated as well.
+
     if (TryGenerateAsyncThunk(resolver, methodILDecoder))
     {
         return true;