Add much more documentation

Author: Smaug123

src/Compiler/Symbols/Exprs.fs

@@ -509,30 +509,46 @@ module FSharpExprConvert =
     and GetWitnessArgs cenv (env: ExprTranslationEnv) (vref: ValRef) m tps tyargs : FSharpExpr list =
         let g = cenv.g
         if g.langVersion.SupportsFeature(Features.LanguageFeature.WitnessPassing) && not env.suppressWitnesses then
+            /// There are two *conditional* properties a typar can have: equality and comparison.
+            /// A generic type having that constraint may be conditional on whether a specific type parameter to that generic has that
+            /// constraint.
+            /// This function returns `true` iff after unification, the type definition contains any conditional typars.
+            ///
+            /// Note that these conditions are only marked on typars that actually appear in the code, *not* on phantom types.
+            /// So `hasConditionalTypar` should tell us exactly when the type parameter is actually being used in the type's equality or
+            /// comparison.
+            let rec hasConditionalTypar ty =
+                match stripTyEqns g ty with
+                | TType_var (tp, _) -> tp.ComparisonConditionalOn || tp.EqualityConditionalOn
+                | TType_app (_, tinst, _) -> tinst |> List.exists hasConditionalTypar
+                | _ -> false
+
             let witnessExprs =
                 match ConstraintSolver.CodegenWitnessesForTyparInst cenv.tcValF g cenv.amap m tps tyargs with
                 // There is a case where optimized code makes expressions that do a shift-left on the 'char'
                 // type.  There is no witness for this case.  This is due to the code
                 //    let inline HashChar (x:char) = (# "or" (# "shl" x 16 : int #) x : int #)
                 // in FSharp.Core.
                 | ErrorResult _  when vref.LogicalName = "op_LeftShift" && List.isSingleton tyargs -> []
