Fix two array-related bugs in Microsoft.CSharp #17810

JonHanna · 2017-04-03T12:48:35Z

Handle dynamic containing non-SZ single-rank arrays.

Fixes #17808

Correct names of array types in dynamic-related error messages.

Fixes #17809

JonHanna · 2017-04-03T12:50:00Z

src/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/Errors/UserStringBuilder.cs

                            // Add [] with (rank-1) commas inside
                            ErrAppendChar('[');

-#if ! CSEE


Does anyone know what this CSEE constant refers to?

I'm guessing the C# expression evaluator. Like below. @VSadov to confirm.
https://github.com/dotnet/corefx/blob/master/src/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/Semantics/Types/AggregateType.cs#L22

In that case, we need to find out why would the result be different?

In this case the CSEE behaviour is what I'd think would be correct for any case (except that it doesn't handle single-ranked non-SZ arrays but the assembly as a whole had no concept of SZ/non-SZ distinction before this PR).

In other cases I'm not sure what to do when I come across it.

It appears to be pieces that supports Expression Evaluator that is used at debug time. They make no sense in the dynamic binder code.
I guess they were just ported together with the rest of the code. Perhaps the initial pass used some automated translator and it picked up everything, or it was expected that there would be a need for a special binder - specifically for EE to call into, but that need never materialized...

Anyways, I am not aware of any CSEE flavor of the runtime binder.
The "CSEE" seems just dead code and can/should be removed.

I'll do that soon, so.

JonHanna · 2017-04-03T12:51:04Z

