Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Don't ask the type loader to build instantiations over generic type definitions#3478

Merged
MichalStrehovsky merged 2 commits intodotnet:masterfrom
MichalStrehovsky:makeGenType
May 2, 2017
Merged

Don't ask the type loader to build instantiations over generic type definitions#3478
MichalStrehovsky merged 2 commits intodotnet:masterfrom
MichalStrehovsky:makeGenType

Conversation

@MichalStrehovsky
Copy link
Member

Found in CoreFX tests.

// The reflection stack will use this to construct a Type that doesn't have a type handle.
// Note: this is different from the validation we do in EnsureSatisfiesClassConstraints because this
// should not throw.
if (RuntimeAugments.IsGenericTypeDefinition(genericTypeArgumentHandles[i]))
Copy link
Contributor

Choose a reason for hiding this comment

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

We throw a NotSupportedException when trying to create arrays with a generic type def element. Should we keep the behavior consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Seems like that is not the right place to throw the exception either. The CLR allows typeof(List<>).MakeArrayType(); just fine.

We don't know the intent of the higher level caller at this point, so returning false is what the protocol would dictate. It's up to the caller to decide what to do with that.

Cc @atsushikan

Copy link

Choose a reason for hiding this comment

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

Agreed - this is a low-level method that should return a boolean (or Debug.Assert if truly nonsensical input like a null array) but it shouldn't throw exceptions for "bad input."

@MichalStrehovsky MichalStrehovsky merged commit 29d330e into dotnet:master May 2, 2017
@MichalStrehovsky MichalStrehovsky deleted the makeGenType branch May 2, 2017 23:51
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