Fix #17904 and #19020

docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@@ -1,5 +1,6 @@
 ### Fixed
 
+* Fix `[<return: X>]` prefix attributes being silently dropped on class members, and fix false-positive `AllowMultiple=false` errors when `[<X>]` and `[<return: X>]` are applied to the same binding. ([Issue #17904](https://github.com/dotnet/fsharp/issues/17904), [Issue #19020](https://github.com/dotnet/fsharp/issues/19020), [PR #19738](https://github.com/dotnet/fsharp/pull/19738))
 * Fix attributes on return type of unparenthesized tuple methods being silently dropped from IL. ([Issue #462](https://github.com/dotnet/fsharp/issues/462), [PR #19714](https://github.com/dotnet/fsharp/pull/19714))
 * Fix internal error FS0073 "Undefined or unsolved type variable" in IlxGen when nested inline SRTP functions with multiple overloads leave unsolved typars in the non-witness codegen path. ([Issue #19709](https://github.com/dotnet/fsharp/issues/19709), [PR #19710](https://github.com/dotnet/fsharp/pull/19710))
 * Fix NRE when calling virtual Object methods on value types through inline SRTP functions. ([Issue #8098](https://github.com/dotnet/fsharp/issues/8098), [PR #19511](https://github.com/dotnet/fsharp/pull/19511))

src/Compiler/Checking/Expressions/CheckExpressions.fs

@@ -11169,7 +11169,7 @@ and TcNormalizedBinding declKind (cenv: cenv) env tpenv overallTy safeThisValOpt
     let envinner = AddDeclaredTypars NoCheckForDuplicateTypars (enclosingDeclaredTypars@declaredTypars) env
 
     match bind with
-    | NormalizedBinding(vis, kind, isInline, isMutable, attrs, xmlDoc, _, valSynData, pat, NormalizedBindingRhs(spatsL, rtyOpt, rhsExpr), _, debugPoint) ->
+    | NormalizedBinding(vis, kind, isInline, isMutable, attrs, xmlDoc, _, valSynData, pat, NormalizedBindingRhs(spatsL, _, rhsExpr), _, debugPoint) ->
         let (SynValData(memberFlags = memberFlagsOpt)) = valSynData
         let mBinding = pat.Range
 
@@ -11223,29 +11223,17 @@ and TcNormalizedBinding declKind (cenv: cenv) env tpenv overallTy safeThisValOpt
                     errorR(Error(FSComp.SR.tcAttributesAreNotPermittedOnLetBindings(), attr.Range))
             attrs
 
-        // Rotate [<return:...>] from binding to return value
-        // Also patch the syntactic representation
-        let retAttribs, valAttribs, valSynData =
-            let attribs = TcAttrs attrTgt false attrs
-            let rotRetSynAttrs, rotRetAttribs, valAttribs =
-                // Do not rotate if some attrs fail to typecheck...
-                if attribs.Length <> attrs.Length then [], [], attribs
-                else attribs
-                     |> List.zip attrs
-                     |> List.partition(function | _, Attrib(_, _, _, _, _, Some ts, _) -> ts &&& AttributeTargets.ReturnValue <> enum 0 | _ -> false)
-                     |> fun (r, v) -> (List.map fst r, List.map snd r, List.map snd v)
-            let retAttribs =
-                match rtyOpt with
-                | Some (SynBindingReturnInfo(attributes = Attributes retAttrs)) ->
-                    rotRetAttribs @ TcAttrs AttributeTargets.ReturnValue true retAttrs
-                | None -> rotRetAttribs
-            let valSynData =
-                match rotRetSynAttrs with
-                | [] -> valSynData
-                | {Range=mHead} :: _ ->
-                let (SynValData(valMf, SynValInfo(args, SynArgInfo(attrs, opt, retId)), valId)) = valSynData
-                SynValData(valMf, SynValInfo(args, SynArgInfo({Attributes=rotRetSynAttrs; Range=mHead} :: attrs, opt, retId)), valId)
-            retAttribs, valAttribs, valSynData
+        // [<return: X>] attributes are moved out of the binding's prefix and into
+        // SynValData.SynValInfo.retInfo by SynInfo.RotateReturnAttributes in mkSynBinding,
+        // alongside any attributes on the return type annotation populated by InferSynReturnData.
+        // Use that as the single source of truth.
+        let valAttribs = TcAttrs attrTgt false attrs
+
+        let retAttribs =
+            let (SynValData(_, SynValInfo(_, SynArgInfo(retAttrs, _, _)), _)) = valSynData
+            retAttrs
+            |> List.collect (fun a -> a.Attributes)
+            |> TcAttrs AttributeTargets.ReturnValue true
 
         let valAttribFlags = computeValWellKnownFlags g valAttribs
 

src/Compiler/Checking/NameResolution.fs

@@ -94,11 +94,16 @@ let ActivePatternElemsOfValRef g (vref: ValRef) =
                 ActivePatternReturnKind.RefTypeWrapper
             else
                 let _, apReturnTy = stripFunTy g vref.TauType
-                let hasStructAttribute() = 
-                    vref.Attribs
-                    |> List.exists (function 
-                        | Attrib(targetsOpt = Some(System.AttributeTargets.ReturnValue)) as a -> hasFlag (classifyValAttrib g a) WellKnownValAttributes.StructAttribute
-                        | _ -> false)
+                let hasStructAttribute() =
+                    // After SynInfo.RotateReturnAttributes, [<return: Struct>] lives in
+                    // ValReprInfo's result ArgReprInfo rather than vref.Attribs. The flag bits
+                    // in ArgReprInfo.Attribs are computed lazily, so classify the underlying
+                    // attribute list directly.
+                    match vref.ValReprInfo with
+                    | Some(ValReprInfo(_, _, retInfo)) ->
+                        let flags = computeValWellKnownFlags g (retInfo.Attribs.AsList())
+                        hasFlag flags WellKnownValAttributes.StructAttribute
+                    | None -> false
                 if isValueOptionTy g apReturnTy || hasStructAttribute() then ActivePatternReturnKind.StructTypeWrapper
                 elif isBoolTy g apReturnTy then ActivePatternReturnKind.Boolean
                 else ActivePatternReturnKind.RefTypeWrapper

src/Compiler/SyntaxTree/SyntaxTreeOps.fs

@@ -740,6 +740,48 @@ module SynInfo =
             let argInfos = infosForObjArgs @ infosForArgs
             SynValData(Some memFlags, SynValInfo(argInfos, retInfo), None)
 
+    let private isReturnTargetedAttribute (a: SynAttribute) =
+        match a.Target with
+        | Some id -> id.idText = "return"
+        | None -> false
+
+    /// Rotate any `[<return: X>]` attributes from a binding's prefix attribute list into the
+    /// arity-info return position (`SynValInfo.retInfo`). Without this, downstream code that
+    /// reads `Val.Attribs` would incorrectly see them alongside method-targeted attributes
+    /// (see issues #17904 and #19020).
+    let RotateReturnAttributes (attrs: SynAttributes) (valSynData: SynValData) : SynAttributes * SynValData =
+        // Fast path: avoid all allocation when there's nothing to rotate (the common case).
+        let hasReturn =
+            attrs
+            |> List.exists (fun lst -> lst.Attributes |> List.exists isReturnTargetedAttribute)
+
+        if not hasReturn then
+            attrs, valSynData
+        else
+            let mutable returnTargeted = []
+
+            let newAttrs =
+                attrs
+                |> List.choose (fun lst ->
+                    let ret, kept = lst.Attributes |> List.partition isReturnTargetedAttribute
+                    returnTargeted <- returnTargeted @ ret
+
+                    if List.isEmpty kept then
+                        None
+                    else
+                        Some { lst with Attributes = kept })
+
+            let (SynValData(memFlags, SynValInfo(args, SynArgInfo(retAttrs, opt, retId)), thisIdOpt)) =
+                valSynData
+
+            let retList: SynAttributeList =
+                {
+                    Attributes = returnTargeted
+                    Range = (List.head returnTargeted).Range
+                }
+
+            newAttrs, SynValData(memFlags, SynValInfo(args, SynArgInfo(retList :: retAttrs, opt, retId)), thisIdOpt)
+
 let mkSynBindingRhs staticOptimizations rhsExpr mRhs retInfo =
     let rhsExpr =
         List.foldBack (fun (c, e1) e2 -> SynExpr.LibraryOnlyStaticOptimization(c, e1, e2, mRhs)) staticOptimizations rhsExpr
@@ -759,6 +801,8 @@ let mkSynBinding
     let info =
         SynInfo.InferSynValData(memberFlagsOpt, Some headPat, Option.map snd retInfo, origRhsExpr)
 
+    let attrs, info = SynInfo.RotateReturnAttributes attrs info
+
     let rhsExpr, retTyOpt = mkSynBindingRhs staticOptimizations origRhsExpr mRhs retInfo
     let mBind = unionRangeWithXmlDoc xmlDoc mBind
     SynBinding(vis, SynBindingKind.Normal, isInline, isMutable, attrs, xmlDoc, info, headPat, retTyOpt, rhsExpr, mBind, spBind, trivia)

tests/FSharp.Compiler.ComponentTests/Language/AttributeCheckingTests.fs

@@ -43,7 +43,114 @@ type C() =
          |> ignoreWarnings
          |> compile
          |> shouldSucceed
-
+
+    // Regression tests for #17904 and #19020. Both have a common root cause:
+    // [<return: X>] prefix attributes must route to the return-value metadata slot,
+    // not stay on the binding alongside method-targeted attributes.
+
+    [<Fact>]
+    let ``Issue 19020 - [<return: X>] is emitted on the return parameter of class members`` () =
+        FSharp """
+module M
+open System
+[<AttributeUsage(AttributeTargets.ReturnValue)>]
+type DescAttribute() = inherit Attribute()
+
+type T() =
+    [<return: Desc>]
+    member _.Inst a = a + 1
+    [<return: Desc>]
+    static member Stat a = a + 1
+
+[<EntryPoint>]
+let main _ =
+    let inst = typeof<T>.GetMethod("Inst").ReturnParameter.GetCustomAttributes(typeof<DescAttribute>, false).Length
+    let stat = typeof<T>.GetMethod("Stat").ReturnParameter.GetCustomAttributes(typeof<DescAttribute>, false).Length
+    if inst <> 1 then failwithf "instance member: expected 1, got %d" inst
+    if stat <> 1 then failwithf "static member: expected 1, got %d" stat
+    0
+        """
+        |> asExe
+        |> compile
+        |> shouldSucceed
+        |> run
+        |> shouldSucceed
+
+    [<Fact>]
+    let ``Issue 17904 - [<X>] on method and [<return: X>] on return value are not duplicates`` () =
+        Fsx """
+open System
+[<AttributeUsage(AttributeTargets.All)>]
+type AttrAttribute() = inherit Attribute()
+
+type T() =
+    [<Attr>]
+    [<return: Attr>]
+    member _.Foo() = ()
+        """
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+
+    [<Fact>]
+    let ``Issue 17904 - two [<return: X>] on the same return value remain duplicates`` () =
+        Fsx """
+open System
+[<AttributeUsage(AttributeTargets.All)>]
+type AttrAttribute() = inherit Attribute()
+
+type T() =
+    [<return: Attr>]
+    [<return: Attr>]
+    member _.Foo() = ()
+        """
+         |> ignoreWarnings
+         |> compile
+         |> shouldFail
+         |> withSingleDiagnostic (Error 429, Line 8, Col 7, Line 8, Col 19, "The attribute type 'AttrAttribute' has 'AllowMultiple=false'. Multiple instances of this attribute cannot be attached to a single language element.")
+
+    [<Fact>]
+    let ``Issue 17904 - [<X>] and [<method: X>] on a method remain duplicates`` () =
+        Fsx """
+open System
+[<AttributeUsage(AttributeTargets.All)>]
+type AttrAttribute() = inherit Attribute()
+
+type T() =
+    [<Attr>]
+    [<method: Attr>]
+    member _.Foo() = ()
+        """
+         |> ignoreWarnings
+         |> compile
+         |> shouldFail
+         |> withSingleDiagnostic (Error 429, Line 8, Col 7, Line 8, Col 19, "The attribute type 'AttrAttribute' has 'AllowMultiple=false'. Multiple instances of this attribute cannot be attached to a single language element.")
+
+    [<Fact>]
+    let ``CompilationRepresentation(Instance) on a union-type member is not rotated to the return value`` () =
+        FSharp """
+module M
+type MyOption<'T> =
+    | MySome of 'T
+    | MyNone
+    [<CompilationRepresentation(CompilationRepresentationFlags.Instance)>]
+    member this.Value = match this with | MySome v -> v | MyNone -> failwith "MyNone"
+
+[<EntryPoint>]
+let main _ =
+    let m = typeof<MyOption<int>>.GetMethod("get_Value")
+    if isNull m then failwith "get_Value not found — CompilationRepresentation(Instance) was likely rotated away from the member"
+    if m.IsStatic then failwith "get_Value should be an instance method"
+    if (MySome 42).Value <> 42 then failwith "Value did not return the carried payload"
+    0
+        """
+        |> asExe
+        |> compile
+        |> shouldSucceed
+        |> run
+        |> shouldSucceed
+
+
     [<FSharp.Test.FactForNETCOREAPP>]
     let ``Regression: typechecker does not fail when attribute is on type variable (https://github.com/dotnet/fsharp/issues/13525)`` () =
         let csharpBaseClass = 

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

@@ -256,6 +256,43 @@ let x = 123
         |> Option.map (fun su -> su.Symbol :?> FSharpMemberOrFunctionOrValue)
         |> Option.iter (fun symbol -> symbol.Attributes.Count |> shouldEqual 1)
 
+    [<Fact>]
+    let ``FCS - [<X>] surfaces on the method's Attributes only`` () =
+        let source = """
+open System.ComponentModel
+type Calculator() =
+    [<Description "method">]
+    member _.Compu{caret}te () = 1
+"""
+        let mfv = (Checker.getSymbolUse source).Symbol :?> FSharpMemberOrFunctionOrValue
+        mfv.HasAttribute<System.ComponentModel.DescriptionAttribute>() |> shouldEqual true
+        mfv.ReturnParameter.HasAttribute<System.ComponentModel.DescriptionAttribute>() |> shouldEqual false
+
+    [<Fact>]
+    let ``FCS - [<return: X>] surfaces on ReturnParameter.Attributes only`` () =
+        let source = """
+open System.ComponentModel
+type Calculator() =
+    [<return: Description "return">]
+    member _.Compu{caret}te () = 1
+"""
+        let mfv = (Checker.getSymbolUse source).Symbol :?> FSharpMemberOrFunctionOrValue
+        mfv.HasAttribute<System.ComponentModel.DescriptionAttribute>() |> shouldEqual false
+        mfv.ReturnParameter.HasAttribute<System.ComponentModel.DescriptionAttribute>() |> shouldEqual true
+
+    [<Fact>]
+    let ``FCS - [<X>] and [<return: X>] surface independently on the same member`` () =
+        let source = """
+open System.ComponentModel
+type Calculator() =
+    [<Description "method">]
+    [<return: Description "return">]
+    member _.Compu{caret}te () = 1
+"""
+        let mfv = (Checker.getSymbolUse source).Symbol :?> FSharpMemberOrFunctionOrValue
+        mfv.HasAttribute<System.ComponentModel.DescriptionAttribute>() |> shouldEqual true
+        mfv.ReturnParameter.HasAttribute<System.ComponentModel.DescriptionAttribute>() |> shouldEqual true
+
 module Types =
     [<Fact>]
     let ``FSharpType.Print parent namespace qualifiers`` () =