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.

Sign up for GitHub

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

Jump to bottom

ActivatorUtilities not depending on ctor order for creating instances #75846

Merged
merged 9 commits into from
Oct 13, 2022
Merged

ActivatorUtilities not depending on ctor order for creating instances #75846

merged 9 commits into from
Oct 13, 2022

Conversation

maryamariyan
Copy link
Member

Went over existing test cases and the ones presented in PR #67493 by @mapogolions and adding some more use cases to investigate behavioral change with this fix.

Fixes #46132

@ghost ghost assigned maryamariyan Sep 19, 2022
@ghost
Copy link

ghost commented Sep 19, 2022

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

Went over existing test cases and the ones presented in PR #67493 by @mapogolions and adding some more use cases to investigate behavioral change with this fix.

Fixes #46132

Author: maryamariyan
Assignees: -
Labels:

area-Extensions-DependencyInjection
Milestone: -

@maryamariyan maryamariyan marked this pull request as ready for review September 20, 2022 22:09
@runfoapp runfoapp bot mentioned this pull request Sep 27, 2022
Open
@maryamariyan
Copy link
Member Author

maryamariyan commented Sep 29, 2022 •
edited
Loading

Lamar seems broken.

Code:
https://github.com/JasperFx/lamar/blob/v8.0.0/src/Lamar.Microsoft.DependencyInjection/HostBuilderExtensions.cs#L37-L53

It's not properly setting IServiceProviderIsService. Therefore I'm gonna keep it disabled in my test.

Related discussion: JasperFx/lamar#355

eerhardt
eerhardt previously approved these changes Sep 30, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This LGTM. Thanks for the good work here @maryamariyan!

We can update the other external providers in a separate PR.

  • Lamar once their bug is fixed.
  • Grace once a new stable version is released
  • DryIoc should be able to be updated now, and remove the SupportsIServiceProviderIsService override, but that can be a separate PR.

...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs Outdated Show resolved Hide resolved
@eerhardt eerhardt dismissed their stale review October 3, 2022 21:57

We should have better logic when IServiceProviderIsService is either not registered, or gives false-negatives

@maryamariyan maryamariyan force-pushed the create-instance branch 2 times, most recently from b53faa2 to 1a428c7 Compare October 7, 2022 03:30
@build-analysis build-analysis bot mentioned this pull request Oct 11, 2022
Closed
2 tasks
tarekgh
tarekgh approved these changes Oct 12, 2022
View reviewed changes
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM!

Should we mark this PR as a breaking change to file a doc for it?

@maryamariyan maryamariyan added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Oct 12, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 12, 2022
@ghost
Copy link

ghost commented Oct 12, 2022 •
edited by maryamariyan
Loading

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

eerhardt
eerhardt approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the great work here, @maryamariyan!

@maryamariyan maryamariyan merged commit f58cee2 into dotnet:main Oct 13, 2022
@maryamariyan maryamariyan deleted the create-instance branch October 13, 2022 21:03
@maryamariyan maryamariyan mentioned this pull request Oct 14, 2022
Closed
2 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2022
@ericstj ericstj added this to the 8.0.0 milestone Jun 28, 2023
@ericstj ericstj assigned steveharter and unassigned maryamariyan Oct 16, 2023
@steveharter steveharter removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tillig tillig tillig left review comments

@alistairjevans alistairjevans alistairjevans left review comments

@halter73 halter73 halter73 approved these changes

@eerhardt eerhardt eerhardt approved these changes

@tarekgh tarekgh tarekgh approved these changes

Assignees

@steveharter steveharter

Labels
area-Extensions-DependencyInjection breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

ActivatorUtilities.CreateInstance depends on the order of constructors' definitions
8 participants
@maryamariyan @halter73 @tillig @eerhardt @tarekgh @alistairjevans @steveharter @ericstj