Adding null check for implicit cast from array to Span. #25257

ahsonkhan · 2017-11-15T01:30:00Z

Fixes https://github.com/dotnet/corefx/issues/24409

Edit: Also fixes https://github.com/dotnet/corefx/issues/16730

~~Will depend on the corresponding PR in coreclr (which will be identical to this one and I will submit it after I address the feedback here).~~
Depends on dotnet/coreclr#15044

cc @stephentoub, @KrzysztofCwalina, @benaadams, @ericstj

benaadams

Probably best solution; going through a .ctor would do more work

stephentoub · 2017-11-15T01:59:26Z

What about the cast from ArraySegment?
And the same casts to {ReadOnly}Memory?

stephentoub · 2017-11-15T02:01:22Z

src/System.Memory/tests/Span/CopyTo.cs

@@ -34,6 +34,24 @@ public static void TryCopyToSingle()
        }

        [Fact]
+        public static void TryCopyToDefaultImplicit()
+        {
+            int[] dst = default;


It'd be easier to understand as null instead of default

int[] dst = null;

stephentoub · 2017-11-15T02:02:47Z

src/System.Memory/tests/Span/CopyTo.cs

+            int[] dst = default;
+            Span<int> srcSpan = default;
+            bool success = srcSpan.TryCopyTo(dst);
+            Assert.True(success);


Nit: why store into a bool rather than just asserting the expression?

Assert.True(srcSpan.TryCopyTo(dst));

stephentoub · 2017-11-15T02:03:34Z

src/System.Memory/tests/Span/CopyTo.cs

+            Span<int> srcSpan = default;
+            bool success = srcSpan.TryCopyTo(dst);
+            Assert.True(success);
+        }


How is this test different from the previous one? We don't need a rest that verifies that the C# compiler knows that default for an array is null.

stephentoub · 2017-11-15T02:04:13Z

src/System.Memory/tests/Span/CopyTo.cs

