dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.82k stars 772 forks source link

F# using try .. with .. inside sequence expression raises trimming warning on publish. #17356

Closed abklearnhere closed 2 days ago

abklearnhere commented 3 days ago

Discussed in https://github.com/dotnet/fsharp/discussions/17323

Originally posted by **@abklearnhere** June 18, 2024 F# using `seq { try .. with .. }` raises trimming warning on publish. For example, ```fsharp let f24 () = seq { try ignore (1/0); 111 with e -> 222 } [] let main _ = f24() |> Seq.head |> System.Console.WriteLine 0 ``` I am creating an F#, .NET 8 (SDK 8.0.302) console app and publish as a single file with AOT, trimming. I run dotnet publish command as below: ```powershell dotnet publish -c Release -r win-x64 --self-contained true -p:PublishAot=true -p:PublishTrimmed=true -p:ReflectionFree=false -o ./publish ``` This results in warning: >D:\a\_work\1\s\src\FSharp.Core\prim-types.fs(7159): Trim analysis warning **IL2091**: Microsoft.FSharp.Control.LazyExtensions.Create(FSharpFunc\`2): 'T' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'System.Lazy\`1'. The generic parameter 'T' of 'Micros oft.FSharp.Control.LazyExtensions.Create(FSharpFunc\`2)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. The `fsproj` file has following settings: ```xml Exe net8.0 true true true false ```
vzarytovskii commented 2 days ago
abklearnhere commented 2 days ago

@vzarytovskii I guess this is marked duplicate because fixing lazy would fix this. Instead, can this be fixed by removing dependency of lazy from seq { try .. with .. } ? I found exactly one use of lazy in entire file seqcore.fs. That reference causes this issue. Would you agree? Would this be preferred?

vzarytovskii commented 2 days ago

@vzarytovskii I guess this is marked duplicate because fixing lazy would fix this. Instead, can this be fixed by removing dependency of lazy from seq { try .. with .. } ? I found exactly one use of lazy in entire file seqcore.fs. That reference causes this issue. Would you agree? Would this be preferred?

Probably not, since getting rid of lazy will mean it will be a substantial change and will need to be replaced with something to emulate it, which might have the same issue, which will need to be addressed.

abklearnhere commented 2 days ago

Ah, okay, good to know. I was thinking of something simple like

        let mutable sourceEnumerator : IEnumerator<'T> = null
        let originalSource () =
            if isNull sourceEnumerator then
                sourceEnumerator <- source.GetEnumerator()

            sourceEnumerator

would be sufficient to replace let originalSource = lazy (source.GetEnumerator()). There are three originalSource.Value calls that could be replaced by originalSource ().

But, if there's more to it, I can understand. Thanks for taking a look.

vzarytovskii commented 2 days ago

Ah, okay, good to know. I was thinking of something simple like


        let mutable sourceEnumerator : IEnumerator<'T> = null

        let originalSource () =

            if isNull sourceEnumerator then

                sourceEnumerator <- source.GetEnumerator()

            sourceEnumerator

would be sufficient to replace let originalSource = lazy (source.GetEnumerator()). There are three originalSource.Value calls that could be replaced by originalSource ().

But, if there's more to it, I can understand. Thanks for taking a look.

Lazy is also thread safe, mutable variable and check aren't, so this also needed to be solved, so own replacement solution will quickly grow in complexity and size, as well as it will require significantly more testing to make sure its behaviour is as before in both single and miltithreaded scenarios.

abklearnhere commented 1 day ago

Got it! Thanks for the explanation!

vzarytovskii commented 1 day ago

Got it! Thanks for the explanation!

Sure, np, thanks for testing out different aspects of AOT and reporting those back!