src/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/SymbolTable.cs

            {
                // Now we return an array of nesting level corresponding to the rank.
-                ctype = _typeManager.GetArray(GetCTypeFromType(t.GetElementType()), t.GetArrayRank());
+                ctype = _typeManager.GetArray(GetCTypeFromType(t.GetElementType()), t.GetArrayRank(), t.IsSZArray);


Is this use of IsSZArray going to be safe when building for netstandard, or do I need to conditionally bring in a kludge for that case?

Ah. It seems I need to kludge.

JonHanna · 2017-04-03T12:55:18Z

src/Microsoft.CSharp/tests/ArrayHandling.cs

+            d[3] = 32;
+            d[-1] = 28;
+            Assert.Equal(32, d[3]);
+            Assert.Equal(28, d[-1]);


This case is an interesting side-effect in that it's not possible to do compile the static equivalent, and yet the usage here seems reasonable and so not worth the explicit prohibition it would require to refuse it.

Also, one can currently do the equivalent with non-zero-based arrays of higher ranks, so prohibiting it here would have to either be a breaking change to that or else inconsistent with it.

VSadov · 2017-04-06T00:58:28Z

src/Microsoft.CSharp/src/Microsoft.CSharp.csproj

+    <DefineConstants Condition="'$(TargetGroup)'=='netcoreapp'">$(DefineConstants);netcoreapp</DefineConstants>
  </PropertyGroup>
  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='netcoreapp-Debug|AnyCPU'" />


The rest of the change seems ok, but not sure about the project/solution changes.
If we need that, perhaps make them a separate change?

Fixes #17808

Fixes #17809

JonHanna · 2017-04-06T01:12:50Z

@VSadov This is now exactly as it was except for the .sln, .csprog and .props files being unchanged.

The #if netcoreapp is still in there, so it can light up the use of the IsSZArray property if that becomes available with a later change.

As discussed at dotnet#17810 (comment)

As discussed at #17810 (comment)

JonHanna · 2017-04-06T11:44:39Z

Test Innerloop OSX10.12 Debug x64 Build and Test please

VSadov

LGTM

safern · 2017-04-06T21:47:10Z

Hey @JonHanna looks like the test SingleRankNonSZArrayIndexed test that you added here is failing on Desktop. Is this a netcoreapp specific test? Difference in behavior?

Microsoft.CSharp.RuntimeBinder.Tests.ArrayHandling.SingleRankNonSZArrayIndexed [FAIL]
        System.Security.VerificationException : Operation could destabilize the runtime.
        Stack Trace:
             at CallSite.Target(Closure , CallSite , Object , Int32 , Int32 )
             at System.Dynamic.UpdateDelegates.UpdateAndExecute3[T0,T1,T2,TRet](CallSite site, T0 arg0, T1 arg1, T2 arg2)
          D:\repos\corefx\src\Microsoft.CSharp\tests\ArrayHandling.cs(28,0): at Microsoft.CSharp.RuntimeBinder.Tests.ArrayHandling.SingleRankNonSZArrayIndexed()

Would you mind in fixing it?

You can repro by running:

build.cmd -framework:netfx
msbuild src\Microsoft.CSharp\tests\Microsoft.CSharp.Tests.csproj /t:rebuildandtest /p:TargetGroup=netfx

JonHanna · 2017-04-06T22:08:14Z

Sure. I'm on phone right now, but should be able to get to my computer within the hour.

safern · 2017-04-06T22:09:36Z

Sure. I'm on phone right now, but should be able to get to my computer within the hour.

Sounds good. Thanks

As discussed at dotnet/corefx#17810 (comment) Commit migrated from dotnet/corefx@9326e91

Fix two array-related bugs in Microsoft.CSharp Commit migrated from dotnet/corefx@06f5653

dnfclas added the cla-already-signed label Apr 3, 2017

JonHanna commented Apr 3, 2017

View reviewed changes

JonHanna force-pushed the dynamic_arrays branch 2 times, most recently from ef26e47 to 3f1ae36 Compare April 3, 2017 13:25

danmoseley requested review from OmarTawfik and VSadov April 4, 2017 00:53

danmoseley added the area-Microsoft.CSharp label Apr 4, 2017

karelz assigned JonHanna, VSadov and OmarTawfik Apr 4, 2017

VSadov reviewed Apr 6, 2017

View reviewed changes

JonHanna added 2 commits April 6, 2017 02:10

Handle dynamic containing non-SZ single-rank arrays.

c5de607

Fixes #17808

Correct names of array types in dynamic-related error messages.

5ee4e67

Fixes #17809

JonHanna force-pushed the dynamic_arrays branch from 3f1ae36 to 5ee4e67 Compare April 6, 2017 01:10

JonHanna added a commit to JonHanna/corefx that referenced this pull request Apr 6, 2017

Remove CSEE code from Microsoft.CSharp

1617b18

As discussed at dotnet#17810 (comment)

JonHanna mentioned this pull request Apr 6, 2017

Remove CSEE code from Microsoft.CSharp #17979

Merged

stephentoub pushed a commit that referenced this pull request Apr 6, 2017

Remove CSEE code from Microsoft.CSharp (#17979)

9326e91

As discussed at #17810 (comment)

VSadov approved these changes Apr 6, 2017

View reviewed changes

VSadov merged commit 06f5653 into dotnet:master Apr 6, 2017

JonHanna deleted the dynamic_arrays branch April 6, 2017 20:09

JonHanna mentioned this pull request Apr 6, 2017

Remove SingleRankNonSZArrayIndexed test. #18038

Merged

karelz modified the milestone: 2.0.0 Apr 8, 2017

JonHanna mentioned this pull request Jan 31, 2020

Indexing of non-sz single-ranked arrays via dynamic dotnet/runtime#20963

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Remove CSEE code from Microsoft.CSharp (dotnet/corefx#17979)

0675496

As discussed at dotnet/corefx#17810 (comment) Commit migrated from dotnet/corefx@9326e91

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#17810 from JonHanna/dynamic_arrays

b684433

Fix two array-related bugs in Microsoft.CSharp Commit migrated from dotnet/corefx@06f5653

Fix two array-related bugs in Microsoft.CSharp #17810

Fix two array-related bugs in Microsoft.CSharp #17810

Uh oh!

Conversation

JonHanna commented Apr 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonHanna commented Apr 6, 2017

Uh oh!

JonHanna commented Apr 6, 2017

Uh oh!

VSadov left a comment

Choose a reason for hiding this comment

Uh oh!

safern commented Apr 6, 2017

Uh oh!

JonHanna commented Apr 6, 2017

Uh oh!

safern commented Apr 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

OmarTawfik Apr 4, 2017 •

edited

Loading

OmarTawfik Apr 4, 2017 •

edited

Loading