Skip to content

Disallow arrays of System.Void - #94835

Merged
jkotas merged 1 commit into
dotnet:mainfrom
jkotas:void_array
Nov 17, 2023
Merged

Disallow arrays of System.Void#94835
jkotas merged 1 commit into
dotnet:mainfrom
jkotas:void_array

Conversation

@jkotas

@jkotasjkotas commented Nov 16, 2023•
edited
Loading

Copy link
Copy Markdown
Member

The CoreCLR type loader allowed creating arrays of System.Void. Many operations with these invalid array types failed, often in inscrutable ways. For example, GetInterfaces() call failed with type load exception of IEnumerable<void> type. The exact failure modes are different between runtimes.

It is better to disallow creating these invalid array types in the first place, across all runtimes, to make the behavior robust and consistent.

Related to #88620

Fixes#94086

@ghostghost assigned jkotasNov 16, 2023
@jkotasjkotas added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Nov 16, 2023
@ghostghost added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Nov 16, 2023
@ghost

ghost commented Nov 16, 2023

Copy link
Copy Markdown

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.

@jkotasjkotas added area-TypeSystem-coreclr and removed needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 16, 2023
@jkotasjkotas mentioned this pull request Nov 16, 2023
Merged
@jkotas

jkotas commented Nov 16, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Nov 16, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

lambdageek
lambdageek reviewed Nov 16, 2023
View reviewed changes
Comment threadsrc/mono/mono/metadata/icall.c Outdated
@jkotas
Disallow arrays of System.Void
6773a92
The CoreCLR type loader allowed creating arrays of System.Void. Many operations with these invalid array types failed, often in inscrutable ways. For example, GetInterfaces() call failed with type load exception of IEnumerable type. The exact failure modes are different between runtimes.
It is better to disallow creating these invalid array types in the first place, across all runtimes, to make the behavior robust and consistent.
Related to dotnet#88620Fixesdotnet#94086
jkotas
jkotas commented Nov 17, 2023
View reviewed changes
Comment threadsrc/mono/mono/metadata/class-init.c
klass->rank = GUINT32_TO_UINT8 (rank);
klass->element_class = eclass;

if (m_class_get_byval_arg (eclass)->type == MONO_TYPE_TYPEDBYREF) {

@jkotasjkotasNov 17, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

TypedReference is byreflike type, so it is not necessary to check for it explicitly here.

@jkotas
jkotas marked this pull request as ready for review November 17, 2023 01:38
@VSadov

VSadov commented Nov 17, 2023

Copy link
Copy Markdown
Member

I thought this was always disallowed.

VSadov
VSadov approved these changes Nov 17, 2023
View reviewed changes

@VSadovVSadov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM !!

@VSadov

VSadov commented Nov 17, 2023

Copy link
Copy Markdown
Member

For example, GetInterfaces() call failed with type load exception of IEnumerable<void> type.

Does that work with open generic types or pointers?

@jkotas

jkotas commented Nov 17, 2023

Copy link
Copy Markdown
MemberAuthor

Does that work with open generic types or pointers?

Yes, it does. Pointers are special cased here:

runtime/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

Line 959 in 45978bd

if(!arrayType.IsPointer)

This was referenced Nov 17, 2023
Open
Closed
@vargaz

vargaz commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

The mono changes look ok to me.

@jkotas
jkotas merged commit 8b04e1a into dotnet:mainNov 17, 2023
@jkotas
jkotas deleted the void_array branch November 17, 2023 18:53
@build-analysisbuild-analysisBot mentioned this pull request Nov 17, 2023
Closed
@jkotas

jkotas commented Nov 18, 2023

Copy link
Copy Markdown
MemberAuthor

Breaking change issue dotnet/docs#38311

@adamsitnikadamsitnik added this to the 9.0.0 milestone Nov 20, 2023
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@VSadovVSadovVSadov approved these changes
@vargazvargazAwaiting requested review from vargaz
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek

Assignees

@jkotasjkotas

Labels

area-TypeSystem-coreclrbreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

[Mono] System.Runtime.Tests fail with TypeLoadException

5 participants

@jkotas@VSadov@vargaz@lambdageek@adamsitnik