-                // Auto-generated structural comparison/equality code for generic types may call
-                // comparison/equality intrinsics with type parameters that have ComparisonConditionalOn
-                // or EqualityConditionalOn but not actual constraints. In this case, there's no witness
-                // available at compile time - the constraint is satisfied conditionally at runtime.
-                // For nested types like Outer<'a> with field Inner<'a>, the tyargs contains Inner<'a>,
-                // so we must recursively check type arguments of applied types.
-                // See https://github.com/ionide/FSharp.Analyzers.SDK/issues/276
-                | ErrorResult _ when
-                    let rec hasConditionalTypar ty =
-                        match tryDestTyparTy g ty with
-                        | ValueSome tp -> tp.ComparisonConditionalOn || tp.EqualityConditionalOn
-                        | ValueNone ->
-                            // For applied types like Inner<'a>, recursively check type arguments
-                            match ty with
-                            | AppTy g (_, tinst) -> tinst |> List.exists hasConditionalTypar
-                            | _ -> false
-                    tyargs |> List.exists hasConditionalTypar -> []
+                // We don't need a witness either at compile time or runtime when there are conditional typars.
+                // Attempting to call a comparison operation with the type causes a compile-time check that all the generic type args
+                // support comparison (thanks to the ComparisonConditionalOn mechanism); the compile-time check doesn't need witnesses,
+                // it's just pure constraint solving.
+                // Nor do we need a witness for runtime logic: the compiler generates a `CompareTo` method (see
+                // `MakeValsForCompareAugmentation`) which handles the comparison by dynamically type-testing, not going through a witness.
+                //
+                // So we don't need to generate a witness.
+                //
+                // In fact, we *can't* generate a witness, because the constraint on the type parameter is only conditional: a rigid type
+                // parameter, defined without the `comparison` constraint, cannot have the constraint added to it later (that's what "rigid"
+                // means). It would change the type signature of the type to add this constraint to the type parameter!
+                //
+                // This code path is only reached through the auto-generated comparison/equality code, which only calls single-constraint
+                // intrinsics: there's exactly one constraint per type parameter in each of those two cases.
+                // In theory, if a function had an autogenerated `'a : comparison and 'b : SomethingElse`, where the `SomethingElse` was
+                // not comparison and failed for a different reason, we'd spuriously hide that failure here; but in fact the only code
+                // paths which get here have no other constraints.
+                | ErrorResult _ when List.exists hasConditionalTypar tyargs -> []
                 | res -> CommitOperationResult res
             let env = { env with suppressWitnesses = true }
             witnessExprs |> List.map (fun arg -> 

src/Compiler/TypedTree/TypedTreeBasics.fsi

@@ -141,6 +141,8 @@ val tryShortcutSolvedUnitPar: canShortcut: bool -> r: Typar -> Measure
 
 val stripUnitEqnsAux: canShortcut: bool -> unt: Measure -> Measure
 
+/// Follows type variable solutions: when a type variable has been solved by unifying it with another type,
+/// replaces that type variable with its solution.
 val stripTyparEqnsAux: nullness0: Nullness -> canShortcut: bool -> ty: TType -> TType
 
 val replaceNullnessOfTy: nullness: Nullness -> ty: TType -> TType

src/Compiler/TypedTree/TypedTreeOps.fsi

@@ -604,9 +604,41 @@ val reduceTyconRefMeasureableOrProvided: TcGlobals -> TyconRef -> TypeInst -> TT
 
 val reduceTyconRefAbbrevMeasureable: TyconRef -> Measure
 
-/// set bool to 'true' to allow shortcutting of type parameter equation chains during stripping
-val stripTyEqnsA: TcGlobals -> bool -> TType -> TType
-
+/// <summary>
+/// Normalizes types.
+/// </summary>
+/// <remarks>
+/// Normalizes a type by:
+/// <list>
+///   <item>replacing type variables with their solutions found by unification</item>
+///   <item>expanding type abbreviations</item>
+/// </list>
+/// as well as a couple of special-case normalizations:
+/// <list>
+///   <item>identifying <c>int&lt;1&gt;</c> with <c>int</c> (for any measurable type)</item>
+///   <item>identifying <c>byref&lt;'T&gt;</c> with <c>byref&lt;'T, ByRefKinds.InOut&gt;</c></item>
+/// </list>
+/// </remarks>
+/// <param name="canShortcut">
+/// <c>true</c> to allow shortcutting of type parameter equation chains during stripping
+/// </param>
+val stripTyEqnsA: TcGlobals -> canShortcut:bool -> TType -> TType
+
+/// <summary>
+/// Normalizes types.
+/// </summary>
+/// <remarks>
+/// Normalizes a type by:
+/// <list>
+///   <item>replacing type variables with their solutions found by unification</item>
+///   <item>expanding type abbreviations</item>
+/// </list>
+/// as well as a couple of special-case normalizations:
+/// <list>
+///   <item>identifying <c>int&lt;1&gt;</c> with <c>int</c> (for any measurable type)</item>
+///   <item>identifying <c>byref&lt;'T&gt;</c> with <c>byref&lt;'T, ByRefKinds.InOut&gt;</c></item>
+/// </list>
+/// </remarks>
 val stripTyEqns: TcGlobals -> TType -> TType
 
 val stripTyEqnsAndMeasureEqns: TcGlobals -> TType -> TType
@@ -707,6 +739,8 @@ val tcrefOfAppTy: TcGlobals -> TType -> TyconRef
 
 val tryTcrefOfAppTy: TcGlobals -> TType -> TyconRef voption
 
+/// Returns ValueSome if this type is a type variable, even after abbreviations are expanded and
+/// variables have been solved through unification.
 val tryDestTyparTy: TcGlobals -> TType -> Typar voption
 
 val tryDestFunTy: TcGlobals -> TType -> (TType * TType) voption

tests/FSharp.Compiler.Service.Tests/ExprTests.fs

@@ -3520,34 +3520,40 @@ module internal ProjectForWitnessConditionalComparison =
     /// This triggers the bug because it forces conversion of auto-generated comparison code
     let walkAllExpressions (source : string) =
         let fileName1 = System.IO.Path.ChangeExtension(getTemporaryFileName (), ".fs")
-        FileSystem.OpenFileForWriteShim(fileName1).Write(source)
-        let options = createProjectOptions [source] []
-        let exprChecker = FSharpChecker.Create(keepAssemblyContents=true, useTransparentCompiler=CompilerAssertHelpers.UseTransparentCompiler)
-        let wholeProjectResults = exprChecker.ParseAndCheckProject(options) |> Async.RunImmediate
-
-        if wholeProjectResults.Diagnostics.Length > 0 then
-            for diag in wholeProjectResults.Diagnostics do
-                printfn "Diagnostic: %s" diag.Message
-
-        for implFile in wholeProjectResults.AssemblyContents.ImplementationFiles do
-            // Walk all declarations and their expressions, including ImmediateSubExpressions
-            let rec walkExpr (e: FSharpExpr) =
-                // Access ImmediateSubExpressions - this is what triggers the bug
-                for subExpr in e.ImmediateSubExpressions do
-                    walkExpr subExpr
-
-            let rec walkDecl d =
-                match d with
-                | FSharpImplementationFileDeclaration.Entity (_, subDecls) ->
-                    for subDecl in subDecls do
-                        walkDecl subDecl
-                | FSharpImplementationFileDeclaration.MemberOrFunctionOrValue (_, _, e) ->
-                    walkExpr e
-                | FSharpImplementationFileDeclaration.InitAction e ->
-                    walkExpr e
-
-            for decl in implFile.Declarations do
-                walkDecl decl
+        try
+            FileSystem.OpenFileForWriteShim(fileName1).Write(source)
+            let options = createProjectOptions [source] []
+            let exprChecker = FSharpChecker.Create(keepAssemblyContents=true, useTransparentCompiler=CompilerAssertHelpers.UseTransparentCompiler)
+            let wholeProjectResults = exprChecker.ParseAndCheckProject(options) |> Async.RunImmediate
+
+            if wholeProjectResults.Diagnostics.Length > 0 then
+                for diag in wholeProjectResults.Diagnostics do
+                    printfn "Diagnostic: %s" diag.Message
+
+            for implFile in wholeProjectResults.AssemblyContents.ImplementationFiles do
+                // Walk all declarations and their expressions, including ImmediateSubExpressions
+                let rec walkExpr (e: FSharpExpr) =
+                    // Access ImmediateSubExpressions - this is what triggered #19118
+                    for subExpr in e.ImmediateSubExpressions do
+                        walkExpr subExpr
+
+                let rec walkDecl d =
+                    match d with
+                    | FSharpImplementationFileDeclaration.Entity (_, subDecls) ->
+                        for subDecl in subDecls do
+                            walkDecl subDecl
+                    | FSharpImplementationFileDeclaration.MemberOrFunctionOrValue (_, _, e) ->
+                        walkExpr e
+                    | FSharpImplementationFileDeclaration.InitAction e ->
+                        walkExpr e
+
+                for decl in implFile.Declarations do
+                    walkDecl decl
+        finally
+            try
+                FileSystem.FileDeleteShim fileName1
+            with
+            | _ -> ()
 
 [<Fact>]
 let ``ImmediateSubExpressions - generic DU with no constraints should not crash`` () =
@@ -3559,7 +3565,7 @@ module M
 type Bar<'appEvent> =
     | Wibble of 'appEvent
 """
-    // This should not throw - before the fix it crashes with ConstraintSolverMissingConstraint
+    // This should not throw. Before the fix, it crashed with ConstraintSolverMissingConstraint.
     ProjectForWitnessConditionalComparison.walkAllExpressions source
 
 [<Fact>]
@@ -3609,7 +3615,8 @@ and Inner<'b> =
 
 [<Fact>]
 let ``ImmediateSubExpressions - generic DU with explicit comparison constraint works`` () =
-    // When the type parameter HAS the comparison constraint, witness generation should work
+    // When the type parameter has the comparison constraint, witness generation should work;
+    // no crash occurred even before the bug was fixed. This test is here for completeness.
     let source = """
 module M
 
@@ -3620,7 +3627,7 @@ type WithConstraint<'a when 'a : comparison> =
 
 [<Fact>]
 let ``ImmediateSubExpressions - non-generic DU works`` () =
-    // Non-generic types should work fine (no generics = no witness issues)
+    // Non-generic types always worked fine (no generics = no witness issues). This test is here for completeness.
     let source = """
 module M
 
@@ -3632,7 +3639,8 @@ type SimpleUnion =
 
 [<Fact>]
 let ``ImmediateSubExpressions - generic DU with NoComparison attribute should not crash`` () =
-    // With NoComparison, no comparison code is generated, so no crash should occur
+    // With NoComparison, no comparison code is generated, so no crash ever occurred even before the bug was fixed.
+    // This test is here for completeness.
     let source = """
 module M