Skip accessibility checks for compiler-generated pattern inputs when used within the same module scope.

[edgarfgp]

Description

BEFORE

module private PM = 
    type PT = 
        abstract A : int
    let a = { new PT with member __.A = 1 } // ok
    let b, c = // error FS0410: The type 'PT' is less accessible than the value, member or type 'val patternInput : PM.PT * PM.PT' it is used in

        { new PT with member __.A = 1 }
        , { new PT with member __.A = 1 }

AFTER

module private PM = 
    type PT = 
        abstract A : int
    let a = { new PT with member __.A = 1 } // Ok
    let b, c = //  Ok
        { new PT with member __.A = 1 }
        , { new PT with member __.A = 1 }

Fixes #4161

Checklist

• Test cases added
• Release notes entry updated

[github-actions]

❗ 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.300.md

[vzarytovskii]

Does it interact with realsig somehow? Also, probably should have tests which check runtime as well, to make sure we're not breaking any IL rules.

[edgarfgp]

Does it interact with realsig somehow?

No idea. I tried withRealInternalSignature true and does not seem to make a difference. I imagine @KevinRansom will know.

Also, probably should have tests which check runtime as well, to make sure we're not breaking any IL rules.

Sure.

[majocha]

Test failure unrelated. I can repro it on main. Looks like some recently merged change made multiemit unstable.

[brianrourkeboll]

OK, now I'm wondering why the compiler emits a whole separate property and backing field for the pattern input here at all instead of eliding it.

namespace N

module internal M =
    type T () = class end

    let t, t' = T (), T ()

using System;
using System.Diagnostics;
using System.Reflection;
using System.Runtime.CompilerServices;
using <StartupCode$_>;
using Microsoft.FSharp.Core;
using N;

[assembly: FSharpInterfaceDataVersion(2, 0, 0)]
[assembly: AssemblyVersion("0.0.0.0")]

namespace N
{
    [CompilationMapping(SourceConstructFlags.Module)]
    internal static class M
    {
        [Serializable]
        [CompilationMapping(SourceConstructFlags.ObjectType)]
        internal class T
        {
        }

        [CompilationMapping(SourceConstructFlags.Value)]
        internal static Tuple<T, T> patternInput@6
        {
            get
            {
                return $_.patternInput@6;
            }
        }

        [CompilationMapping(SourceConstructFlags.Value)]
        internal static T t'
        {
            get
            {
                return $_.t'@6;
            }
        }

        [CompilationMapping(SourceConstructFlags.Value)]
        internal static T t
        {
            get
            {
                return $_.t@6;
            }
        }
    }
}

namespace <StartupCode$_>
{
    internal static class $_
    {
        [DebuggerBrowsable(DebuggerBrowsableState.Never)]
        internal static Tuple<M.T, M.T> patternInput@6;

        [DebuggerBrowsable(DebuggerBrowsableState.Never)]
        internal static M.T t'@6;

        [DebuggerBrowsable(DebuggerBrowsableState.Never)]
        internal static M.T t@6;

        [DebuggerBrowsable(DebuggerBrowsableState.Never)]
        [CompilerGenerated]
        [DebuggerNonUserCode]
        internal static int init@;

        public static void main@()
        {
            patternInput@6 = new Tuple<M.T, M.T>(new M.T(), new M.T());
            t'@6 = M.patternInput@6.Item2;
            t@6 = M.patternInput@6.Item1;
        }
    }
}

At first I considered whether it might be to handle something like active patterns with side effects... But even there, the compiler emits a property and backing field to store and retrieve the active pattern result only to immediately store it in the backing field emitted for x:

module M =
    let (|A|) x = printfn "lol"; A x

    let (A x) = 1

I wonder if getting rid of the redundant "pattern input" field/property here would make the problem this PR is addressing not exist to begin with. (Of course, there may be other scenarios where this behavior is required that I'm not thinking of... But it seems definitely unnecessary in these examples.)

[edgarfgp]

@brianrourkeboll That sounds interesting. Let's see what the team says.

[edgarfgp]

This is ready.

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[KevinRansom]

Okay ... this is not the right fix.

I don't know what the correct fix is yet, but it is a bit more fundamental than skipping visibility checks for generated code. At the end of the day it boils down to the way that F# handles class initialization.

C# initializes classes in the order that they are encountered executing the process, which is always the most performant choice. I.e. a class isn't initialized until some other class touches it (reads a value, invokes a static method, constructs an instance ...)

F#, however, initializes types in the order the fields are encountered in the source file. Effectively every source file is a bit like a script that is processed from beginning to end. In order to achieve this F# creates a source file initialization class, that invokes a property on the class holding the module or type. In order to access these properties they need to be at least internal. Realsig addresses this using nested classes and class initialization function, but the type checker knows nothing about realsig so the old non realsig approach is operant.

The problem in the example with tuples is that the F# sees a tuple and generates a closure to represent it as a type. This type is generated as an internal type. Now the function isLessAccessible tyconAcc valAcc is for checking the relative visibility of two F# types, but valAcc is actually a generated ILType, and the F# compiler has conveniently forgotten (better yet, was never clearly told) the visibility of the binding that caused it to be generated. So as far as the type checker is concerned at this point, there is an internal type which has a couple of F# private types on it, which it doesn't think are accessible (because when code gen happens everything private ends up being internal, it all works out nicely.)

The fix is probably something along the lines of ensuring closures have an F# representation or something, so that these types of visibility checks work correctly, or failing that pass down the scope of the binding containing the closure, although that probably has many implementation difficulties of their own.

I'm sorry this is such a long explanation, but every time I look at this PR I struggle with it, because the code is fine, it does what it says on the tin, and it causes the examples to work. However, it introduces a wart that no one will understand in future years. Similar to the reasons I still haven't figured out how to make the optimizer understand realsig fully, because the optimizing codegen has similar warts.

Thanks for looking at this.

Kevin

[edgarfgp]

The fix is probably something along the lines of ensuring closures have an F# representation or something, so that these types of visibility checks work correctly, or failing that pass down the scope of the binding containing the closure, although that probably has many implementation difficulties of their own.

I sort of came to the same conclusion but currently the visibility checks are very ad-hoc feels like playing wacamole.

I hope at least this PR bring attention to this 8 years old bug.

[KevinRansom]

@edgarfgp , thanks mate, I shall close this. I will add the bug to the list of things about realsig to address.