Code gen of record updates when all fields are updated about fsharp HOT 16 CLOSED

Nice. Is this planned for F# 9 ?

from fsharp.

No, or I don't think so at least, we just came up with this, prompted by the other issue :)

from fsharp.

I don't understand. Do what?

from fsharp.

copy and update all fields I guess

from fsharp.

I don't understand. Do what?

We should codegen new records instantiations when all fields are getting updated instead of generating a warning

from fsharp.

Nice. Is this planned for F# 9 ?

It should be small enough so we can make it for 9

from fsharp.

I am still none the wiser. These are all exactly the same

let x y = { y with B = 1 }

let xx yy = { yy with A = yy.A; B = 1 }

let xxx yyy = { A = yyy.A; B = 1 }

The warning has nothing to do with codegen.

from fsharp.

@kerams You are right. Sorry for my earlier message.
Indeed codegen is ok, the warning is opt-in, and is ok as opt-in warning.
So there is really nothing to do.

from fsharp.

I am still none the wiser. These are all exactly the same

let x y = { y with B = 1 }

let xx yy = { yy with A = yy.A; B = 1 }

let xxx yyy = { A = yyy.A; B = 1 }

The warning has nothing to do with codegen.

Right, and we will get rid of warning altogether and instead will codegen new record construction when we can successfully detect that it's updating all fields.

from fsharp.

Nice. Is this planned for F# 9 ?

Everything planned for F# 9 can be found in the project - https://github.com/orgs/dotnet/projects/126/views/40
We don't plan to change the list, we might shelve some items if something crucial will come in (blocking engineering work, bug fixes, blocking interop work).

from fsharp.

codegen new record construction when we can successfully detect that it's updating all fields.

As opposed to emitting what?

The warning exists to inform you about a potential (refactoring) pitfall. Whether you choose to heed it or not, the resulting assembly is the same.

from fsharp.

codegen new record construction when we can successfully detect that it's updating all fields.

As opposed to emitting what?

The warning exists to inform you about a potential (refactoring) pitfall. Whether you choose to heed it or not, the resulting assembly is the same.

I for some reason thought that it doesn't generate the ctor call, must've thought about how C# records were working w.r.t. their clone method.

Then I don't really understand existence of this warning in the compiler. Could've been an analyzer.

from fsharp.

I don't know, why is 'unused value' in the compiler? What's the guidance? Do you want to relegate all potential future warnings into analyzers? If not, what are the deciding factors?

from fsharp.

I don't know, why is 'unused value' in the compiler? What's the guidance? Do you want to relegate all potential future warnings into analyzers? If not, what are the deciding factors?

I suppose if we have good analyzers infrastructure in future, all of the additional checks should go there unless decided otherwise. It's always good to offload some work from compiler. I saw a bunch of legit uses of this particular feature, when people have records with one field, it's "Empty" instance, and they just do Empty with Id=134. That's the reason we turned it off.

from fsharp.

I don't know, why is 'unused value' in the compiler? What's the guidance? Do you want to relegate all potential future warnings into analyzers? If not, what are the deciding factors?

I suppose if we have good analyzers infrastructure in future, all of the additional checks should go there unless decided otherwise. It's always good to offload some work from compiler. I saw a bunch of legit uses of this particular feature, when people have records with one field, it's "Empty" instance, and they just do Empty with Id=134. That's the reason we turned it off.

As someone who contributes a lot of error reporting. I think is important to get a proper guidance here. Personally I would not want of the Analysers to be the “new compiler”

from fsharp.

I don't know, why is 'unused value' in the compiler? What's the guidance? Do you want to relegate all potential future warnings into analyzers? If not, what are the deciding factors?

I suppose if we have good analyzers infrastructure in future, all of the additional checks should go there unless decided otherwise. It's always good to offload some work from compiler. I saw a bunch of legit uses of this particular feature, when people have records with one field, it's "Empty" instance, and they just do Empty with Id=134. That's the reason we turned it off.

As someone who contributes a lot of error reporting. I think is important to get a proper guidance here. Personally I would not want of the Analysers to be the “new compiler”

Any additional analysis which can be done outside of compiler, should be done outside of compiler (when and if we have infrastructure for it). Compiler should ideally only do typechecking and, well, compilation. Any additional analysis will be slowing things down.

from fsharp.