[Python] Type annotation and Option handling improvement (#4328)

Author: dbrattli

pyrightconfig.ci.json

@@ -5,7 +5,6 @@
     "**/node_modules/**",
     "temp/fable-library-py/fable_library/list.py",
     "temp/tests/Python/test_applicative.py",
-    "temp/tests/Python/test_map.py",
     "temp/tests/Python/test_misc.py",
     "temp/tests/Python/test_type.py",
     "temp/tests/Python/fable_modules/thoth_json_python/encode.py"

src/Fable.Build/Test/Python.fs

@@ -67,7 +67,10 @@ let handle (args: string list) =
     else
 
         // Test against .NET
-        Command.Run("dotnet", "test -c Release", workingDirectory = sourceDir)
+        if compileOnly then
+            printfn "Skipping .NET test execution (--compile-only specified)"
+        else
+            Command.Run("dotnet", "test -c Release", workingDirectory = sourceDir)
 
         // Test against Python
         Command.Fable(fableArgs, workingDirectory = buildDir)
@@ -80,7 +83,7 @@ let handle (args: string list) =
 
         // Run pytest
         if compileOnly then
-            printfn "Skipping test execution (--compile-only specified)"
+            printfn "Skipping Python test execution (--compile-only specified)"
         else
             Command.Run("uv", $"run pytest {buildDir} -x")
 

src/Fable.Transforms/FableTransforms.fs

@@ -228,10 +228,17 @@ let noSideEffectBeforeIdent identName expr =
 
     findIdentOrSideEffect expr && not sideEffect
 
-let canInlineArg com identName value body =
+let canInlineArg (com: Compiler) identName value body =
     match value with
     | Value((Null _ | UnitConstant | TypeInfo _ | BoolConstant _ | NumberConstant _ | CharConstant _), _) -> true
-    | Value(StringConstant s, _) -> s.Length < 100
+    | Value(StringConstant s, _) ->
+        match com.Options.Language with
+        | Python ->
+            // Only inline short strings if they're referenced at most once,
+            // to avoid duplicating the literal in generated code (which can cause
+            // issues like property access on string literals in Python)
+            s.Length < 100 && countReferencesUntil 2 identName body <= 1
+        | _ -> s.Length < 100
     | _ ->
         let refCount = countReferencesUntil 2 identName body
 

src/Fable.Transforms/FableTransforms.fsi

@@ -5,6 +5,7 @@ open Fable.AST.Fable
 
 val isIdentCaptured: identName: string -> expr: Expr -> bool
 val isTailRecursive: identName: string -> expr: Expr -> bool * bool
+val countReferencesUntil: limit: int -> identName: string -> body: Expr -> int
 val replaceValues: replacements: Map<string, Expr> -> expr: Expr -> Expr
 val uncurryType: typ: Type -> Type
 

src/Fable.Transforms/Python/Fable2Python.Annotation.fs

@@ -543,7 +543,7 @@ let makeEntityTypeAnnotation com ctx (entRef: Fable.EntityRef) genArgs repeatedG
     | Types.iobservableGeneric, _ ->
         let resolved, stmts = resolveGenerics com ctx genArgs repeatedGenerics
         fableModuleAnnotation com ctx "observable" "IObservable" resolved, stmts
-    | Types.idictionary, _ -> stdlibModuleTypeHint com ctx "collections.abc" "MutableMapping" genArgs repeatedGenerics
+    | Types.idictionary, _ -> stdlibModuleTypeHint com ctx "collections.abc" "Mapping" genArgs repeatedGenerics
     | Types.ievent2, _ ->
         // IEvent<'Delegate, 'Args> - only use Args (second param) since Delegate is phantom in Python
         let argsType = genArgs |> List.tryItem 1 |> Option.defaultValue Fable.Any
@@ -559,7 +559,7 @@ let makeEntityTypeAnnotation com ctx (entRef: Fable.EntityRef) genArgs repeatedG
     | "Fable.Core.Py.Set`1", _ ->
         let resolved, stmts = resolveGenerics com ctx genArgs repeatedGenerics
         fableModuleAnnotation com ctx "protocols" "ISet_1" resolved, stmts
-    | "Fable.Core.Py.Map`2", _ ->
+    | "Py.Mapping.IMapping`2", _ ->
         let resolved, stmts = resolveGenerics com ctx genArgs repeatedGenerics
         fableModuleAnnotation com ctx "protocols" "IMap" resolved, stmts
     | "Fable.Core.Py.Callable", _ ->
@@ -581,7 +581,16 @@ let makeEntityTypeAnnotation com ctx (entRef: Fable.EntityRef) genArgs repeatedG
         let isErased =
             ent.Attributes |> Seq.exists (fun att -> att.Entity.FullName = Atts.erase)
 
-        if ent.IsInterface && not isErased then
+        // Check for [<Global>] attribute - use the global name directly as the type annotation
+        match com, ent.Attributes with
+        | FSharp2Fable.Util.GlobalAtt(Some customName) ->
+            // Use the custom global name (e.g., "list" for [<Global("list")>])
+            makeGenericTypeAnnotation com ctx customName genArgs repeatedGenerics, []
+        | FSharp2Fable.Util.GlobalAtt None ->
+            // Use the entity's display name
+            let name = Helpers.removeNamespace ent.FullName
+            makeGenericTypeAnnotation com ctx name genArgs repeatedGenerics, []
+        | _ when ent.IsInterface && not isErased ->
             let name = Helpers.removeNamespace ent.FullName
 
             // If the interface is imported then it's erased and we need to add the actual imports
@@ -597,10 +606,10 @@ let makeEntityTypeAnnotation com ctx (entRef: Fable.EntityRef) genArgs repeatedG
                 | _ -> ()
 
             makeGenericTypeAnnotation com ctx name genArgs repeatedGenerics, []
-        elif isErased then
+        | _ when isErased ->
             // Erased types should use Any for type annotations
             stdlibModuleTypeHint com ctx "typing" "Any" [] repeatedGenerics
-        else
+        | _ ->
             match tryPyConstructor com ctx ent with
             | Some(entRef, stmts) ->
                 match entRef with

src/Fable.Transforms/Python/Fable2Python.Transforms.fs

@@ -69,16 +69,7 @@ let getMemberArgsAndBody (com: IPythonCompiler) ctx kind hasSpread (args: Fable.
     let args, body, returnType, _typeParams =
         Annotation.transformFunctionWithAnnotations com ctx funcName args body
 
-    let args =
-        let len = args.Args.Length
-
-        if not hasSpread || len = 0 then
-            args
-        else
-            { args with
-                VarArg = Some { args.Args[len - 1] with Annotation = None }
-                Args = args.Args[.. len - 2]
-            }
+    let args = adjustArgsForSpread hasSpread args
 
     args, body, returnType
 
@@ -740,6 +731,16 @@ let transformCallArgs
     let hasSpread =
         paramsInfo |> Option.map (fun i -> i.HasSpread) |> Option.defaultValue false
 
+    // Helper to transform an arg and wrap with widen() if needed
+    let transformArgWithWiden (sigType: Fable.Type option) (argExpr: Fable.Expr) =
+        let expr, stmts = com.TransformAsExpr(ctx, argExpr)
+
+        if needsOptionWidenForArg sigType argExpr then
+            let widen = com.TransformImport(ctx, "widen", getLibPath com "option")
+            Expression.call (widen, [ expr ]), stmts
+        else
+            expr, stmts
+
     let args, stmts' =
         match args with
         | [] -> [], []
@@ -759,7 +760,14 @@ let transformCallArgs
 
                 let expr, stmts' = com.TransformAsExpr(ctx, last)
                 rest @ [ Expression.starred expr ], stmts @ stmts'
-        | args -> List.map (fun e -> com.TransformAsExpr(ctx, e)) args |> Helpers.unzipArgs
+        | args ->
+            // Transform args with widen() where needed based on signature types
+            args
+            |> List.mapi (fun i e ->
+                let sigType = List.tryItem i callInfo.SignatureArgTypes
+                transformArgWithWiden sigType e
+            )
+            |> Helpers.unzipArgs
 
     match objArg with
     | None -> args, [], stmts @ stmts'
@@ -1515,7 +1523,12 @@ let getDecisionTargetAndBoundValues (com: IPythonCompiler) (ctx: Context) target
         let bindings, replacements =
             (([], Map.empty), identsAndValues)
             ||> List.fold (fun (bindings, replacements) (ident, expr) ->
-                if canHaveSideEffects com expr then
+                // Only inline if the expression has no side effects AND is referenced at most once.
+                // If referenced multiple times, we should bind to a variable to avoid duplicating
+                // the expression (which can cause issues like accessing properties on literals).
+                let refCount = FableTransforms.countReferencesUntil 2 ident.Name target
+
+                if canHaveSideEffects com expr || refCount > 1 then
                     (ident, expr) :: bindings, replacements
                 else
                     bindings, Map.add ident.Name expr replacements
@@ -4129,23 +4142,42 @@ let transformInterface (com: IPythonCompiler) ctx (classEnt: Fable.Entity) (_cla
                     // Make protocol method parameters positional-only (using /) to avoid
                     // parameter name mismatch errors when subclasses use different names
                     // (e.g., value_1 instead of value due to closure captures)
+                    let allParams =
+                        memb.CurriedParameterGroups
+                        |> Seq.indexed
+                        |> Seq.collect (fun (n, parameterGroup) ->
+                            parameterGroup |> Seq.indexed |> Seq.map (fun (m, pg) -> (n + m, pg))
+                        )
+                        |> Seq.toList
+
+                    // Split regular params from vararg param using shared helper
+                    let regularParams, varArgParam = splitVarArg memb.HasSpread allParams
+
                     let posOnlyArgs =
                         [
                             if memb.IsInstance then
                                 Arg.arg "self"
 
-                            for n, parameterGroup in memb.CurriedParameterGroups |> Seq.indexed do
-                                for m, pg in parameterGroup |> Seq.indexed do
-                                    // Uncurry function types to match class implementations
-                                    // F# interface uses curried types (LambdaType) but class methods
-                                    // use uncurried types (DelegateType) at runtime
-                                    let paramType = FableTransforms.uncurryType pg.Type
-                                    let ta, _ = Annotation.typeAnnotation com ctx None paramType
+                            for idx, pg in regularParams do
+                                // Uncurry function types to match class implementations
+                                // F# interface uses curried types (LambdaType) but class methods
+                                // use uncurried types (DelegateType) at runtime
+                                let paramType = FableTransforms.uncurryType pg.Type
+                                let ta, _ = Annotation.typeAnnotation com ctx None paramType
 
-                                    Arg.arg (pg.Name |> Option.defaultValue $"__arg%d{n + m}", annotation = ta)
+                                Arg.arg (pg.Name |> Option.defaultValue $"__arg%d{idx}", annotation = ta)
                         ]
 
-                    Arguments.arguments (posonlyargs = posOnlyArgs)
+                    // For vararg parameter, extract element type from array type for annotation
+                    let vararg =
+                        varArgParam
+                        |> Option.map (fun (_idx, pg) ->
+                            let elementType = getVarArgElementType pg.Type
+                            let ta, _ = Annotation.typeAnnotation com ctx None elementType
+                            Arg.arg (pg.Name |> Option.defaultValue "rest", annotation = ta)
+                        )
+
+                    Arguments.arguments (posonlyargs = posOnlyArgs, ?vararg = vararg)
 
                 // Also uncurry return type for consistency with class implementations
                 let uncurriedReturnType = FableTransforms.uncurryType memb.ReturnParameter.Type

src/Fable.Transforms/Python/Fable2Python.Util.fs

@@ -39,6 +39,36 @@ module Util =
         | [] -> [ Statement.Pass ]
         | _ -> stmts
 
+    /// Extract the element type from an array type for vararg annotations.
+    /// For Array<T>, returns T. For other types, returns Any as fallback.
+    let getVarArgElementType (paramType: Fable.Type) : Fable.Type =
+        match paramType with
+        | Fable.Array(elemType, _) -> elemType
+        | _ -> Fable.Any // Fallback if not array type
+
+    /// Splits a list of items into regular items and an optional vararg item.
+    /// When hasSpread is true and items is non-empty, the last item becomes the vararg.
+    let splitVarArg<'T> (hasSpread: bool) (items: 'T list) : 'T list * 'T option =
+        if hasSpread && not items.IsEmpty then
+            let regular = items |> List.take (items.Length - 1)
+            let vararg = items |> List.last
+            regular, Some vararg
+        else
+            items, None
+
+    /// Adjusts Arguments to move the last arg to vararg when hasSpread is true.
+    /// Removes the annotation from vararg since Python infers it from *args.
+    let adjustArgsForSpread (hasSpread: bool) (args: Arguments) : Arguments =
+        let len = args.Args.Length
+
+        if not hasSpread || len = 0 then
+            args
+        else
+            { args with
+                VarArg = Some { args.Args[len - 1] with Annotation = None }
+                Args = args.Args[.. len - 2]
+            }
+
     let hasAttribute fullName (atts: Fable.Attribute seq) =
         atts |> Seq.exists (fun att -> att.Entity.FullName = fullName)
 
@@ -79,6 +109,55 @@ module Util =
         | Fable.Call _ -> needsOptionEraseForCall expectedReturnType
         | _ -> false
 
+    /// Recursively check if a type contains Option with generic parameter that requires wrapping.
+    /// This checks return types of lambdas for Option[GenericParam].
+    let rec private hasWrappedOptionInReturnType (typ: Fable.Type) =
+        match typ with
+        | Fable.LambdaType(_, returnType) ->
+            // Check if return type is Option[GenericParam] or recurse into nested lambdas
+            match returnType with
+            | Fable.Option(inner, _) -> mustWrapOption inner
+            | Fable.LambdaType _ -> hasWrappedOptionInReturnType returnType
+            | _ -> false
+        | _ -> false
+
+    /// Check if a type would have its Option return type erased (T | None instead of Option[T]).
+    /// Returns true when the return type is Option[ConcreteType] which gets erased.
+    let rec private hasErasedOptionReturnType (typ: Fable.Type) =
+        match typ with
+        | Fable.LambdaType(_, returnType) ->
+            match returnType with
+            | Fable.Option(inner, _) -> not (mustWrapOption inner) // Erased when NOT wrapped
+            | Fable.LambdaType _ -> hasErasedOptionReturnType returnType
+            | _ -> false
+        | _ -> false
+
+    /// Check if a callback argument needs widen() to convert erased Option callback
+    /// to wrapped Option form for type checker compatibility.
+    /// Returns true when:
+    /// - Expected type is Callable with Option[GenericParam] in return position
+    /// - Actual arg is a lambda/function that would have erased Option return type
+    let needsOptionWidenForArg (expectedType: Fable.Type option) (argExpr: Fable.Expr) =
+        match expectedType with
+        | Some sigType when hasWrappedOptionInReturnType sigType ->
+            // Check if argument is a callable with ERASED Option return type
+            // If arg already has wrapped Option return, don't apply widen()
+            match argExpr with
+            | Fable.Lambda _
+            | Fable.Delegate _ ->
+                // Check if the lambda's return type would be erased
+                hasErasedOptionReturnType argExpr.Type
+            | Fable.IdentExpr ident ->
+                // Check if the identifier's type has erased Option return
+                hasErasedOptionReturnType ident.Type
+            | Fable.Get(_, Fable.FieldGet fieldInfo, _, _) ->
+                // Field access to a function
+                match fieldInfo.FieldType with
+                | Some typ -> hasErasedOptionReturnType typ
+                | None -> false
+            | _ -> false
+        | _ -> false
+
     /// Wraps None values in cast(type, None) for type safety.
     /// Skips if type annotation is also None (unit type).
     let wrapNoneInCast (com: IPythonCompiler) ctx (value: Expression) (typeAnnotation: Expression) : Expression =

src/Fable.Transforms/Python/PythonPrinter.fs

@@ -143,11 +143,13 @@ module PrinterExtensions =
                 if i < args.Length - 1 then
                     printer.Print(", ")
 
-            match arguments.Args, arguments.VarArg with
-            | [], Some vararg ->
+            match arguments.PosOnlyArgs, arguments.Args, arguments.VarArg with
+            | [], [], Some vararg ->
+                // No positional args at all, just print *vararg
                 printer.Print("*")
                 printer.Print(vararg)
-            | _, Some vararg ->
+            | _, _, Some vararg ->
+                // Has positional-only or regular args, need comma before *vararg
                 printer.Print(", *")
                 printer.Print(vararg)
             | _ -> ()

src/Fable.Transforms/Python/Replacements.fs

@@ -2590,12 +2590,21 @@ let dictionaries (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Exp
     | "get_Item", _ ->
         Helper.LibCall(com, "map_util", "getItemFromDict", t, args, i.SignatureArgTypes, ?thisArg = thisArg, ?loc = r)
         |> Some
-    | ReplaceName [ "set_Item", "set"
-                    "get_Keys", "keys"
-                    "get_Values", "values"
-                    "ContainsKey", "has"
-                    "Clear", "clear" ] methName,
-      Some c -> Helper.InstanceCall(c, methName, t, args, i.SignatureArgTypes, ?loc = r) |> Some
+    | "get_Keys", Some c ->
+        // Wrap .keys() with to_enumerable since KeysView doesn't implement IEnumerable_1
+        let keysCall =
+            Helper.InstanceCall(c, "keys", t, args, i.SignatureArgTypes, ?loc = r)
+
+        Helper.LibCall(com, "util", "to_enumerable", t, [ keysCall ], ?loc = r) |> Some
+    | "get_Values", Some c ->
+        // Wrap .values() with to_enumerable since ValuesView doesn't implement IEnumerable_1
+        let valuesCall =
+            Helper.InstanceCall(c, "values", t, args, i.SignatureArgTypes, ?loc = r)
+
+        Helper.LibCall(com, "util", "to_enumerable", t, [ valuesCall ], ?loc = r)
+        |> Some
+    | ReplaceName [ "set_Item", "set"; "ContainsKey", "has"; "Clear", "clear" ] methName, Some c ->
+        Helper.InstanceCall(c, methName, t, args, i.SignatureArgTypes, ?loc = r) |> Some
     | _ -> None
 
 let hashSets (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Expr option) (args: Expr list) =