Skip to content

Conversation

@TIHan
Copy link
Contributor

@TIHan TIHan commented Jun 8, 2018

This is intended to fix several safety rules with ref returns and span.

@TIHan TIHan force-pushed the ref-span-fixes2 branch 2 times, most recently from 1d915c6 to ae5e5a8 Compare June 8, 2018 17:20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is a bit complicated, but something like this is necessary when we start supporting stack referring span likes, and mix call functions with arguments that have both stack + heap referring span likes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, at this point I may want to return a LimitValFlags.ByRef here.

@TIHan
Copy link
Contributor Author

TIHan commented Jun 9, 2018

I'm using these as tests:

As of this commit: 1359ca7

let mutable beef = 1

let returning_span (x: byref<int>) = Span.Empty

let returning_ref (x: byref<int>) = &x

let returning_ref2 (x: byref<int>) (y: byref<int>) = &x

let should_not_work1 () =
    let mutable x = 1
    &x

let should_not_work2 () =
    let mutable x = 1
    let mutable x = (let mutable y = &x in &y)

    let y = &returning_ref &x
    &y

let should_not_work3 () =
    let mutable x = 1
    let mutable x =
        match &x with
        | x -> &x

    let y = &returning_ref &x
    &y

let should_not_work4 () =
    let mutable x = 1
    let mutable y = &x
    y <- 1
    &y

let should_not_work5 () =
    let x = 1
    let y = &x
    &y

let should_not_work6 () =
    let mutable x = 1
    &returning_ref &x

let should_not_work7 () =
    let mutable x = 1
    &returning_ref2 &beef &x

let should_not_work8 () =
    let mutable x = 1
    let y = &returning_ref2 &beef &x
    &y

let should_work1 () =
    Span.Empty

let should_work3 () =
    let y = &returning_ref &beef
    &y
    
let should_work2 () =
    let x = Span.Empty
    x

let should_work4 () =
    &returning_ref2 &beef &beef

let should_work5 () =
    let mutable x = 1
    returning_ref2 &x &x

let should_work6 () =
    let mutable x = 1
    returning_span &x

let should_work7 () =
    let mutable x = 1
    let y = returning_span &x
    y

All behave as intended except for "should_not_work2" and "should_not_work3", which pass in the compiler. Will need to dig into the ast a bit more.

@cartermp cartermp added this to the 15.8 milestone Jun 9, 2018
@cartermp cartermp requested a review from dsyme June 9, 2018 17:57
@cartermp
Copy link
Contributor

cartermp commented Jun 9, 2018

Tagging as 15.8 - we're in escrow for Preview 3, so ideally this would get in there, but we may have to get it into Preview 4 instead. Which is probably fine, it would just mean that we'd have a set of known issues for Span in Preview 3.

@TIHan
Copy link
Contributor Author

TIHan commented Jun 11, 2018

Here are some updated cases:

let mutable beef = 1

let returning_span (x: byref<int>) = Span.Empty

let returning_ref (x: byref<int>) = &x

let returning_ref2 (x: byref<int>) (y: byref<int>) = &x

let passing_span_returning_span (x: Span<int>) = Span.Empty

let passing_span_returnin_ref (x: Span<int>) = &x.[0]

let passing_span2 (x: Span<int>) (y: Span<int>) = ()

let passing_byref_of_span_returning_span (x: byref<Span<int>>) = x

let passing_byref_of_span_returning_byref_of_span (x: byref<Span<int>>) = &x

let should_not_work () =
    let test = ReadOnlySpan.Empty
    let f = fun () -> test
    ()

let should_not_work1 () =
    let mutable x = 1
    &x

let should_not_work2 () =
    let mutable x = 1
    let mutable x = (let mutable y = &x in &y)

    let y = &returning_ref &x
    &y

let should_not_work3 () =
    let mutable x = 1
    let mutable x =
        match &x with
        | x -> &x

    let y = &returning_ref &x
    &y

let should_not_work4 () =
    let mutable x = 1
    let mutable y = &x
    y <- 1
    &y

let should_not_work5 () =
    let x = 1
    let y = &x
    &y

let should_not_work6 () =
    let mutable x = 1
    &returning_ref &x

let should_not_work7 () =
    let mutable x = 1
    &returning_ref2 &beef &x

let should_not_work8 () =
    let mutable x = 1
    let y = &returning_ref2 &beef &x
    &y

let should_not_work9 () =
    let mutable s = Span.Empty
    passing_span_returning_span s

let should_not_work10 () =
    let mutable s = Span.Empty
    &passing_span_returnin_ref s

let should_not_work11 () =
    let mutable s = Span.Empty
    &passing_span_returnin_ref (passing_span_returning_span s)

let should_not_work12 () =
    let mutable s = Span.Empty
    passing_byref_of_span_returning_span &s

let should_not_work13 () =
    let mutable s = Span.Empty
    s

let should_not_work14 () =
    let mutable s = Span.Empty
    let mutable x = &s
    passing_byref_of_span_returning_span &x

let should_work1 () =
    Span.Empty

let should_work3 () =
    let y = &returning_ref &beef
    &y
    
let should_work2 () =
    let x = Span.Empty
    x

let should_work4 () =
    &returning_ref2 &beef &beef

let should_work5 () =
    let mutable x = 1
    returning_ref2 &x &x

let should_work6 () =
    let mutable x = 1
    returning_span &x

let should_work7 () =
    let mutable x = 1
    let y = returning_span &x
    y

let should_work8 () =
    let s = Span.Empty
    passing_span_returning_span s

let should_work9 () =
    let mutable s = Span.Empty
    passing_span_returnin_ref s

