6.0.400 compilation error with private types in namespace used in a different file

[l3m]

Repro steps

Defining a private type in a namespace and using it in a module in another file in the same namespace leads to a compilation error in 6.0.400.

Compiles without error in 6.0.300.

A simple repro can be found here: https://github.com/l3m/fsharp-error-repro

Expected behavior

Unclear if the behavior in 6.0.300 or 6.0.400 is the actually correct one, but it's certainly a change.

So the expectation would have been that it compiles in 6.0.400

Actual behavior

Fails to compile in 6.0.400

Known workarounds

Replace

type private Blah = ... 

with

type internal Blah = ... 

Related information

Observed on both linux and windows, with dotnet sdk 6.0.300 and 6.0.400

[vzarytovskii]

Looks like this suggestion been fixed
fsharp/fslang-suggestions#389
Let me bisect and find where it was changed.

[vzarytovskii]

Ok, with help of @0101, we bisected to that PR: #13287
CC @nojaf @dsyme

[nojaf]

@vzarytovskii I can't really put my finger on it.
For some reason the Blah type is not being added to

fsharp/src/Compiler/Checking/NameResolution.fsi

Line 215 in bdb6462

eTyconsByAccessNames: LayeredMultiMap<string, TyconRef>

and it is later not found in

fsharp/src/Compiler/Checking/NameResolution.fs

Line 3311 in bdb6462

match LookupTypeNameInEnvHaveArity fullyQualified id.idText staticResInfo.NumStaticArgs nenv with

[nojaf]

The problem appears to go away with a CustomEquality for type ModuleOrNamespaceKind.

[<CustomEquality; NoComparison>]
type ModuleOrNamespaceKind =

    /// Indicates that a module is compiled to a class with the "Module" suffix added. 
    | FSharpModuleWithSuffix 

    /// Indicates that a module is compiled to a class with the same name as the original module 
    | ModuleOrType 

    /// Indicates that a 'module' is really a namespace 
    | Namespace of
        /// Indicates that the sourcecode had a namespace.
        /// If false, this namespace was implicitly constructed during type checking. 
        isExplicit: bool
        
    override this.Equals other =
        match other with
        | :? ModuleOrNamespaceKind as kind ->
            match this, kind with
            | FSharpModuleWithSuffix, FSharpModuleWithSuffix
            | ModuleOrType, ModuleOrType
            | Namespace _, Namespace _ -> true
            | _ -> false
        | _ -> false

    override this.GetHashCode () =
        match this with
        | FSharpModuleWithSuffix -> 0
        | ModuleOrType -> 1
        | Namespace _ -> 2

@vzarytovskii

[vzarytovskii]

Huh, interesting. Do did lookup work before?

[nojaf]

I could be way off here but I think some structure that contains a ModuleOrNamespaceKind is being compared somewhere.

For example:

fsharp/src/Compiler/TypedTree/TypedTree.fs

Lines 503 to 528 in bdb6462

	type CompilationPath =
	| CompPath of ILScopeRef * (string * ModuleOrNamespaceKind) list
	member x.ILScopeRef = let (CompPath(scoref, _)) = x in scoref
	member x.AccessPath = let (CompPath(_, p)) = x in p
	member x.MangledPath = List.map fst x.AccessPath
	member x.NestedPublicPath (id: Ident) = PubPath(Array.append (Array.ofList x.MangledPath) [| id.idText |])
	member x.ParentCompPath =
	let a, _ = List.frontAndBack x.AccessPath
	CompPath(x.ILScopeRef, a)
	member x.NestedCompPath n moduleKind =
	CompPath(x.ILScopeRef, x.AccessPath@[(n, moduleKind)])
	member x.DemangledPath =
	x.AccessPath |> List.map (fun (nm, k) -> CompilationPath.DemangleEntityName nm k)
	/// String 'Module' off an F# module name, if FSharpModuleWithSuffix is used
	static member DemangleEntityName nm k =
	match k with
	| FSharpModuleWithSuffix -> String.dropSuffix nm FSharpModuleSuffix
	| _ -> nm

[dsyme]

Subtle introduction of the bug. Marking CompilationPath as NoEquality may help track track this down, looks like it is the structural equality on this:

[<StructuralEquality; NoComparison; StructuredFormatDisplay("{DebugText}")>]
type Accessibility = 
    | TAccess of compilationPaths: CompilationPath list

[dsyme]

By using this diff https://github.com/dotnet/fsharp/compare/main...dsyme:cwe?expand=1 it looks like these are the origin points for structural equality

let isPublicVal (lv: Val) = (lv.Accessibility = taccessPublic)
let isPublicUnionCase (ucr: UnionCaseRef) = (ucr.UnionCase.Accessibility = taccessPublic)
let isPublicRecdField (rfr: RecdFieldRef) = (rfr.RecdField.Accessibility = taccessPublic)
let isPublicTycon (tcref: Tycon) = (tcref.Accessibility = taccessPublic)

Also this in AccessibilityLogic:

        | AccessibleFrom(cs1, tc1), AccessibleFrom(cs2, tc2) -> (cs1 = cs2) && (match tc1, tc2 with None, None -> true | Some tc1, Some tc2 -> tyconRefEq g tc1 tc2 | _ -> false)

One option is to switch to NoEquality and NoComparison on Accessibility and CompilationPath and use an explicit equality routine in the above places.

Another is to use the custom equality like @nojaf above, though I think perhaps just making everything more explicit is better. There are enough notions of equality needed in the compiler its better to be explicit for anything in TypedTree

[0101]

This particular case seems to be caused by the comparison made here:

fsharp/src/Compiler/TypedTree/TypedTreeBasics.fs

Lines 443 to 450 in 5420f09

	let inline canAccessCompPathFrom (CompPath(scoref1, cpath1)) (CompPath(scoref2, cpath2)) =
	let rec loop p1 p2 =
	match p1, p2 with
	| (a1, k1) :: rest1, (a2, k2) :: rest2 -> (a1=a2) && (k1=k2) && loop rest1 rest2
	| [], _ -> true
	| _ -> false // cpath1 is longer
	loop cpath1 cpath2 &&
	(scoref1 = scoref2)

But I imagine it could also cause a similar issue here:

fsharp/src/Compiler/Symbols/Symbols.fs

Lines 125 to 132 in 5420f09

	let canAccessCompPathFromCrossProject (CompPath(scoref1, cpath1)) (CompPath(scoref2, cpath2)) =
	let rec loop p1 p2 =
	match p1, p2 with
	| (a1, k1) :: rest1, (a2, k2) :: rest2 -> (a1=a2) && (k1=k2) && loop rest1 rest2
	| [], _ -> true
	| _ -> false // cpath1 is longer
	loop cpath1 cpath2 &&
	nameOfScoRef thisCcu1 scoref1 = nameOfScoRef thisCcu2 scoref2

Switching Accessibility and CompilationPath to NoEquality wouldn't really force the fix here since the equality comparison is performed directly on ModuleOrNamespaceKind.

We can mark that one as NoEquality but that will add quite a bit of extra equality code in different places. So I would lean more towards @nojaf 's custom equality there. Especially since I don't see a need for the current default version where a implicitly defined namespace is considered different than the same but explicitly defined one (though I might be wrong there).