Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotate methods that get MethodBase from a stackwalk as RUC #59851

Merged
merged 3 commits into from
Oct 4, 2021

Conversation

MichalStrehovsky
Copy link
Member

Fixes #53242. Saddened this didn't make .NET 6.0.

Stack traces in trimmed apps look ugly without this fix:

Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'id')
   at System.TimeZoneInfo.ValidateTimeZoneInfo(String , TimeSpan , AdjustmentRule[] , Boolean& )
   at System.TimeZoneInfo..ctor(String , TimeSpan , String , String , String , AdjustmentRule[] , Boolean , Boolean )
   at System.TimeZoneInfo.CreateCustomTimeZone(String , TimeSpan , String , String )
   at Program.<Main>$(String[] args) in C:\stacktrace\Program.cs:line 3

Fixes dotnet#53242. Saddened this didn't make .NET 6.0.

Stack traces in trimmed apps look ugly without this fix:

```
Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'id')
   at System.TimeZoneInfo.ValidateTimeZoneInfo(String , TimeSpan , AdjustmentRule[] , Boolean& )
   at System.TimeZoneInfo..ctor(String , TimeSpan , String , String , String , AdjustmentRule[] , Boolean , Boolean )
   at System.TimeZoneInfo.CreateCustomTimeZone(String , TimeSpan , String , String )
   at Program.<Main>$(String[] args) in C:\stacktrace\Program.cs:line 3
```
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Oct 1, 2021

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

Issue Details

Fixes #53242. Saddened this didn't make .NET 6.0.

Stack traces in trimmed apps look ugly without this fix:

Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'id')
   at System.TimeZoneInfo.ValidateTimeZoneInfo(String , TimeSpan , AdjustmentRule[] , Boolean& )
   at System.TimeZoneInfo..ctor(String , TimeSpan , String , String , String , AdjustmentRule[] , Boolean , Boolean )
   at System.TimeZoneInfo.CreateCustomTimeZone(String , TimeSpan , String , String )
   at Program.<Main>$(String[] args) in C:\stacktrace\Program.cs:line 3
Author: MichalStrehovsky
Assignees: -
Labels:

area-AssemblyLoader-coreclr, new-api-needs-documentation

Milestone: -

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@@ -301,8 +303,12 @@ internal void ToString(TraceFormat traceFormat, StringBuilder sb)
if (pi[j].ParameterType != null)
typeName = pi[j].ParameterType.Name;
sb.Append(typeName);
sb.Append(' ');
sb.Append(pi[j].Name);
string? parameterName = pi[j].Name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible future improvement: I vaguely remember discussing this: Would it make sense to add RUC onto the ParameterInfo.Name? I know it can be null even without trimming, but it is an observable difference with trimming.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ILLink will not strip parameter names from things that are visible targets of reflection. By adding RUC on all APIs that get MethodBase from a stack walk, we cover all scenarios where stripping parameter names could be an observable difference (it's the API that lets you get hold of a broken MethodBase in the first place).

Since there will be a warning on getting the MethodBase, we don't need another warning on Name.

This sets us also up to do attribute stripping correctly, similar to how we strip the parameter names: dotnet/linker#2078 - we can remove known "useless" custom attributes from methods/fields that are not visible targets of reflection for the same reason it's safe to strip parameter names and it will not cause any warningless runtime behavior differences.

@MichalStrehovsky MichalStrehovsky merged commit b72b8a2 into dotnet:main Oct 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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