Cover test plan for "stackalloc initializers" #25101

alrz · 2018-02-27T20:59:12Z

tracking issues: #24699 and #24983

alrz · 2018-02-27T20:59:47Z

docs/compilers/CSharp/testplans/stackalloc_initializers.md

 - [ ] in the array type inference case the outer expression cannot be used to determine the type - [ ] should be an error and should not be some kind of crash due to circular dependency
 - [ ] make it to infer strange types. 
-	- [ ] dynamic
+	- [x] dynamic
 	- [ ] ref-struct


The test exists but doesn't fail for span due to #25086

We should sync the branch after the fix #25131 goes in. Then we can assert the failure as it is expected.

That's ok. and can be marked as having coverage. The test could be "skipped" for now - due to #25086

In reply to: 171064165 [](ancestors = 171064165)

The branch is severely behind features/compiler if that's alright, I'll extract this test to a skipped one.

I think it is ok as-is. The test is now passing but has a comment that it will fail when merged.

In reply to: 171702476 [](ancestors = 171702476)

I can merge the /compiler, just did not want to interfere with your work. Perhaps will do after this PR?

In reply to: 171712095 [](ancestors = 171712095,171702476)

alrz · 2018-02-27T21:02:06Z

Other than symbol tests (and perhaps IOperation) I'm not sure how to test other unchecked items.

alrz · 2018-02-28T10:25:15Z

/cc @VSadov @jcouv

gafter · 2018-03-01T17:32:52Z

The process we use for test plans is

The "secondary developer" on a feature writes a test plan, typically as an issue
The compiler team reviews the test plan, and decides which elements are required before and after integration
The "secondary developer" checks off covered items on the plan, with assistance from the primary developer, who may develop additional tests to cover the plan.
When the required tests for integration have been completed, integration can happen.
Remaining tests are developed by the primary developer.

Your "secondary developer" for this feature is @VSadov .

@jaredpar It would be nice to add this to our development process doc.

alrz · 2018-03-01T17:36:56Z

Linking to #24699 and #24983

gafter · 2018-03-01T17:55:12Z

@alrz That is an interesting experiment, but as you can see in master there are no test plan docs in the production repositories.

VSadov · 2018-03-01T21:20:29Z

I think we can close this now once #25160 exists.

alrz · 2018-03-01T21:23:34Z

@VSadov this is the actual covering PR.

VSadov · 2018-03-01T21:26:03Z

@alrz - ah I see. I thought this is just to edit the document.

VSadov · 2018-03-01T21:28:03Z

docs/compilers/CSharp/testplans/stackalloc_initializers.md

@@ -10,24 +10,24 @@ In such case we can just check them off.
 ### correctness ###
 - [ ] check that partial result is not visible. - if element throws and exception caught, whole thing is not reassigned to the new value. (try spans and ordinary stackallocs)
 - [ ] if we refer to the elements of outer local in the initializers we still see the old values.
- [ ] put `await` in the size or in the middle of element initializers
+- [x] put `await` in the size or in the middle of element initializers


I think we might want to just delete the file now - to not duplicate where the tracking happens - since we have an issue for this.

It seemed a good idea at a time, but not anymore. Editing a file is not so easy - especially if it needs multiple small edits and by multiple people.

In reply to: 171699064 [](ancestors = 171699064)

VSadov · 2018-03-01T22:03:15Z

src/Compilers/CSharp/Test/Emit/CodeGen/StackAllocInitializerTests.cs

+    static void Main()
+    {
+        var p = stackalloc[] { M(), true ? throw null : M(), M() };
+        Use(p);


~~The "no assignment while evaluating" could be tested with code like:~~

~~I am not sure the same pattern is possible with Span. I think escape rules should not allow it. Then we don't care about that case :-)~~

Ignore the above. That is not possible even with pointers. The reason is that the target variable must be a local.
We can store in local first and then store the local to a field, but that is not interesting.

and we cannot stackalloc in a try and then check if assignment worked by accident before elements were evaluated in the catch

tried a couple of variations with no luck. good to know it wasn't possible 😄

VSadov · 2018-03-01T22:07:05Z

src/Compilers/CSharp/Test/Semantic/Semantics/StackAllocInitializerTests.cs

+{
+    void Method()
+    {
+        var obj1 = stackalloc int[1] { obj1 };


worth also trying {obj1[0], obj1[1]}

VSadov · 2018-03-01T22:09:04Z

src/Compilers/CSharp/Test/Semantic/Semantics/StackAllocInitializerTests.cs

+    {
+        var obj1 = c ? default : stackalloc[] { """" };
+        var obj2 = c ? default : stackalloc[] { new {} };
+        var obj3 = c ? default : stackalloc[] { s }; // Should be an error, see https://github.com/dotnet/roslyn/issues/25086


This looks ok. When we merge the changes with the fix, the test will fails and we will update it.

VSadov · 2018-03-01T22:13:54Z

src/Compilers/CSharp/Test/Semantic/Semantics/StackAllocInitializerTests.cs

+{
+    async void M()
+    {
+        Span<int> p = stackalloc int[await Task.FromResult(1)] { await Task.FromResult(2) };


nice! I thought we might have some work with stack-spilling of initializers/size because of await . It looks like there is no scenario here that would not be a compile error though due to constraints of async methods and unsafe/span.

VSadov · 2018-03-01T22:19:13Z

Had some comments on couple more scenarios, but otherwise looks good.

VSadov

LGTM

alrz · 2018-03-02T22:01:45Z

so far, the title is false advertising.

we can hold this off until the plan is actually reviewed, then I will look into adding more tests. (maybe in follow up PRs, whichever works for you)

VSadov · 2018-03-02T22:24:27Z

@alrz more PRs is fine. I may also add some scenarios to the testplan if could think of something interesting.

The goal of the review is primarily to decide whether the current coverage is adequate for merging the branch.
Some test work always happens later.

VSadov · 2018-03-05T18:51:46Z

Thanks!!

alrz requested a review from a team as a code owner February 27, 2018 20:59

alrz commented Feb 27, 2018

View reviewed changes

alrz changed the title ~~Stackalloc testplan~~ Test plan for "stackalloc initializers" Feb 27, 2018

Add more tests.

1e681a5

alrz force-pushed the stackalloc-init-tests branch from 70f6d49 to 1e681a5 Compare February 27, 2018 22:00

gafter requested a review from VSadov March 1, 2018 17:34

alrz changed the title ~~Test plan for "stackalloc initializers"~~ Cover test plan for "stackalloc initializers" Mar 1, 2018

VSadov reviewed Mar 1, 2018

View reviewed changes

alrz added 2 commits March 2, 2018 15:46

Delete stackalloc_initializers.md

2037b29

PR feedback

a1a7258

VSadov approved these changes Mar 2, 2018

View reviewed changes

jaredpar approved these changes Mar 5, 2018

View reviewed changes

VSadov merged commit 1c292ee into dotnet:features/stackalloc-init Mar 5, 2018

alrz deleted the stackalloc-init-tests branch March 6, 2018 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover test plan for "stackalloc initializers" #25101

Cover test plan for "stackalloc initializers" #25101

alrz commented Feb 27, 2018 •

edited

Loading

alrz Feb 27, 2018 •

edited

Loading

alrz Mar 1, 2018

VSadov Mar 1, 2018

alrz Mar 1, 2018

VSadov Mar 1, 2018

VSadov Mar 1, 2018

alrz commented Feb 27, 2018

alrz commented Feb 28, 2018

gafter commented Mar 1, 2018

alrz commented Mar 1, 2018

gafter commented Mar 1, 2018

VSadov commented Mar 1, 2018

alrz commented Mar 1, 2018

VSadov commented Mar 1, 2018

VSadov Mar 1, 2018

VSadov Mar 1, 2018

VSadov Mar 1, 2018 •

edited

Loading

VSadov Mar 1, 2018 •

edited

Loading

VSadov Mar 2, 2018 •

edited

Loading

alrz Mar 2, 2018

VSadov Mar 1, 2018

VSadov Mar 1, 2018

VSadov Mar 1, 2018 •

edited

Loading

VSadov commented Mar 1, 2018

VSadov left a comment

alrz commented Mar 2, 2018 •

edited

Loading

VSadov commented Mar 2, 2018

VSadov commented Mar 5, 2018

Cover test plan for "stackalloc initializers" #25101

Cover test plan for "stackalloc initializers" #25101

Conversation

alrz commented Feb 27, 2018 • edited Loading

alrz Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz commented Feb 27, 2018

alrz commented Feb 28, 2018

gafter commented Mar 1, 2018

alrz commented Mar 1, 2018

gafter commented Mar 1, 2018

VSadov commented Mar 1, 2018

alrz commented Mar 1, 2018

VSadov commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

VSadov Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

VSadov Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

VSadov commented Mar 1, 2018

VSadov left a comment

Choose a reason for hiding this comment

alrz commented Mar 2, 2018 • edited Loading

VSadov commented Mar 2, 2018

VSadov commented Mar 5, 2018

alrz commented Feb 27, 2018 •

edited

Loading

alrz Feb 27, 2018 •

edited

Loading

VSadov Mar 1, 2018 •

edited

Loading

VSadov Mar 1, 2018 •

edited

Loading

VSadov Mar 2, 2018 •

edited

Loading

VSadov Mar 1, 2018 •

edited

Loading

alrz commented Mar 2, 2018 •

edited

Loading