let should_work10 () =
    let mutable s = Span.Empty
    passing_span_returnin_ref (passing_span_returning_span s)

let should_work11 () =
    let s = Span.Empty
    &passing_span_returnin_ref (passing_span_returning_span s)

let should_work12 () =
    let s = Span.Empty
    passing_byref_of_span_returning_span &s

let should_work13 () =
    let s = Span.Empty
    let mutable x = &s
    passing_byref_of_span_returning_span &x

let should_work14 (s: byref<Span<int>>) =
    passing_byref_of_span_returning_byref_of_span &s

let should_work15 (s: byref<Span<int>>) =
    let x = &s
    let y = &passing_byref_of_span_returning_byref_of_span &x
    &y

type TestDelegate = delegate of byref<Span<int>> -> Span<int>

let should_work16 (s: byref<Span<int>>) =
    let f = TestDelegate(fun _ -> Span.Empty)
    //let mutable s = Span.Empty
    f.Invoke(&s)

type IBeef =

    abstract Test : byref<Span<int>> -> byref<Span<int>>

let should_work17 () =
    { new IBeef with
        member __.Test s = &s
    }

@TIHan
Copy link
Contributor Author

TIHan commented Jun 12, 2018

All tests are passing, this is a good sign. I will add more tests, code cleanup and comments.

@dsyme
Copy link
Contributor

dsyme commented Jun 13, 2018

Fantastic work!

@TIHan
Copy link
Contributor Author

TIHan commented Jun 13, 2018

I'm going to write more tests. Going to port some roslyn ones over.

@forki
Copy link
Contributor

forki commented Jun 16, 2018

This is starting to become an epic - shouldn't we try to get it into the bank now and then you can send new PRs on top?

@TIHan
Copy link
Contributor Author

TIHan commented Jun 16, 2018

We set a plan to get this in next week, hopefully Monday if all goes well. All that remains are a few more tests and I think it should be good to go.

@TIHan
Copy link
Contributor Author

TIHan commented Jun 16, 2018

test Ubuntu16.04 Release_fcs Build please

@TIHan
Copy link
Contributor Author

TIHan commented Jun 16, 2018

test Windows_NT Release_fcs Build please

@TIHan TIHan force-pushed the ref-span-fixes2 branch from 9e6fc8a to 6460167 Compare June 16, 2018 18:09
@TIHan TIHan changed the title [WIP] Ref/Span fixes Ref/Span fixes Jun 20, 2018
@TIHan
Copy link
Contributor Author

TIHan commented Jun 20, 2018

test Windows_NT Release_ci_part4 Build please

@TIHan TIHan merged commit 582b967 into dotnet:master Jun 20, 2018
@forki
Copy link
Contributor

forki commented Jun 20, 2018 via email

TIHan added a commit to TIHan/visualfsharp that referenced this pull request Jun 20, 2018
* Added LimitedValFlags for PostInferenceChecks

* More work

* Renamed IsLimited to IsLimitedSpanLike

* Don't use isByRefReturnCall in CheckExprs

* Minor cleanup

* Changed error message. Fixed safety rule regarding byrefs returning and passing.

* Fixed an edge case with returning span. Simplified Expr.App

* Cleanup

* Fixed a few ref return checks. Partially handling byref of span-like types

* Simplifying flags. Added CheckCall

* Updated test baseline. Added several CheckCall functions

* Updated baseline tests

* Handling byref of span likes

* Added a little bit better error msg for LByrefGet

* Removed commented code

* Fixed test

* Fixed inability to use byref<Span<>> for delegates and slots

* Minor cleanup with some comments

* Few more comments

* Running neg tests in core/span. Fixed bug in IsByRefLike structs with span fields

* Updated baseline

* Fixed more issues with slots. Simplified PermitByRefType

* Added a little better error messages on spans

* Fixed record construction for span likes. Fixed setting fields on mutable structs not having any rules for span likes. Updated a few error msgs. Added lots of tests.

* Updated error msg on function call. Added a removed comment from previous commit. Updated baselines

* Added internalTestSpanStackReferring test option

* Added special handling for receivers

* Fixing build

* Fixing build

* Fixing build

* More tests, updated baseline
KevinRansom pushed a commit that referenced this pull request Jun 20, 2018
* Ref/Span fixes (#5146)

* Added LimitedValFlags for PostInferenceChecks

* More work

* Renamed IsLimited to IsLimitedSpanLike

* Don't use isByRefReturnCall in CheckExprs

* Minor cleanup

* Changed error message. Fixed safety rule regarding byrefs returning and passing.

* Fixed an edge case with returning span. Simplified Expr.App

* Cleanup

* Fixed a few ref return checks. Partially handling byref of span-like types

* Simplifying flags. Added CheckCall

* Updated test baseline. Added several CheckCall functions

* Updated baseline tests

* Handling byref of span likes

* Added a little bit better error msg for LByrefGet

* Removed commented code

* Fixed test

* Fixed inability to use byref<Span<>> for delegates and slots

* Minor cleanup with some comments

* Few more comments

* Running neg tests in core/span. Fixed bug in IsByRefLike structs with span fields

* Updated baseline

* Fixed more issues with slots. Simplified PermitByRefType

* Added a little better error messages on spans

* Fixed record construction for span likes. Fixed setting fields on mutable structs not having any rules for span likes. Updated a few error msgs. Added lots of tests.

* Updated error msg on function call. Added a removed comment from previous commit. Updated baselines

* Added internalTestSpanStackReferring test option

* Added special handling for receivers

* Fixing build

* Fixing build

* Fixing build

* More tests, updated baseline

* Opening System in PostInferenceChecks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants