Fix #17797 -- Realsig+ generates nested closures with incorrect Generic arguments #17877

KevinRansom · 2024-10-13T04:04:19Z

When realsig+ is specified we generate closures as nested classes rather than classes at the same level as the class that the closures are for. This eliminates the need for members of the class to be internal allowing us to make the members private to reflect their source visibility.

Issue 17797 occurs because the closures were generated without the generic parameters. This fix addresses that by getting the generic parameters from the argument/typar environment.

github-actions · 2024-10-13T04:05:01Z

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description

src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md

T-Gro · 2024-11-26T10:24:54Z

tests/FSharp.Compiler.ComponentTests/EmittedIL/RealInternalSignature/ClassTypeInitialization.fs

+    [<InlineData(false, false)>]        // Regular NoOptimize
+    [<Theory>]
+    let ``BigTuples`` (realSig, optimize) =
+        let withOptimization compilation =


This block is repeated across tests, can be moved on top of the file:

|> withRealSig realSig |> withOptimize optimize

I can add something to CompilerUtilities, it is general enough

tests/FSharp.Compiler.ComponentTests/EmittedIL/RealInternalSignature/ClassTypeInitialization.fs

T-Gro · 2024-11-26T10:28:36Z

...ler.ComponentTests/EmittedIL/Inlining/Match01_RealInternalSignatureOn.fs.il.net472.debug.bsl

