FS1118 on inlined function in private module from SDK 8.0.300

[loop-evgeny]

Repro steps

Compile the following code using .NET SDK 8.0.300

module private PrivateModule =
    let moduleValue = 1

    let inline getModuleValue () =
        moduleValue

[<EntryPoint>]
let main argv =
    //   [FS1118] Failed to inline the value 'getModuleValue' marked 'inline', perhaps because a recursive value was marked 'inline'
    //   (fixed by making PrivateModule internal instead of private)
    PrivateModule.getModuleValue () |> ignore
    0

Expected behavior

Compiles without warnings, as it did up to .NET SDK 8.0.205

Actual behavior

Compiles with 3 warnings:

Program.fs(11,5): Warning FS1118 : Failed to inline the value 'getModuleValue' marked 'inline', perhaps because a recursive value was marked 'inline' Program.fs(11,5): Warning FS1118 : Failed to inline the value 'getModuleValue' marked 'inline', perhaps because a recursive value was marked 'inline' Program.fs(11,5): Warning FS1118 : Failed to inline the value 'getModuleValue' marked 'inline', perhaps because a recursive value was marked 'inline'

Known workarounds

Make the module internal instead.

I don't know whether the function was actually being inlined before or not. If not then I guess the warning is better than no warning, but:

• it seems strange that the module would need to be internal to inline it
• the warning message is confusing
• the warning message appears 3 times for the same call

[loop-evgeny]

A similar case with classes:

module FS1118b

type private FirstType () =
    member this.FirstMethod () = ()

type private SecondType () =
    member this.SecondMethod () =
        let inline callFirstMethod (first: FirstType) =
            first.FirstMethod ()

        callFirstMethod (FirstType())

[FS1118] Failed to inline the value 'callFirstMethod' marked 'inline', perhaps because a recursive value was marked 'inline'

[vzarytovskii]

Yes, this is expected, result of https://github.com/dotnet/fsharp/pull/15484

I don't know whether the function was actually being inlined before or not.

I believe it wasn't, due to how it's lifted.

@KevinRansom shall this be resolved as by design?

[loop-evgeny]

I don't understand at all. That PR just mentions adding a new compiler switch, but I am not using any new compiler switch and the same code that used to build now generates a warning. We have /WarnAsError on in CI, so the build now fails. The linked release notes just say "Minor tweaks to inline specifications to support Visibility PR", which gives no clue as to this breaking change.

If the functions were not being inlined before then of course knowing about it is good, but the warning message needs much improvement. I spent most of the day tracking this down, as the real code is much more complex. It probably needs to link to a whole page that would explain why some functions cannot be inlined. I still don't understand why they can't be in these cases. I would expect inlining to ignore accessibility, since it's purely a performance optimization and does not change the meaning of the code.

[vzarytovskii]

I don't understand at all. That PR just mentions adding a new compiler switch, but I am not using any new compiler switch and the same code that used to build now generates a warning. We have /WarnAsError on in CI, so the build now fails. The linked release notes just say "Minor tweaks to inline specifications to support Visibility PR", which gives no clue as to this breaking change.

If the functions were not being inlined before then of course knowing about it is good, but the warning message needs much improvement. I spent most of the day tracking this down, as the real code is much more complex. It probably needs to link to a whole page that would explain why some functions cannot be inlined. I still don't understand why they can't be in these cases. I would expect inlining to ignore accessibility, since it's purely a performance optimization and does not change the meaning of the code.

Inlining error became warning and its scope widened, which resulted in compiler emitting it for additional code. Before this change compiler emitted all private code as internal. Now it's really private.

Compiler can't ignore visibility of the code, since it's unable to sufficiently analyze whether it's inlineable or not and can easily lead to invalid IL.

[loop-evgeny]

Then the warning needs to explain properly what's wrong. Right now it's beyond unhelpful - it actually gives the user a red herring with the "recursive value" thing. (Yes, I know it says "perhaps", but still.) Something like "Failed to inline the value 'getModuleValue' marked 'inline' because it accesses private field 'moduleValue' in module 'PrivateModule', which is not accessible from the call site" (Note that without accessing "moduleValue" you don't get the warning.)

It also should not report the warning 3 times, but that's a relatively minor issue.

In the second case ("callFirstMethod") it's even more confusing, because I can inline that function manually just fine:

        let inline callFirstMethod (first: FirstType) =
            first.FirstMethod ()

        let first = FirstType()
        callFirstMethod first // Warning FS1118
        first.FirstMethod () // No warning!

Before this change compiler emitted all private code as internal. Now it's really private.

Isn't this considered a breaking change? I would not expect to see that in a minor SDK release, without any explanation.

[vzarytovskii]

It is a breaking change, which should've been behind language flag. Not sure why it's not.

Well, warning is a breaking change. Codegen (both internal representation of visibility and IL gen) is not. Those are implementation details.

Regarding the error message - right now compiler doesn't have information why it can't inline by the time it emits the diagnostic. This will require a significant rework of the analysis.

[T-Gro]

The warning message could be extended by or the function accesses private members

[brianrourkeboll]

@loop-evgeny

Known workarounds

Make the module internal instead.

I think another workaround that doesn't require code changes would be to add this to your project file:

<PropertyGroup>
  <!-- …TargetFramework, etc. -->
  <OtherFlags>$(OtherFlags) --realsig-</OtherFlags>
</PropertyGroup>

Or maybe to add -p:OtherFlags=--realsig- to your invocation of dotnet build.

Edit: as @vzarytovskii points out, it looks like this is probably happening outside of that flag, so never mind.

[vzarytovskii]

@loop-evgeny

Known workarounds

Make the module internal instead.

I think another workaround that doesn't require code changes would be to add this to your project file:

<PropertyGroup>

  <!-- …TargetFramework, etc. -->

  <OtherFlags>$(OtherFlags) --realsig-</OtherFlags>

</PropertyGroup>

Or maybe to add -p:OtherFlags=--realsig- to your invocation of dotnet build.

I don't think it's on. I think this particular instance of warning slipped elsewhere.

[T-Gro]

An alternative workaround to make CI pass for you would be to locally disable FS1118 warning in that file. Or remove the "inline" modifier.

Both of these will allow to keep the accessibility as private.

[KevinRansom]

So ... the PR did tighten up the visibility rules for inline functions. I think I may have over-done it. It seems as if in the optimizer, I am acting as if 'private' on PrivateModule means 'private', however, private on modules and classes at the top level means internal, that's just how il is.

Given that the code below works both realsig+ and realsig-, and shows that it is possible to access moduleValue directly from main, it seems like it should be possible to inline getModuleValue () in main.

So I will take a look at the issue and see if there is something that can be done to fix this. In the meantime as identified by my colleagues a #nowarn "1118" in the source or the project file will unblock you.

I am not certain that it is easily fixable, but I do agree, how it is now is not ideal.

module private PrivateModule =
    let moduleValue = 1

    let inline getModuleValue () =
        moduleValue

[<EntryPoint>]
let main argv =
    //   [FS1118] Failed to inline the value 'getModuleValue' marked 'inline', perhaps because a recursive value was marked 'inline'
    //   (fixed by making PrivateModule internal instead of private)
    let x = PrivateModule.moduleValue
    printfn $"{x}"
    0

[loop-evgeny]

I can still repro with .NET SDK 8.0.301. Wasn't the fix supposed to be released in that?

[vzarytovskii]

I can still repro with .NET SDK 8.0.301. Wasn't the fix supposed to be released in that?

No, it wasn't merged to 301, it waits for branch lockdown lift - https://github.com/dotnet/sdk/pull/41047

[loop-evgeny]

I can still repro with .NET SDK 8.0.302, too. What is the target version for the fix, please?

[vzarytovskii]

I can still repro with .NET SDK 8.0.302, too. What is the target version for the fix, please?

Hm, I have no idea how sdk versions are numbered or how to tell what's the release is unfortunately.

PR was merged on 24th of May, @baronfel might know better which release it ended up in, perhaps?

[T-Gro]

Confirmed:

Fails on 8.0.302 Suceeds on 9.0.100-preview

The production version of 8.0.302 is: 12.8.300-beta.24211.1+90a81d78e3a2780e8fc599fff60c7bfcc5ab4526

[T-Gro]

Whereas the fix was only pushed with https://github.com/dotnet/fsharp/pull/17189 , few weeks later.

[baronfel]

I believe the late May check in meant that it got pushed to the July release.

[vzarytovskii]

Ty @T-Gro and @baronfel for checking!

[loop-evgeny]

Confirmed that this is fixed in 8.0.303. Thanks!