Should we mark MethodBase.GetCurrentMethod() and friends RequiresUnreferencedCode?

[MichalStrehovsky]

Illink started trimming bits of metadata off things that are not seen as reflected on. We have APIs that allow reflecting on basically everything in the program (e.g. MethodBase.GetCurrentMethod()). One of the bits illinker strips is parameter names.

While MethodBase.GetCurrentMethod is not "trim unsafe" per se, the MethodInfos returned from it might be "damaged" and will result in different runtime behavior. We have been tagging places like that so that the developer can decide whether it's acceptable. Tagging ParameterInfo.Name as trim-unsafe feels like unnecessarily big hammer - it will work if the MethodBase was retrieved through more standard means. It feels more appropriate to just tag MethodBase.GetCurrentMethod and friends as RequiresUnreferenced.

E.g. the Illinker optimization is going to cause different behavior in this code. I expect we're fine with a different behavior there, but it should be a conscious decision, not a random result of an unsafe optimization. E.g. we might want to revise what happens when the parameter name was null in that codepath.

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs

Lines 286 to 303 in 57bdb95

	// arguments printing
	sb.Append('(');
	bool fFirstParam = true;
	for (int j = 0; j < pi.Length; j++)
	{
	if (!fFirstParam)
	sb.Append(", ");
	else
	fFirstParam = false;
	string typeName = "<UnknownType>";
	if (pi[j].ParameterType != null)
	typeName = pi[j].ParameterType.Name;
	sb.Append(typeName);
	sb.Append(' ');
	sb.Append(pi[j].Name);
	}
	sb.Append(')');

This would apply to APIs like: Exception.TargetSite and StackFrame.GetMethod too.

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Illink started trimming bits of metadata off things that are not seen as reflected on. We have APIs that allow reflecting on basically everything in the program (e.g. MethodBase.GetCurrentMethod()). One of the bits illinker strips is parameter names.

While MethodBase.GetCurrentMethod is not "trim unsafe" per se, the MethodInfos returned from it might be "damaged" and will result in different runtime behavior. We have been tagging places like that so that the developer can decide whether it's acceptable. Tagging ParameterInfo.Name as trim-unsafe feels like unnecessarily big hammer - it will work if the MethodBase was retrieved through more standard means. It feels more appropriate to just tag MethodBase.GetCurrentMethod and friends as RequiresUnreferenced.

E.g. the Illinker optimization is going to cause different behavior in this code. I expect we're fine with a different behavior there, but it should be a conscious decision, not a random result of an unsafe optimization. E.g. we might want to revise what happens when the parameter name was null in that codepath.

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs

Lines 286 to 303 in 57bdb95

	// arguments printing
	sb.Append('(');
	bool fFirstParam = true;
	for (int j = 0; j < pi.Length; j++)
	{
	if (!fFirstParam)
	sb.Append(", ");
	else
	fFirstParam = false;
	string typeName = "<UnknownType>";
	if (pi[j].ParameterType != null)
	typeName = pi[j].ParameterType.Name;
	sb.Append(typeName);
	sb.Append(' ');
	sb.Append(pi[j].Name);
	}
	sb.Append(')');

This would apply to APIs like: Exception.TargetSite and StackFrame.GetMethod too.

Author:	MichalStrehovsky
Assignees:	-
Labels:	linkable-framework
Milestone:	6.0.0

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @CoffeeFlux, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Illink started trimming bits of metadata off things that are not seen as reflected on. We have APIs that allow reflecting on basically everything in the program (e.g. MethodBase.GetCurrentMethod()). One of the bits illinker strips is parameter names.

While MethodBase.GetCurrentMethod is not "trim unsafe" per se, the MethodInfos returned from it might be "damaged" and will result in different runtime behavior. We have been tagging places like that so that the developer can decide whether it's acceptable. Tagging ParameterInfo.Name as trim-unsafe feels like unnecessarily big hammer - it will work if the MethodBase was retrieved through more standard means. It feels more appropriate to just tag MethodBase.GetCurrentMethod and friends as RequiresUnreferenced.

E.g. the Illinker optimization is going to cause different behavior in this code. I expect we're fine with a different behavior there, but it should be a conscious decision, not a random result of an unsafe optimization. E.g. we might want to revise what happens when the parameter name was null in that codepath.

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs

Lines 286 to 303 in 57bdb95

	// arguments printing
	sb.Append('(');
	bool fFirstParam = true;
	for (int j = 0; j < pi.Length; j++)
	{
	if (!fFirstParam)
	sb.Append(", ");
	else
	fFirstParam = false;
	string typeName = "<UnknownType>";
	if (pi[j].ParameterType != null)
	typeName = pi[j].ParameterType.Name;
	sb.Append(typeName);
	sb.Append(' ');
	sb.Append(pi[j].Name);
	}
	sb.Append(')');

This would apply to APIs like: Exception.TargetSite and StackFrame.GetMethod too.

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-AssemblyLoader-coreclr, linkable-framework, untriaged
Milestone:	6.0.0

[vitek-karas]

I think it makes sense to annotate these. We'll see how widespread the usage is. If it's too bad we may consider supporting some of these view instrinsics if possible to avoid warnings (For example the GetCurrentMethod is something linker can figure out and make "safe" always).