[Rust] Bindings with Emit on interfaces

[ncave]

@alexswan10k

• Support modelling Rust interop bindings with Emit on interfaces.
• Calling convention is Rust native by default (not pass-by-ref).

Note: Unlike Enum on module let bindings which gives you more control, in interface member signatures we can only use F# language features, so we have to model passing references using something like F# byref/inref/outref. This is still TBD.

[alexswan10k]

Nice.

Ah yes, the ref keyword has got me thinking now. We currently pass everything by reference for function parameters but this is inconsistent with dotnet which only passes by ref if the ref keyword is present, and value by default. Should we be trying to model this do you think? You would get clones at every boundary which is not ideal, but otherwise you are often passing a pointer to a pointer to an object which is kinda weird anyway such as &Rc. If the ref keyword switched this on you would at least have control over this. By value seems conceptually closer to dotnet.

Sorry if this is obvious

[ncave]

@alexswan10k We could try passing by value and carve a special case for generic parameters, as they need to be passed by reference. The goal IMO is to make the generated code mode idiomatic, not sure if cloning and passing by value would do that, but perhaps it will, as there will be less need to clone afterwards. Changing the calling convention is a big and breaking change so we should decide on it sooner rather than later.

[ncave]

@alexswan10k To clarify, modelling pass by reference using byref/inref/outref can be tricky as they are already implemented using mutable cells. Perhaps we can keep byref/outref as is using mutable cells, and only use inref for pass by reference?

[alfonsogarciacaro]

@ncave @alexswan10k I've added a NEXT section to release notes so when sending a PR if you want you can add a brief description of the changes that will be included in the notes of the next release 👍

Fable/src/Fable.Cli/RELEASE_NOTES.md

Lines 1 to 3 in 72f8209

	### NEXT
	* Write here changes for next release

[alexswan10k]

Yeah i am aware this is a major breaking change which is super frustrating.. I am torn. It seems like the main reason we introduced pass by reference as default was to reduce cloning. I guess it never occured to me we could allow a user to control it by opting in, which gives them better fine tuning capabilities where needed. I am just a little worried we have made things harder for ourselves by diverging from dotnet behaviour. I dont like cloning everywhere either though!

I was experimenting a bit and inref seems to lead to reasonable code, although there are some caveats such as all inrefs must be bound first, presumably so dotnet has a pinned stack mem location to use as ref. Inref would be a good choice though.

Why must generics be ref always? I had a quick experiment with rust genetic fns and this does not seem to be needed, but there might be more nuance.

[alfonsogarciacaro]

@alexswan10k Ah, sorry! I didn't pretend to mean that we should document thoroughly breaking changes 😅 TBH I'm a bit lost in the discussion because I don't have experience with Rust, but we're still in the alpha release cycle so experimentation is totally fine, please do! I was just meaning that adding some comments in the release notes is a good way to track what has changed between releases. I've to admit that I'm not very strict with it yet, but it will become more important when more users start testing the releases 👍

[alexswan10k]

No need to apologise @alfonsogarciacaro 😆 when there are release notes we will be sure to write them up. Thanks for mentioning. My comments are too often an incoherent stream of consciousness so I should apologise 😀

We are still somewhat battling with the lack of a garbage collector, which means everything has to be abstracted with smart pointers. The problem is there are many incompatible smart pointers to choose from, all with their own trade offs.

I will write up tickets when it is obvious what needs to be decided or done, but a lot of this is just mentally working through the possibility space. Maybe this warrants it's own issue though.

[ncave]

@alexswan10k Re: passing by ref or by value, see also the arguments for and against it in the original discussion here.

[alexswan10k]

Good points, a good refresher. The benchmark we did was a particularly useful exercise. It clearly shows that passing by reference has significant performance advantages, and we know that reference counting is generally slow anyway.

That being said, I did not fully consider giving a user control over this might give you the best of both worlds. I am definitely coming round to the idea that control is preferred here, which means picking a sensible default. The problem is that our best representation of references (inref) is opt-in, which means by-value should probably be default. Gahh! I am happy to look at this if we go that way, since it is arguably my mess. I think it is worth thinking this over carefully before we commit to anything though.

We will still need reference tracking though for inref, I imagine the big change here is switching to leaveContextByValue as default.

I think the generic argument problem may well go away because we no longer represent closures and interfaces by &impl something, but Rc<impl/dyn Something>. This means it can be passed by value. I think the caveat with generics is that whatever you choose (value or reference), it must apply for all types, so you cannot have value for ints, and ref for structs.

I was going to create a generic issue for memory management stuff to consolidate all these ideas and track everything. That ok?

[ncave]

@alexswan10k

That ok?

Very much so, please do, thanks!

Also, micro-benchmarks are obviously not representative of real performance difference in an average code base, I'm sure we can create micro-benchmarks that show that passing by value is faster in certain cases, but that's not the point. Usually from performance point of view passing by ref is an optimization when the parameter is big enough to warrant that. But there are other considerations, so I fully agree that the sensible defaults is what we should be doing, whatever makes most sense.

[alexswan10k]

Totally agree. Ticket is here btw, not sure if you can edit but feel free to if I have missed anything. I will try keep it up to date with discoveries/ideas etc.

What are the other key outstanding issues that were total dealbreakers? Are you happy with where we are on Emit now for bindings? Import was another big one, wasn't it? Not sure how to move that forward heh. I don't think the summary ticket covers these.

[ncave]

@alexswan10k No, it doesn't cover it.

• The Emit I think is already in a good place, and we can gradually add features as needed (e.g. emitting constructors on types, etc.).
• The module import name clashing needs to be resolved before we can advance anything else.
  A robust module import support remains the last big hurdle for Fable2Rust before it can be usable for general purpose code.

[alexswan10k]

Great. So the problem with module imports is that you cannot name them I presume, you have to use the exported name as is?

You can use the "as" keyword to create a local alias. Does this https://doc.rust-lang.org/rust-by-example/mod/use.html help?

[ncave]

@alexswan10k Yes, we will use aliases to avoid name clashing, the question is what level of granularity we want to go to with the use clauses, i.e. do we want to keep using wildcards at some level, or do we want to go all the way down and declare a use clause for each imported name. Fable goes all the way down for JS imports, so that's always an option, although it will affect the readability of the generated code (not necessarily a concern if you don't look at it).

[alexswan10k]

I would suggest we go all the way down and spit out one for each function/type imported, it is probably the easiest to implement for MVP. We can always come back and rework it to be prettier/more lint friendly in the future. That feels pretty solvable though.

Beyond that is there anything else that is fundamentally wrong or needs to be working with imports? You can sort of call stuff directly by namespace using emit too if it is an external lib you are importing.

[ncave]

@alexswan10k No, that's it, just a matter of implementing it properly and making sure it works as expected in a complex multi-file project with many modules.

[alexswan10k]

Cool. Feels within reach now :)