-      IL_0012:  callvirt   instance !1 class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class [FSharp.Core]Microsoft.FSharp.Core.Unit,int32>::Invoke(!0)
-      IL_0017:  ret
+      IL_0008:  ldnull
+      IL_0009:  call       int32 Match01/Test1::CompareTo$cont@4(class Match01/Test1,


I assume this diff is THE thing we have wanted to address in the dotnet/performance regression with structs, right?
i.e. calling a function directly as opposed to going via new closure allocation+invocation.

I think the merge of main fixed this. there is no longer a change in this baseline

T-Gro · 2024-11-26T10:31:21Z

...ler.ComponentTests/EmittedIL/Inlining/Match01_RealInternalSignatureOn.fs.il.net472.debug.bsl

-      .maxstack  4
-      .locals init (class Match01/Test1 V_0,
-               class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class [FSharp.Core]Microsoft.FSharp.Core.Unit,int32> V_1)
+      .maxstack  5


Seeing the amount of locals below, is the .maxstack 5 correct here?

Nevermind, you do plan do run ilverify and it will complain if there is any point in time for the program where it would needed more.

(just remembering we do reuse locals of same type, so I assume there must be some point in execution where more things are stacked)

I think the merge of main fixed this. there is no longer a change in this baseline

T-Gro · 2024-11-26T10:38:35Z

tests/fsharp/Compiler/CodeGen/EmittedIL/TaskGeneratedCode.fs

@@ -1104,7 +1104,7 @@ type Generic1InGeneric1<'T>() =
         extends [runtime]System.Object
  {
    .custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::.ctor(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags) = ( 01 00 03 00 00 00 00 00 ) 
-    .class auto autochar sealed nested assembly beforefieldinit specialname clo@7<A>
+    .class auto autochar sealed nested assembly beforefieldinit specialname clo@7<T,A>


This code is closure heavy, can we have it ilverified too?

I think vlad may have added some ilverification, so I'll see what I can reuse.

tests/FSharp.Compiler.ComponentTests/Conformance/LexicalAnalysis/SymbolicOperators.fs

T-Gro · 2024-11-26T10:41:21Z

tests/FSharp.Compiler.ComponentTests/EmittedIL/ComputedCollections/ComputedCollections.fs

@@ -17,59 +18,59 @@ module ComputedCollections =
    [<Theory; Directory(__SOURCE_DIRECTORY__, Includes=[|"Int32RangeArrays.fs"|])>]
    let ``Int32RangeArrays_fs`` compilation =
        compilation
-        |> verifyCompilation
+        |> verifyCompilation false


I am okay with this PR not turning realSig's default to true yet.

But when we do, I think we should be running as much tests as possible with the same default.
Will it be a painful update, or was there another reason for using "false" here?

I can double the test, so it does both, although InlineData and Directory don't play nice together.

T-Gro · 2024-11-26T10:44:16Z

My review is finished, but let's wait for the ilverify for realsig as a CI step as well.

…is/SymbolicOperators.fs Yeah the comments are pointless. Co-authored-by: Tomas Grosup <[email protected]>

KevinRansom

I will make these changes, they are good suggestions.

tests/FSharp.Compiler.ComponentTests/EmittedIL/RealInternalSignature/ClassTypeInitialization.fs

KevinRansom · 2024-12-12T17:50:42Z

tests/FSharp.Compiler.ComponentTests/EmittedIL/RealInternalSignature/ClassTypeInitialization.fs

+    [<InlineData(false, false)>]        // Regular NoOptimize
+    [<Theory>]
+    let ``BigTuples`` (realSig, optimize) =
+        let withOptimization compilation =


I can add something to CompilerUtilities, it is general enough

KevinRansom · 2024-12-12T17:51:49Z

tests/fsharp/Compiler/CodeGen/EmittedIL/TaskGeneratedCode.fs

@@ -1104,7 +1104,7 @@ type Generic1InGeneric1<'T>() =
         extends [runtime]System.Object
  {
    .custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::.ctor(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags) = ( 01 00 03 00 00 00 00 00 ) 
-    .class auto autochar sealed nested assembly beforefieldinit specialname clo@7<A>
+    .class auto autochar sealed nested assembly beforefieldinit specialname clo@7<T,A>


I think vlad may have added some ilverification, so I'll see what I can reuse.

KevinRansom · 2024-12-12T17:53:55Z

tests/FSharp.Compiler.ComponentTests/EmittedIL/ComputedCollections/ComputedCollections.fs

@@ -17,59 +18,59 @@ module ComputedCollections =
    [<Theory; Directory(__SOURCE_DIRECTORY__, Includes=[|"Int32RangeArrays.fs"|])>]
    let ``Int32RangeArrays_fs`` compilation =
        compilation
-        |> verifyCompilation
+        |> verifyCompilation false


I can double the test, so it does both, although InlineData and Directory don't play nice together.

psfinaki

Can't find anything suspicious here, well tested IMO.

psfinaki · 2024-12-19T13:14:04Z

DEVGUIDE.md

@@ -204,6 +204,33 @@ Linux/macOS:
 export TEST_UPDATE_BSL=1
 ```

+## Retain Test run built artifacts
+
+When investigating tests issues it is sometimes usefull to examine the artifacts built whe running tests.  Those built using the newer test framework are usually,


Suggested change

When investigating tests issues it is sometimes usefull to examine the artifacts built whe running tests. Those built using the newer test framework are usually,

When investigating tests issues it is sometimes useful to examine the artifacts built when running tests. Those built using the newer test framework are usually,

KevinRansom requested a review from a team as a code owner October 13, 2024 04:04

KevinRansom marked this pull request as draft October 13, 2024 04:04

KevinRansom force-pushed the fix17797 branch 4 times, most recently from 35a8aad to a7eeda8 Compare October 20, 2024 01:44

KevinRansom force-pushed the fix17797 branch 5 times, most recently from bb06ffa to 1d562ef Compare November 8, 2024 20:58

KevinRansom force-pushed the fix17797 branch 8 times, most recently from e13abad to ef80877 Compare November 20, 2024 01:49

KevinRansom changed the title ~~WIP : Attempt to fix #17797~~ Fix #17797 -- Realsig+ generates nested closures with incorrect Generic arguments Nov 20, 2024

KevinRansom marked this pull request as ready for review November 20, 2024 05:20

KevinRansom and others added 6 commits November 21, 2024 09:02

tests

a28c4c8

code

f36e0a3

baseline

97290fb

fantomas

e972335

release notes

2b7d917

ilverify baselines

d2ff439

KevinRansom force-pushed the fix17797 branch from 376285d to d2ff439 Compare November 21, 2024 18:56

T-Gro reviewed Nov 26, 2024

View reviewed changes

tests/FSharp.Compiler.ComponentTests/EmittedIL/RealInternalSignature/ClassTypeInitialization.fs Show resolved Hide resolved

T-Gro reviewed Nov 26, 2024

View reviewed changes

tests/FSharp.Compiler.ComponentTests/Conformance/LexicalAnalysis/SymbolicOperators.fs Outdated Show resolved Hide resolved

T-Gro reviewed Nov 26, 2024

View reviewed changes

Update tests/FSharp.Compiler.ComponentTests/Conformance/LexicalAnalys…

9283bce

…is/SymbolicOperators.fs Yeah the comments are pointless. Co-authored-by: Tomas Grosup <[email protected]>

KevinRansom commented Dec 12, 2024

View reviewed changes

KevinRansom added 2 commits December 13, 2024 07:06

merge

c624f52

Feedback pt 1

c7f7154

T-Gro approved these changes Dec 13, 2024

View reviewed changes

psfinaki approved these changes Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #17797 -- Realsig+ generates nested closures with incorrect Generic arguments #17877

Fix #17797 -- Realsig+ generates nested closures with incorrect Generic arguments #17877

KevinRansom commented Oct 13, 2024 •

edited

Loading

github-actions bot commented Oct 13, 2024 •

edited

Loading

T-Gro Nov 26, 2024

KevinRansom Dec 12, 2024

KevinRansom Dec 13, 2024

T-Gro Nov 26, 2024

KevinRansom Dec 13, 2024

T-Gro Nov 26, 2024

T-Gro Nov 26, 2024

KevinRansom Dec 13, 2024

T-Gro Nov 26, 2024

KevinRansom Dec 12, 2024

T-Gro Nov 26, 2024

KevinRansom Dec 12, 2024

T-Gro commented Nov 26, 2024

KevinRansom left a comment

KevinRansom Dec 12, 2024

KevinRansom Dec 12, 2024

KevinRansom Dec 12, 2024

psfinaki left a comment

psfinaki Dec 19, 2024

	When investigating tests issues it is sometimes usefull to examine the artifacts built whe running tests. Those built using the newer test framework are usually,
	When investigating tests issues it is sometimes useful to examine the artifacts built when running tests. Those built using the newer test framework are usually,

Fix #17797 -- Realsig+ generates nested closures with incorrect Generic arguments #17877

Are you sure you want to change the base?

Fix #17797 -- Realsig+ generates nested closures with incorrect Generic arguments #17877

Conversation

KevinRansom commented Oct 13, 2024 • edited Loading

github-actions bot commented Oct 13, 2024 • edited Loading

❗ Release notes required

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

T-Gro commented Nov 26, 2024

KevinRansom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psfinaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom commented Oct 13, 2024 •

edited

Loading

github-actions bot commented Oct 13, 2024 •

edited

Loading