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 #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.