+        {
+            int[] dst = null;
+            Span<int> srcSpan = default;
+            bool success = srcSpan.TryCopyTo(dst);


Why call a method rather than just doing the cast?

int[] dst = null; Span<int> srcSpan = dst; Assert.True(Span<int>.Empty == srcSpan);

Right. Why does that require calling a method? My example does that without TryCopyTo.

Nvm, I see what you mean.

ahsonkhan · 2017-11-15T19:07:14Z

What about the cast from ArraySegment?

I was thinking of adding it but array segment can't be null. However, I realize that the array property can be null (for default value of ArraySegment). Will add the check.

And the same casts to {ReadOnly}Memory?

I will do this in a future PR (want to reduce conflicts in Memory, so will add it after the current PRs go through).

gafter · 2017-11-15T23:20:45Z

This looks like a better fix for https://github.com/dotnet/corefx/issues/16730 than my suggestion on that thread.

ahsonkhan · 2017-11-17T19:29:50Z

@dotnet-bot test Windows x86 Release Build

ahsonkhan · 2017-11-17T20:15:45Z

@dotnet-bot test this please

ahsonkhan · 2017-11-17T21:56:41Z

https://mc.dot.net/#/user/ahsonkhan/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/9d18070b991098114e47a7f378aff5e85d5a2f94/workItem/System.Diagnostics.PerformanceCounter.Tests/analysis/xunit/System.Diagnostics.Tests.PerformanceCounterTests~2FPerformanceCounter_IncrementBy_IncrementByReadOnly

Windows.7.Amd64.Open-Release-x86

System.Diagnostics.PerformanceCounter.Tests xunit System.Diagnostics.Tests.PerformanceCounterTests/PerformanceCounter_IncrementBy_IncrementByReadOnly

   at System.Diagnostics.Tests.Helpers.RetryOnAllPlatforms[T](Func`1 func) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.PerformanceCounter\tests\Helpers.cs:line 70
   at System.Diagnostics.Tests.PerformanceCounterTests.CreateCounterWithCategory(String name, Boolean readOnly, PerformanceCounterCategoryType categoryType) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.PerformanceCounter\tests\PerformanceCounterTests.cs:line 295
   at System.Diagnostics.Tests.PerformanceCounterTests.PerformanceCounter_IncrementBy_IncrementByReadOnly() in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.PerformanceCounter\tests\PerformanceCounterTests.cs:line 228

cc @wtgodbe, @joperezr

https://mc.dot.net/#/user/ahsonkhan/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/9d18070b991098114e47a7f378aff5e85d5a2f94/workItem/System.Drawing.Common.Tests/wilogs

OSX.1013.Amd64.Open:Debug-x64
System.Drawing.Common.Tests

2017-11-17 12:59:38,135: INFO: proc(54): run_and_log_output: Output:    System.Drawing.Printing.Tests.PrinterSettingsTests.Static_InstalledPrinters_ReturnsExpected [SKIP]
2017-11-17 12:59:38,135: INFO: proc(54): run_and_log_output: Output:       Condition(s) not met: \"IsAnyInstalledPrinters\"
2017-11-17 12:59:38,614: INFO: proc(54): run_and_log_output: Output: dotnet(85097,0x700002ad5000) malloc: *** mach_vm_map(size=18446744071562067968) failed (error code=3)
2017-11-17 12:59:38,614: INFO: proc(54): run_and_log_output: Output: *** error: can't allocate region
2017-11-17 12:59:38,614: INFO: proc(54): run_and_log_output: Output: *** set a breakpoint in malloc_error_break to debug
2017-11-17 12:59:38,946: INFO: proc(54): run_and_log_output: Output: 
2017-11-17 12:59:38,947: INFO: proc(54): run_and_log_output: Output: ** (process:85097): WARNING **: Path conversion requested 0 bytes (8388608 x 8388608). Maximum size is 8388608 bytes.
2017-11-17 12:59:38,947: INFO: proc(54): run_and_log_output: Output: 
2017-11-17 12:59:38,947: INFO: proc(54): run_and_log_output: Output: ** (process:85097): WARNING **: Path conversion requested 0 bytes (8388608 x 8388608). Maximum size is 8388608 bytes.
2017-11-17 12:59:45,273: INFO: proc(54): run_and_log_output: Output: /Users/dotnet-bot/dotnetbuild/work/4e9fae2e-8fa2-4437-9bf0-5f0b99a4f598/Work/47ed6a9b-fc87-4405-b204-801f0fe5b385/Unzip/RunTests.sh: line 87: 85097 Segmentation fault: 11  (core dumped) $RUNTIME_PATH/dotnet xunit.console.netcore.exe System.Drawing.Common.Tests.dll -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapptests -notrait category=nonosxtests -notrait category=OuterLoop -notrait category=failing

cc @safern

ahsonkhan · 2017-11-17T21:56:58Z

@dotnet-bot test OSX x64 Debug Build
@dotnet-bot test Windows x86 Release Build

joperezr

LGTM

…x#25257) * Adding null check for implicit cast from array to Span. * Addressing PR feedback - adding check for ArraySegment Commit migrated from dotnet/corefx@8a4934f

Adding null check for implicit cast from array to Span.

994409a

benaadams approved these changes Nov 15, 2017

View reviewed changes

stephentoub reviewed Nov 15, 2017

View reviewed changes

Addressing PR feedback - adding check for ArraySegment

9d18070

ahsonkhan mentioned this pull request Nov 15, 2017

Adding null check for implicit cast from array/arraysegment to Span dotnet/coreclr#15044

Merged

karelz added the area-System.Memory label Nov 16, 2017

karelz assigned ahsonkhan Nov 16, 2017

ahsonkhan merged commit 8a4934f into dotnet:master Nov 18, 2017

ahsonkhan deleted the FixImplicitNull branch November 18, 2017 00:13

joperezr reviewed Nov 18, 2017

View reviewed changes

karelz added this to the 2.1.0 milestone Nov 18, 2017

This was referenced Feb 3, 2018

Changing Span to return default on null instead of throwing. #26807

Closed

Changing Span/Memory to return default on null instead of throwing. dotnet/coreclr#16186

Merged

richlander mentioned this pull request Nov 8, 2020

is null check on Span<T> not supported dotnet/roslyn#49230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding null check for implicit cast from array to Span. #25257

Adding null check for implicit cast from array to Span. #25257

ahsonkhan commented Nov 15, 2017 •

edited

Loading

benaadams left a comment

stephentoub commented Nov 15, 2017

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

ahsonkhan Nov 15, 2017

ahsonkhan commented Nov 15, 2017 •

edited

Loading

gafter commented Nov 15, 2017

ahsonkhan commented Nov 17, 2017

ahsonkhan commented Nov 17, 2017

ahsonkhan commented Nov 17, 2017

ahsonkhan commented Nov 17, 2017

joperezr left a comment

Adding null check for implicit cast from array to Span. #25257

Adding null check for implicit cast from array to Span. #25257

Conversation

ahsonkhan commented Nov 15, 2017 • edited Loading

benaadams left a comment

Choose a reason for hiding this comment

stephentoub commented Nov 15, 2017

stephentoub Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Nov 15, 2017

Choose a reason for hiding this comment

ahsonkhan commented Nov 15, 2017 • edited Loading

gafter commented Nov 15, 2017

ahsonkhan commented Nov 17, 2017

ahsonkhan commented Nov 17, 2017

ahsonkhan commented Nov 17, 2017

ahsonkhan commented Nov 17, 2017

joperezr left a comment

Choose a reason for hiding this comment

ahsonkhan commented Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

stephentoub Nov 15, 2017 •

edited

Loading

ahsonkhan commented Nov 15, 2017 •

edited

Loading