Enforce AttributeTargets.Interface

[edgarfgp]

Description

Enforce AttributeTargets.Interface

• AttributeTargets.Interface should only work on interfaces
• AttributeTargets.Class should only work on classes

open System

[<AttributeUsage(AttributeTargets.Interface)>]
type CustomInterfaceAttribute() =
    inherit Attribute()

[<AttributeUsage(AttributeTargets.Class)>]
type CustomClassAttribute() =
    inherit Attribute()

[<CustomClass>] // Should fail, but does not
type IFoo =
    abstract member Foo : string

[<CustomClass>] // Should fail, but does not
[<Interface>]
type IFoo2 =
    abstract member Foo : string
    
[<CustomInterface>] // Should fail, but does not
type Foo() = class end

[<Class; CustomInterface>] // Should fail, but does not
type Foo2() = class end

See C# version for reference sharplab

Checklist

• Test cases added
• Release notes entry updated

[github-actions]

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

[edgarfgp]

AllowNullLiteralAttribute needs updating

[edgarfgp]

Interesting. VS Integration is using System.Runtime.InteropServices.ClassInterface attribute docs which is meant to be used only on a class

  /// <summary>Indicates the type of class interface to be generated for a class exposed to COM, if an interface is generated at all.</summary>
  [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class, Inherited = false)]
  public sealed class ClassInterfaceAttribute : Attribute
  {
    /// <summary>Initializes a new instance of the <see cref="T:System.Runtime.InteropServices.ClassInterfaceAttribute" /> class with the specified <see cref="T:System.Runtime.InteropServices.ClassInterfaceType" /> enumeration member.</summary>
    /// <param name="classInterfaceType">One of the <see cref="T:System.Runtime.InteropServices.ClassInterfaceType" /> values that describes the type of interface that is generated for a class.</param>
    public ClassInterfaceAttribute(ClassInterfaceType classInterfaceType)
    {
      this.Value = classInterfaceType;
    }

sharplab

[<System.Runtime.InteropServices.ClassInterface(ClassInterfaceType.None)>] // This is allowed but should not
type public IVsMicrosoftInstalledProduct =
    inherit IVsInstalledProduct
    abstract IdBmpSplashM : byref<uint32> -> unit

[<System.Runtime.InteropServices.ClassInterface(ClassInterfaceType.None)>]
type Foo() = class end

sharplab

using System.Runtime.InteropServices;

[ClassInterface(ClassInterfaceType.AutoDispatch)]
public class MyClass
{
}

[ClassInterface(ClassInterfaceType.AutoDispatch)] // This raises an error in C#
public interface IFoo {}

cc @vzarytovskii seems like System.Runtime.InteropServices.ClassInterface has been wrongly used here ?

[vzarytovskii]

Not sure what to do with it, it'll probably need to be an abstract class. This is legacy project system, so won't be able to test it that easy, and we have to support it.

[edgarfgp]

Not sure what to do with it, it'll probably need to be an abstract class. This is legacy project system, so won't be able to test it that easy, and we have to support it.

Yeah an AbstractClass will be ok here but it won't be able to inherit from an interface IVsInstalledProduct(seems to be a close source so I can't see the defined members)

[majocha]

@edgarfgp it's just this https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.shell.interop.ivsinstalledproduct?view=visualstudiosdk-2022

[edgarfgp]

Ohh Thanks my googling failed 😅

[majocha]

Yeah, we don't actually implement it, so it's not really used as the docs prescribe. This is some ancient COM trickery that worked long time ago but not necessarily still do.

In fact I just comment out that whole IVsMicrosoftInstalledProduct interface, started VS debug instance and things seem to work the same without it:

[edgarfgp]

Yeah, we don't actually implement it, so it's not really used as the docs prescribe. This is some ancient COM trickery that worked long time ago but not necessarily still do.

In fact I just comment out that whole IVsMicrosoftInstalledProduct interface, started VS debug instance and things seem to work the same without it

If it is not longer used/needed we could just comment out / remove this code.

ideally enforcing the right attribute targets in the compiler should not depend on the use of this interface

[abelbraaksma]

I'd really like this change and for the compiler to enforce attribute targets, but technically this would be a backward incompatible change, right? Do we have a lang suggestion or bug or issue report (I believe this has come up several times)?

Ah, wait, found it: #8547.

And related are all these: #16692, #16764, #16845, #16790. I assume my point above has already been discussed before then ;).

And there's mention of an RFC by you (@edgarfgp), can we link that from the original post above, or is that TODO? #8547 (comment)

[edgarfgp]

I'd really like this change and for the compiler to enforce attribute targets, but technically this would be a backward incompatible change, right? Do we have a lang suggestion or bug or issue report (I believe this has come up several times)?

Ah, wait, found it: #8547.

And related are all these: #16692, #16764, #16845, #16790. I assume my point above has already been discussed before then ;).

And there's mention of an RFC by you (@edgarfgp), can we link that from the original post above, or is that TODO? #8547 (comment)

@abelbraaksma Once I have addressed all the AttributeTargets inconsistencies I would be willing to write an RFC to properly spec this. This is behind a LangVersion preview.

[psfinaki]

Thanks for your continuous work here, Edgar :)

Yeah so the VS change looks a bit risky, but since Jakub tested the related functionality, I don't think we should block the fix by that thing. Also, this is a well-isolated PR, so can be reverted quickly if needed.