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

[EXPERIMENTATION ONLY] - Prevent Reflection's MakeGenericType() from Allowing the Instantiation of Abstract Classes #105844

Closed

Conversation

ivdiazsa
Copy link
Member

@ivdiazsa ivdiazsa commented Aug 1, 2024

Original PR is #101963.

This PR aims to experiment with fixing the loophole where MakeGenericType() does not fail when attempting to create an object from an abstract class, and thus resulting in incorrect behavior.

@lambdageek
Copy link
Member

Needs a test

@MichalStrehovsky
Copy link
Member

Port of abandoned PR #101963.

Is there indication the original PR is abandoned? The author looks to be waiting for next steps from area owners - might want to ping the author on the PR. It's not a good look to take someone's work and commit under own name.

I see milestone 9.0 - do we still have runway to do breaking changes?

@ivdiazsa
Copy link
Member Author

ivdiazsa commented Aug 2, 2024

Port of abandoned PR #101963.

Is there indication the original PR is abandoned? The author looks to be waiting for next steps from area owners - might want to ping the author on the PR. It's not a good look to take someone's work and commit under own name.

I see milestone 9.0 - do we still have runway to do breaking changes?

It looks abandoned since it hasn't had any activity for the last 2-3 months. But I will ping the author. Also I marked it as draft for a reason. I wanted to do some experimentation with the CI first. Was planning to initiate the conversation once I had checked what I wanted to check..

@ivdiazsa ivdiazsa changed the title Prevent Reflection's MakeGenericType() from Allowing the Instantiation of Abstract Classes [EXPERIMENTATION ONLY] - Prevent Reflection's MakeGenericType() from Allowing the Instantiation of Abstract Classes Aug 2, 2024
@MichalStrehovsky
Copy link
Member

We have push access to the branches with PRs. So for example if that PR is missing a test and the author doesn't react anymore, it's possible to just push a commit with the test to the branch, even if the author is non-responsive (not everyone can work on our schedules). We tend to do that so that we don't rob people of their contributions. Contributors GitHub handle shows up e.g. here: https://dotnet.microsoft.com/en-us/thanks.

It wasn't clear to me this is for experimentation, I guess I'll see the intent here later.

@ivdiazsa
Copy link
Member Author

ivdiazsa commented Aug 2, 2024

We have push access to the branches with PRs. So for example if that PR is missing a test and the author doesn't react anymore, it's possible to just push a commit with the test to the branch, even if the author is non-responsive (not everyone can work on our schedules). We tend to do that so that we don't rob people of their contributions. Contributors GitHub handle shows up e.g. here: https://dotnet.microsoft.com/en-us/thanks.

It wasn't clear to me this is for experimentation, I guess I'll see the intent here later.

I've rephrased to make it extra clear that it is for experimentation purposes now. And I did not know we could just push to anyone's PR so that's good to know..

@ivdiazsa ivdiazsa closed this Aug 2, 2024
@ivdiazsa ivdiazsa deleted the abstract-classes-can-be-instantiated branch August 2, 2024 20:02
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2024
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.

3 participants