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

Array index from end in ValueTuple assignment throws IndexOutOfRangeException #58472

Closed
zmj opened this issue Dec 23, 2021 · 5 comments · Fixed by #58752
Closed

Array index from end in ValueTuple assignment throws IndexOutOfRangeException #58472

zmj opened this issue Dec 23, 2021 · 5 comments · Fixed by #58752
Assignees
Labels
Area-Compilers Test Test failures in roslyn-CI
Milestone

Comments

@zmj
Copy link

zmj commented Dec 23, 2021

Description

Assignment from tuple destructuring into a FromEnd index of a generic array throws IndexOutOfRangeException. It looks like stelem is called with the Index's raw value, instead of a computed value from the array length.

Reproduction Steps

M(new int[1]);

void M<T>(T[] a)
{
    (a[0], a[^1]) = (default, default);
}

Expected behavior

Assignment succeeds

Actual behavior

IndexOutOfRangeException

Regression?

No response

Known Workarounds

No response

Configuration

.NET 6.0.101

Other information

No response

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@333fred 333fred transferred this issue from dotnet/runtime Dec 23, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 23, 2021
@CyrusNajmabadi
Copy link
Member

Yup. I repro this, and it's super odd. @jcouv ?

@reflectronic
Copy link
Contributor

Seems like there are two issues at play. Roslyn is emitting invalid IL for the second assignment:

.maxstack 4
.locals init (
    [0] !!T[],
    [1] valuetype [System.Private.CoreLib]System.Index,
    [2] !!T
)

// ...
IL_001b: ldloc.0
IL_001c: ldloc.1 // <---- Suspicious
IL_001d: ldloca.s 2
IL_001f: initobj !!T
IL_0025: ldloc.2
IL_0026: stelem !!T

The stelem opcode requires that the value pushed for index (which I emphasized in the above example with a comment) must be "of type native int or int32" (§III.4.26). However, the value pushed by ldloc.1 is a System.Index. So, aside from Roslyn emitting invalid code, there is a secondary (and perhaps less critical) bug where the JIT isn't detecting this as an invalid program.

@CyrusNajmabadi
Copy link
Member

repros for:

M(new int[1]);

void M<T>(T[] a)
{
    (int v, a[^1]) = (default, default);
} 

Does not repro for:

M(new int[1]);

void M(int[] a)
{
    (int v, a[^1]) = (default, default);
} 

@Youssef1313
Copy link
Member

Youssef1313 commented Dec 23, 2021

Doesn't reproduce in main branch. A test-only change to prevent regression can close this.

@jaredpar jaredpar added Area-Compilers Bug and removed Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 3, 2022
@jaredpar jaredpar added this to the 17.1 milestone Jan 3, 2022
@jcouv jcouv added Test Test failures in roslyn-CI 4 - In Review A fix for the issue is submitted for review. and removed Bug labels Jan 6, 2022
jcouv added a commit that referenced this issue Jan 12, 2022
@jcouv jcouv removed the 4 - In Review A fix for the issue is submitted for review. label Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants