search

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.83k stars 773 forks source link

Fix inlining regression: https://github.com/dotnet/fsharp/issues/17161 #17189

Closed KevinRansom closed 1 month ago

KevinRansom commented 1 month ago

Description A recent feature impacting the IL Visibility of some F# types had an unintended impact to the F# optimization feature. The optimizer is incorrectly unable to inline some code that has "private" visibility, even when the inlined variables are "in-scope". The fix removes the code that aggressively and incorrectly detects this private visibility.

Regression? (was it working in a previous release or preview?) Yes this is a regression, it has been observed internally, as well as by external developers using the 8.0.300 preview SDK.

Risk (see taxonomy) Low: it removes an unnecessary, incorrect warning.

Testing

New automated regression tests added.

Original issue: https://github.com/dotnet/fsharp/issues/17161

PR into main: https://github.com/dotnet/fsharp/commit/9d053591bbec208490652ffba5747c5f13b35b34

github-actions[bot] commented 1 month ago

:warning: Release notes required, but author opted out

[!WARNING] Author opted out of release notes, check is disabled for this pull request. cc @dotnet/fsharp-team-msft

baronfel commented 1 month ago

Y'all, you gotta stop merging PRs that are waiting for servicing approval 😅

Now this needs to be reverted again and a new PR prepped so that Tactics can review at today's servicing meeting.

vzarytovskii commented 1 month ago

Y'all, you gotta stop merging PRs that are waiting for servicing approval 😅

Now this needs to be reverted again and a new PR prepped so that Tactics can review at today's servicing meeting.

Reverting PRs back and forth messed up our CI, previous mergws were to revert this change in main. This fixes it. I cancelled the build for insertion. If it's not approved, we will revert it and deal with merges.

baronfel commented 1 month ago

The problem is that merging this makes is disappear from the list that Tactics reviews during the meeting - https://tacticsview.azurewebsites.net/ is powered by looking for PRs that are open with the servicing-consider label. The typical process would be to do the work in main, cherry pick to a release branch, get tactics approval on that PR, then merge and let code flow happen to SDK.

vzarytovskii commented 1 month ago

The typical process would be to do the work in main, cherry pick to a release branch, get tactics approval on that PR, then merge and let code flow happen to SDK.

Yes, and it wasn't done like that, hence issues with code flow. Revert was undoing the fix in both 17.11 and main.