Skip to content

Avoiding discarding exception details of MissingMethodException in RuntimeType.CreateInstanceImpl#108876

Merged
steveharter merged 26 commits into
dotnet:mainfrom
Takym:AvoidingDiscardingExceptionDetails/2024_10
Oct 24, 2024
Merged

Avoiding discarding exception details of MissingMethodException in RuntimeType.CreateInstanceImpl#108876
steveharter merged 26 commits into
dotnet:mainfrom
Takym:AvoidingDiscardingExceptionDetails/2024_10

Conversation

@Takym

@Takym Takym commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

I found the code that try { ... } catch (MissingMethodException) { invokeMethod = null; } and immediately later, re-throwing MissingMethodException if invokeMethod was null. This code removes stack trace information. So, I think debugging or analyzing gets difficult. I kept the code throwing MissingMethodException because binder.BindToMethod may return null.

@Takym Takym requested a review from marek-safar as a code owner October 15, 2024 08:32
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Oct 15, 2024
Comment thread src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
Comment thread src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs Outdated
Comment thread src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated

@steveharter steveharter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good; thanks!

Comment thread src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ActivatorTests.cs Outdated
Takym and others added 2 commits October 19, 2024 00:55
…/ActivatorTests.cs

Co-authored-by: Steve Harter <steveharter@users.noreply.github.com>
…/ActivatorTests.cs

Co-authored-by: Steve Harter <steveharter@users.noreply.github.com>
Comment thread src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ActivatorTests.cs Outdated
…/ActivatorTests.cs

Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>
@am11

am11 commented Oct 21, 2024

Copy link
Copy Markdown
Member

@Takym just a heads-up: we’re not required to keep the PR branch in sync with the latest changes in main. Once the review starts, the area owners will handle merging main if they deem it necessary (usually depending on how old the branch is). Given that this is a busy repository, there's a high chance that something will merge into main before the CI run even completes so we always see the Update button.

@steveharter

Copy link
Copy Markdown
Contributor

Takym the changes look OK to me; do you plan any more commits? If not, I'll merge pending tests.

@Takym

Takym commented Oct 21, 2024

Copy link
Copy Markdown
Contributor Author

I think this PR is completed.

@steveharter steveharter merged commit f21f51f into dotnet:main Oct 24, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Reflection community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants