Allow extension methods without type attribute.

[nojaf]

@edgarfgp and I are trying to address fsharp/fslang-suggestions#835.

It currently works when the extension method is being consumed in the same F# project.
It doesn't appear to be exposed when consumed in a C# project.
I'm probably still missing something and I could use a pointer.

[vzarytovskii]

As discussed on Slack:
C# compiler expects type to have ExtensionAttribute to "see" its methods and treat them as extensions.

Probably a different approach will work better here - if type has static method(s) with ExtensionAttribute, then add this attribute to the type itself if it is missing it.

[abelbraaksma]

As @vzarytovskii mentions, and same is true for assembly, and possibly module (the latter I'm not sure of, it wasn't clear from the suggestions thread, I don't know if C# also adds it to .NET modules). Also, assuming there's an upcoming RFC, can we specify there what kind of members will be "detected" and what rules specifically apply?

[vzarytovskii]

can we specify there what kind of members will be "detected" and what rules specifically apply?

Regarding of detecting those, it is already in place, and probably should be changed to not check enclosing type, may be used as reference:

fsharp/src/Compiler/Checking/NameResolution.fs

Lines 509 to 515 in e06e079

	let IsMethInfoPlainCSharpStyleExtensionMember g m isEnclExtTy (minfo: MethInfo) =
	// Method must be static, have 'Extension' attribute, must not be curried, must have at least one argument
	isEnclExtTy &&
	not minfo.IsInstance &&
	not minfo.IsExtensionMember &&
	(match minfo.NumArgs with [x] when x >= 1 -> true | _ -> false) &&
	MethInfoHasAttribute g m g.attrib_ExtensionAttribute minfo

[edgarfgp]

Humm. There seems to be some error regarding vsintegration that have not touched . Maybe we could rerun the CI to see if that helps

[nojaf]

@edgarfgp I should open the VisualFSharp.sln when I find some time.
Another that struct me is that we didn't consider the impact of signature files in this PR.

What happens when you have:

module Foo

[<System.Runtime.CompilerServices.Extension>]
val PlusOne: a: int -> int

and

module Foo

[<System.Runtime.CompilerServices.Extension>]
let PlusOne (a:int) = a + 1

will that still be picked up?

[edgarfgp]

@edgarfgp I should open the VisualFSharp.sln when I find some time. Another that struct me is that we didn't consider the impact of signature files in this PR.

What happens when you have:

module Foo

[<System.Runtime.CompilerServices.Extension>]
val PlusOne: a: int -> int

and

module Foo

[<System.Runtime.CompilerServices.Extension>]
let PlusOne (a:int) = a + 1

will that still be picked up?

I think for the sake of completeness. We should look at it and see how doable is :)

[edgarfgp]

@vzarytovskii Should we put this under a preview flag ?

[smoothdeveloper]

@edgarfgp can there be a test that it works with VB.NET too?

[edgarfgp]

@smoothdeveloper As far I can see there is not way to do that in the compiler. If you know how please let me know :)

Update : We have tested with a local compiler version with a VB project and works Ok

[smoothdeveloper]

@Edgarfp this test checks things among F#, C# & VB.NET:

fsharp/tests/fsharp/tests.fs

Lines 2069 to 2074 in eaa723d

	let ``property setter in method or constructor`` () =
	let cfg = testConfig "core/members/set-only-property"
	csc cfg @"%s /target:library /out:cs.dll" cfg.csc_flags ["cs.cs"]
	vbc cfg @"%s /target:library /out:vb.dll" cfg.vbc_flags ["vb.vb"]
	fsc cfg @"%s /target:library /out:fs.dll" cfg.fsc_flags ["fs.fs"]
	singleNegTest cfg "calls"

Maybe there is a way which is similar with string literal with code in it in the unit tests, albeit I don't favour having those written like this, and generally prefer to deal with standalone code files.

Update : We have tested with a local compiler version with a VB project and works Ok

Can you confirm if this works without adding the Extension attribute to the assembly?

[vzarytovskii]

@vzarytovskii Should we put this under a preview flag ?

I would say so, yes. Better guard it by the preview version.

[nojaf]

Can you confirm if this works without adding the Extension attribute to the assembly?

This is outside the scope of our proposal. The fact that VB still needs the assembly attribute is no change in the current behaviour.

[T-Gro]

Passing now, please approve :)

[edgarfgp]

@T-Gro You are an absolute legend. Thanks for the help.

[dsyme]

This seems to duplicate the code up above - could we have factored this out to prevent the duplication?

[dsyme]

I'm concerned about the performance implications of this preview feature. If my late-night analysis is correct it seems to me we're now looking into the methods of every type in every opened namespace. This must be traversing a lot more metadata than previously. This will be very significant both from a perf question - think what happens if there are types in referenced assemblies that were previously unused that contain 100,000 methods. It also has some correctness implications see #15158.

Given the feature being implemented this is a little tricky to avoid. We really have to be looking for the ExtensionAttribute that mark the types before we traverse them, and avoiding the traversal for types that do not contain extension methods. That's really what the attributes are for.

Now, these attributes will be correctly present in compiled reference assemblies because they are either explicit or added in CheckDeclarations, so for referenced assemblies we can just look at them. However these attributes are not necessarily present yet in the assembly being compiled - e.g. in the case of namespace rec where one part of the recursive set of types opens another before the attributes have been fully stitched in.

My (late night dont-take-as-gospel) recommendation is that this code be adjusted to look for ExtensionAttribute, except in the assembly being compiled. So something like:

    if g.langVersion.SupportsFeature(LanguageFeature.CSharpExtensionAttributeNotRequired) then
        let ty = generalizedTyconRef g tcrefOfStaticClass
        let csharpStyleExtensionMembers = 
            if IsTyconRefUsedForCSharpStyleExtensionMembers g m tcrefOfStaticClass || tcrefOfStaticClass.IsLocalRef then
                GetImmediateIntrinsicMethInfosOfType (None, AccessorDomain.AccessibleFromSomeFSharpCode) g amap m ty
                |> List.filter (IsMethInfoPlainCSharpStyleExtensionMember g m true)
            else
                []
                
        if not minfos.IsEmpty then
                let pri = NextExtensionMethodPriority()