fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

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

Open l3m opened this issue 3 years ago • 8 comments

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

l3m avatar Aug 10 '22 10:08 l3m

Looks like this suggestion been fixed https://github.com/fsharp/fslang-suggestions/issues/389 Let me bisect and find where it was changed.

vzarytovskii avatar Aug 10 '22 11:08 vzarytovskii

Ok, with help of @0101, we bisected to that PR: https://github.com/dotnet/fsharp/pull/13287 CC @nojaf @dsyme

vzarytovskii avatar Aug 10 '22 15:08 vzarytovskii

@vzarytovskii I can't really put my finger on it. For some reason the Blah type is not being added to https://github.com/dotnet/fsharp/blob/bdb64624f0ca220ca4433c83d02dd5822fe767a5/src/Compiler/Checking/NameResolution.fsi#L215

and it is later not found in

https://github.com/dotnet/fsharp/blob/bdb64624f0ca220ca4433c83d02dd5822fe767a5/src/Compiler/Checking/NameResolution.fs#L3311

nojaf avatar Aug 11 '22 09:08 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

nojaf avatar Aug 11 '22 10:08 nojaf

Huh, interesting. Do did lookup work before?

vzarytovskii avatar Aug 11 '22 12:08 vzarytovskii

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

For example: https://github.com/dotnet/fsharp/blob/bdb64624f0ca220ca4433c83d02dd5822fe767a5/src/Compiler/TypedTree/TypedTree.fs#L503-L528

nojaf avatar Aug 11 '22 12:08 nojaf

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 avatar Aug 11 '22 17:08 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

dsyme avatar Aug 11 '22 17:08 dsyme

This particular case seems to be caused by the comparison made here: https://github.com/dotnet/fsharp/blob/5420f094882d1e16e970727f7474ce06cbe57b5a/src/Compiler/TypedTree/TypedTreeBasics.fs#L443-L450

But I imagine it could also cause a similar issue here: https://github.com/dotnet/fsharp/blob/5420f094882d1e16e970727f7474ce06cbe57b5a/src/Compiler/Symbols/Symbols.fs#L125-L132

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).

0101 avatar Aug 15 '22 16:08 0101

Is there a chance we could get this fix rolled out soon? The SDK version 6.0.401 (released yesterday) is still failing to build our project even though the fix was merged one month ago.

stroborobo avatar Sep 14 '22 16:09 stroborobo

Is there a chance we could get this fix rolled out soon? The SDK version 6.0.401 (released yesterday) is still failing to build our project even though the fix was merged one month ago.

Fix was only merged to .NET7 branch, we couldn't do servicing to 401, it was too late. One of future 6.x.x servicing is a target.

vzarytovskii avatar Sep 14 '22 16:09 vzarytovskii

Ok, thanks for the quick response. We'll have to hold out another month or two then. :'(

stroborobo avatar Sep 14 '22 16:09 stroborobo

Ok, thanks for the quick response. We'll have to hold out another month or two then. :'(

Yeah, sorry for that, was a bit unfortunate timing on our side.

vzarytovskii avatar Sep 14 '22 16:09 vzarytovskii