[WIP] 12366: Improve closure names

[dsyme]

This makes some small improvements to the names we use for closures, see https://github.com/dotnet/fsharp/issues/12366

Closure names are <prefix>@<line-number>[<disambiguation-suffix>]. We previously used the following rule for the prefix:

If inside a non-compiler-generated "let" or "member" definition then use the compiled name of the value being defined. Otherwise use clo

This PR adjusts (1) to:

If inside a top-level "let" or "member" definition, or a locally defined function, then use the compiled name of the value being defined. Otherwise use clo.

If the closure is being generated into "<StartupCode..." then the prefix becomes <entity>::<function-or-member-name> where "" is the core display name of the entity responsible for the generation of the closure

This addresses two problems

1. avoids the use names of simple values let x = ....
2. avoids a sea of "clo@456" in "<StartupCode..." especially in optimized code

One particular example of (1) is that closure names like Pipe #3 input at line 50@52 are removed - these crept in with the implementation of pipeline debugging and the name of the generated debug local was leaking into closures here. Instead you now get something like "shortDigits@54-2". There are many examples of these in the code generated baselines

[dsyme]

@Happypig375 Thanks for the +1. I'd love your thoughts on how we can improve things further.

[Happypig375]

You're welcome. "shortDigits@54-2" -> "shortDigits@line54closure2" would be more understandable for beginners?

[dsyme]

You're welcome. "shortDigits@54-2" -> "shortDigits@line54closure2" would be more understandable for beginners?

Maybe

shortDigits@54-2 -> shortDigits@line-54-closure-2

[Happypig375]

Can we emit spaces? shortDigits @ line 54, closure 2

[dsyme]

Can we emit spaces? shortDigits @ line 54, closure 2

In theory I think yes

[KevinRansom]

@dsyme, do you want to use spaces or hyphens?

[dsyme]

@dsyme, do you want to use spaces or hyphens?

I'm genuinely not sure, let's discuss here https://github.com/dotnet/fsharp/issues/12366#issuecomment-974232138

We could accept this PR as it represents an improvement as far as I can see. But let's discuss first

[dsyme]

Baselines will need updating

[edgarfgp]

Would love to have this, seems to be a good improvement in the debugging experience. Is there anything I can help to progress with this :)? @dsyme @vzarytovskii

[dsyme]

Would love to have this, seems to be a good improvement in the debugging experience. Is there anything I can help to progress with this :)? @dsyme @vzarytovskii

I've merged - I guess try it out and update the baselines? I think that's the only thing remaining

[edgarfgp]

@vzarytovskii @T-Gro This seems to be a good candidate for F# 8. Any thoughts?

[vzarytovskii]

@vzarytovskii @T-Gro This seems to be a good candidate for F# 8. Any thoughts?

We don't want to take any more new features for F# 8. Consider feature set + nullable being frozen.

We'll be spending time until release on stabilizing and testing existing features.

[edgarfgp]

@vzarytovskii @T-Gro This seems to be a good candidate for F# 8. Any thoughts? We won't be taking any more new features for F# 8. Consider feature set + nullable being frozen.

We'll be spending time until release on stabilizing and testing existing features.

Thanks for the reply. This is not a new feature perse, it is arguably an improvement on how the closures names are reported. with will help a lot in debugging

Edit: But will need some testing I imagine so it is fair I guess

[vzarytovskii]

@vzarytovskii @T-Gro This seems to be a good candidate for F# 8. Any thoughts? We won't be taking any more new features for F# 8. Consider feature set + nullable being frozen.

We'll be spending time until release on stabilizing and testing existing features.

Thanks for the reply. This is not a new feature perse, it is arguably an improvement on how the closures names are reported. with will help a lot in debugging

Edit: But will need some testing I imagine so it is fair I guess

Oh, OK, misread the pr. Codegen changes are fine, if they're safe (i.e. don't require a feature flag)

[edgarfgp]

@vzarytovskii @T-Gro This seems to be a good candidate for F# 8. Any thoughts? We won't be taking any more new features for F# 8. Consider feature set + nullable being frozen.

We'll be spending time until release on stabilizing and testing existing features.

Thanks for the reply. This is not a new feature perse, it is arguably an improvement on how the closures names are reported. with will help a lot in debugging Edit: But will need some testing I imagine so it is fair I guess

Oh, OK, misread the pr. Codegen changes are fine, if they're safe (i.e. don't require a feature flag)

Would it be Ok if I take these commits and applied to a new branch? So I can update the baselines and do some testing?

[vzarytovskii]

@vzarytovskii @T-Gro This seems to be a good candidate for F# 8. Any thoughts? We won't be taking any more new features for F# 8. Consider feature set + nullable being frozen.

We'll be spending time until release on stabilizing and testing existing features.

Thanks for the reply. This is not a new feature perse, it is arguably an improvement on how the closures names are reported. with will help a lot in debugging Edit: But will need some testing I imagine so it is fair I guess

Oh, OK, misread the pr. Codegen changes are fine, if they're safe (i.e. don't require a feature flag)

Would it be Ok if I take these commits and applied to a new branch? So I can update the baselines and do some testing?

Sure, that'd be great.

[edgarfgp]

Unfortunately have not had luck trying to cherry pick / apply the changes manually but the diff is significant.