Good catch! We'll want to take this for 16.7 for sure.

@cartermp, no need to do that. it only occurs when using --langver:preview or 5.0. And by definition they are currently preview. The next preview release is early enough to get this in :-)

@cartermp of course if it's merged by friday, then 16.7 will be the next preview :-)

I don't understand the fix or the test change. What actually works and what doesn't with what features enabled/disabled? Why is this a good fix?

It looks like we're now only running the byrefs test with --langversion:5.0? Why? Does the test no longer work with default language version?

It looks like we're now only running the byrefs test with --langversion:5.0? Why? Does the test no longer work with default language version?

It feels like we should be explicitly running that test with both --langversion:4.7 and --langversion:5.0? Does it pass with 4.7?

It might be a good idea to always run the tests in 'preview' and only be explicit with versions when we want to test disabling features.

@TIHan, the thing is, the test that fails, fails because of the new code, but is not a really a related feature. If we always test preview, then we may discover that we have the opposite problem, where adding a feature works in preview, but some other existing feature regresses. I think we probably pretty much need two distinct test passes. One with preview by default and one with released by default. Anyway, I thought I had added the legacy version of the test, it's just my addled brain didn't. And now I also agree with don, the fix, fixes the issue by turning the feature off for a class of types, it's a rubbish fix. I have one I can at least explain, if you are around Will, I would like to discuss the new version

I think this fix feels like band-aid to a larger problem, since the fact that we're dealing with a nonoptional argument is apparently not handled correctly in the catch-all case of that pattern match. So perhaps a more appropriate fix should be done there?

We don't want to have a massive test matrix for every permeation of language features. It would be hell to maintain and make our CI times unbearable. This is exactly the sort of thing that promoting features out of preview will also catch.

@cartermp,
Will and I have talked about this, we have an approach that should work, there is a larger problem, actually caused by two different Byref types and unification of them. I need to do a bit more testing but I think we have a way forward that is okay.

Oh joy!

I'd really like to see the minimal example that demonstrates the actual problem being solved here. All I see in the discussion above is that the "byrefs" test started failing when the feature is activated?

I think this fix feels like band-aid to a larger problem, since the fact that we're dealing with a nonoptional argument is apparently not handled correctly in the catch-all case of that pattern match. So perhaps a more appropriate fix should be done there?

I'd say the opposite - the fix feels like it is too large for what must be a single failing case.

I think we'd welcome an alternative fix if it's smaller. It's one class of failing case exemplified here, bit surfaces in many individual cases since they test different combinations of byref kinds:

type C() = 
    static member f1 (x: inref<'T>) = 1
let f2 (x: outref<'T>) = C.f1(&x)

Do you disagree with the analysis in the PR text?

@dsyme , I will catch up with you next week on teams, however, this isn't really a big fix at all, indeed it is more like merely a refactoring.

Firstly, the code was bifurcated and did similar things in each bifurcation. If the feature nullable interop wasn't enabled then it did this code:
https://github.com/dotnet/fsharp/blob/master/src/fsharp/MethodCalls.fs#L1173

    | NotOptional when not (g.langVersion.SupportsFeature LanguageFeature.NullableOptionalInterop) ->
        if isOptCallerArg then errorR(Error(FSComp.SR.tcFormalArgumentIsNotOptional(), m))
        assignedArg

In the case the feature was enabled it does this code instead:
https://github.com/dotnet/fsharp/blob/master/src/fsharp/MethodCalls.fs#L1180

            | NotOptional ->
                //  T --> Nullable<T> widening at callsites
                if isOptCallerArg then errorR(Error(FSComp.SR.tcFormalArgumentIsNotOptional(), m))
                if isNullableTy g calledArgTy then 
                    MakeNullableExprIfNeeded infoReader calledArgTy callerArgTy callerArgExpr m
                else
                    callerArgExpr

callerArgExpre is returned in the exact same circumstance that assignedArg is called with the feature disabled, and is an expression for fetching the value of assignedArg. So if we can just fit in a language version check the old code can go away. It turns out that MakeNullableExpreIf needed is a good place to do that check.

Now the rest of the changes, were due to the observation, that whenever the newcode returns callerArg, it is in fact just returning an expression that fetches the value for assignedArg. computed here. So we simply eliminate the need to calculate the expression in those cases by returning an option instead. And then when we have None return assignedArg.

So ... and here is the bit you won't like ... the reason that byrefs actually failed, is because when we compute CallerArg2 here for expressions that contain byrefs<T, U> we create the expression incorrectly using byref. hich cause the typechecker to not recognize the expression we pass back, yielding the error messages from the explanation above.

The reason this refactoring doesn't show those errors is because when we don't have to create a new expression we use the original assignedArg that was computed elsewhere and passed into this function. Rather than only fixing the | NotOptional I fixed all of the places where we return callerArg2, this reduces the work we have to do, and as a side effect fixes the byref issue.

Anyway, I will call you Monday and we can discuss it in persion, if you have any other issues.

Thanks

Kevin

@dsyme note that the lack of this fix is also preventing some of our infrastructure PRs from moving across branches. We want to get this in, in part, so that we can have regular code flow through the repo.

@cartermp , I will chat with @dsyme next week. All will be fine.

Do you disagree with the analysis in the PR text?

I'll take a look at the issue in depth, partly to determine what went wrong with my coding techniques to ensure no change for existing code.

So my approach is to look at the difference between what change happened for AdjustCallerArgForOptional for input:

type C() = 
    static member f1 (x: inref<'T>) = 1
let f2 (x: outref<'T>) = C.f1(&x)

Here are my notes to self:

current master (with --langversion:5.0) this code returns:

    let calledArg = assignedArg.CalledArg
    let calledArgTy = calledArg.CalledArgumentType
    let callerArg2 = CallerArg(tyOfExpr g callerArgExpr, m, isOptCallerArg, callerArgExpr)
    { assignedArg with CallerArg=callerArg2 }

previous releases this code (and also current master with --langversion:4.7 this code) :

    assignedArg

The difference here is that in the result result.CallerArg.CallerArgumentType has changed from inref<...> to outref<_>. This is definitely the root cause of the problem.

Now, I rewrote the code on the LanguageFeature.NullableOptionalInterop path on the assumption that tyOfExpr g callerArgExpr was going to be identical in this case. I recall thinking about this at the time and came to the conclusion that it should be correct, but this was mistaken.

Specifically in this case we are allowing an outref expression to be used as an inref argument, but we are not inserting any code of coercion in the expressions, e.g. this code. For other mistmaches at a callsite we make a corresponding adjustment in the expression (e.g. converting a function expression to a delegate, or a non-nullable to a nullable), but not in this case. This was the flaw in my thinking. Allowing the type mismatch is deliberate, see here.

My proposal is that we make a simpler fix, specifically just adding these lines:

    | NotOptional when not (isNullableTy g calledArgTy) -> 
        if isOptCallerArg then errorR(Error(FSComp.SR.tcFormalArgumentIsNotOptional(), m))
        assignedArg

See #9629

closing in favour of: #9629