FS1118 on inlined function in private module from SDK 8.0.300

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

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'

@KevinRansom

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?

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.

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.

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.

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.

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

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

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

@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>

  

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

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.

Fix inlining regression: dotnet#17161

Merge pull request #17189 from KevinRansom/cp_opt

Revert "Fix inlining regression: #17161"

Merge pull request #17200 from dotnet/revert-17189-cp_opt

Fix inlining regression: dotnet#17161

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

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 - dotnet/sdk#41047

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

@baronfel

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?

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+90a81d7

Whereas the fix was only pushed with #17189 , few weeks later.

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

@T-Gro

Ty @T-Gro and @baronfel for checking!

Confirmed that this is fixed in 8.0.303. Thanks!

Navigation Menu

Repository navigation

FS1118 on inlined function in private module from SDK 8.0.300 #17161

3 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

FS1118 on inlined function in private module from SDK 8.0.300 #17161

Description

Activity

emz00 commented on May 16, 2024

vzarytovskii commented on May 16, 2024

emz00 commented on May 16, 2024

vzarytovskii commented on May 16, 2024

emz00 commented on May 16, 2024

vzarytovskii commented on May 16, 2024

T-Gro commented on May 16, 2024

brianrourkeboll commented on May 16, 2024

vzarytovskii commented on May 16, 2024

T-Gro commented on May 16, 2024

3 remaining items

emz00 commented on May 29, 2024

vzarytovskii commented on May 29, 2024

emz00 commented on Jun 13, 2024

vzarytovskii commented on Jun 13, 2024

T-Gro commented on Jun 13, 2024

T-Gro commented on Jun 13, 2024

baronfel commented on Jun 13, 2024

vzarytovskii commented on Jun 13, 2024

emz00 commented on Jul 10, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions