Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arguments cannot be passed byref #1696

Closed
MangelMaxime opened this issue Jan 4, 2019 · 6 comments

Comments

@MangelMaxime
Copy link
Member

commented Jan 4, 2019

Repro code

Given the following code:

open System.Collections.Generic

let cache = Dictionary<string, int>()
cache.Add("un", 1)
cache.Add("deux", 2)
cache.Add("trois", 3)

let mutable res = -1
let mutable test4 = false

test4 <- cache.TryGetValue("un", &res);;

Over dotnet runtime, we get:

printfn "%A" test4
// Gives: true

 printfn "%A" res;;                      
// Gives 1

While over Fable, we get:

printfn "%A" test4
// Gives: true

 printfn "%A" res;;                      
// Gives -1

Note that we get -1 instead of 1.

Expected and actual results

It should output 1 for Fable too.


For me the problem is in this function. It consider the third argument as a defaultValue while .Net documentation says:

When this method returns, contains the value associated with the specified key, if the key is found; otherwise, the default value for the type of the value parameter. This parameter is passed uninitialized.

Source

I don't know if others tryGetValue methods except the same behavior or no. And so if we should create a new function, also how to generate the default value for the expected type from runtime or should fable inject another arguments ?

@ncave

This comment has been minimized.

Copy link
Collaborator

commented Jan 5, 2019

@MangelMaxime Technically this should be a Fable compiler error as byref parameters are unsupported by Fable (because of javascript not supporting passing by reference). You can use the one returning a tuple:

let ok, res = cache.TryGetValue("un")

or match directly on the tuple:

match cache.TryGetValue("un") with
| (true, v) -> v
| _ -> -1
@MangelMaxime

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

Ah ok I didn't know that.

I couldn't use let ok, res = cache.TryGetValue("un") version because the variable are defined outside the scope using mutable. It's a low level code that's need this kind of "optimization".

But I found a work around to my problem.

@alfonsogarciacaro

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

We should have an error when trying to access the address of a value. I think originally we had it but I removed it to support Result which is a value type. It seems AddressOf is always used when accessing the contents of a value/struct type. Not sure how to discriminate these "valid" uses of AddressOf (Fable compiles structs as reference types, but they still work) from "invalid" uses like in the code of the first comment. Any ideas @ncave?

@ncave

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2019

@alfonsogarciacaro Yeah, a few, but they may or may not be of any use :)

  • The particular need to throw error on TryGetValue (and TryParse etc.) with a byref parameter can be solved by limiting the number of parameters we're matching on (as the byref parameter is an extra parameter).
  • That solves this issue, but not the general case of using byref parameters. Unless we can somehow restrict that usage to Result type only, perhaps by entity name?
  • This was probably already considered in the past and rejected for some reason, but what about actually implementing support for byref parameters, by wrapping them in an object?
@alfonsogarciacaro

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

That solves this issue, but not the general case of using byref parameters. Unless we can somehow restrict that usage to Result type only, perhaps by entity name?

It seems AddressOf is used whenever the contents of a struct class/union/record/tuple are accessed, so we would need to find a condition other than the name to discriminate the uses.

This was probably already considered in the past and rejected for some reason, but what about actually implementing support for byref parameters, by wrapping them in an object?

Hmm, right now we're actually doing something similar for mutable public values, so it could be possible to do it. For private and local mutable values we'll need to create a getter and setter that access the original value. Something like:

export function createAtomAdHoc<T>(getter: () => T, setter: (v: T) => void): (v?: T) => T | void {
    return (value: T) => value === void 0 ? getter() : setter(value);
}

So the following code:

let mutate (i: int byref) =
    i <- i * 2

let test() =
    let mutable x = 5
    mutate &x
    log x

Would become:

function mutate(i) {
    i(i() * 2);
}

function test() {
    let x = 5;
    foo(createAtomAdHoc(() => x, (v) => { x = v; }));
    log(x);
}

@alfonsogarciacaro alfonsogarciacaro changed the title Dictionnary.TryGetValue doesn't align with dotnet behavior Arguments cannot be passed byref Jan 9, 2019

@alfonsogarciacaro

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

A problem with that approach ☝️ however is devs tend to pass arguments byref for performance reasons (as @MangelMaxime was trying to do) and with the wrapping probably using byref becomes actually less performant.

I still couldn't find a way to distinguish the explicit AddressOf when using the & operator and the implicit AddressOf when accessing the contents of a struct type (for example, check the typed AST in this snippet). So for now, I just added a check for call arguments. This raises a warning atm to prevent regressions, but if we see it's safe we can turn it into an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.