Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VS 2017 RC - From VSTS] C#-style extension method fails to compile #1802

Closed
cartermp opened this issue Nov 22, 2016 · 30 comments
Closed

[VS 2017 RC - From VSTS] C#-style extension method fails to compile #1802

cartermp opened this issue Nov 22, 2016 · 30 comments
Milestone

Comments

@cartermp
Copy link
Contributor

cartermp commented Nov 22, 2016

FROM VSTS:

Take an extension method written in F# for consumption by C#. Note that this is not using F#-style extension methods, it's using the same attributes that have been implicitly inserted by the C# compiler in previous versions when writing extension methods in C#.

open System.Runtime.CompilerServices

[<System.Runtime.CompilerServices.Extension>]
type CsStringEx private () =
    [<System.Runtime.CompilerServices.Extension>]
    static member EqualsIgnoreCase(this:string, other) =
        String.Equals(this, other, StringComparison.OrdinalIgnoreCase)

Use that extension method in some C# code.

string foo = "abc123";
bool isEqual = foo.EqualsIgnoreCase("ABC123");

That sort of code has compiled without error since at least VS 2012, and now without any changes, in VS 2017 RC it's getting this compiler error:

CS0411: The type arguments for method 'CsStringEx.EqualsIgnoreCase(string, t)' cannot be inferred from the usage. Try specifying the type arguments explicitly.

Note that the above code is a rough illustration, and doesn't actually reproduce the error. I'm attaching a stand-alone minimal repro case that does reproduce the compiler error.

@dsyme
Copy link
Contributor

dsyme commented Nov 22, 2016

@cartermp This seems like a C# compiler regression: I don't think F# is doing anything different here. Does adding the Extension attribute to the assembly help? My understanding is that the C# compiler has some kind of special case to allow this to work, where it doesn't actually require the Extension attribute on the assembly.

@cartermp
Copy link
Contributor Author

@jaredpar for FYI. I'm looking to repro on my machine, but Parallels seems to be stalled on an update and I don't have a Windows machine nearby.

@jaredpar
Copy link
Member

There needs to be an assembly level attribute as well. This is a known change that came in the Roslyn 1.0 timeframe.

@dsyme
Copy link
Contributor

dsyme commented Nov 22, 2016

@jaredpar I thought the decision was to fix that in C#? Anyway there's a relevant old issue here: http://visualfsharp.codeplex.com/workitem/76

@dsyme
Copy link
Contributor

dsyme commented Nov 22, 2016

Specifcally:

Motivation: When consuming extension methods from references assemblies, the old C# compiler had a bug where it didn't bother to check for the assembly-level ExtensionAttribute. That's why nobody remembers it! With the new Roslyn compilers, this is now enforced**. So it would be very useful for F# developers to be reminded when they are missing these.

But then this:

** Sort of - we convinced them to special-case F# to preserve back-compat. Still, F# devs should start doing the right thing.

@dsyme
Copy link
Contributor

dsyme commented Nov 22, 2016

@jaredpar Perhaps the "F#-specific hack" is no longer working?

@smoothdeveloper
Copy link
Contributor

just for notice, the assembly level attribute has always been required for the call to work in VB.NET.

I had this comment on another issue: #1254 (comment)

It would be nice to have [<Extension>] added automatically to a class/module if it contains a member with it defined. And it also should define it automatically on the assembly otherwise VB.NET won't find the extension methods.

@KevinRansom
Copy link
Member

@dsyme
Perhaps we can emit a warning about the missing Assembly Level Attribute?

@smoothdeveloper
Copy link
Contributor

Is emitting the warning any easier than actually putting the attribute implicitly? C# and VB.NET don't have to state the attribute explicitly as the compiler will add it to the class(es) and assembly whenever an extension method is found.

@forki
Copy link
Contributor

forki commented Nov 25, 2016

+1 for adding the attribute if that's possible.

@AlekseyTs
Copy link

@cartermp

I'm attaching a stand-alone minimal repro case that does reproduce the compiler error.

Could you please provide the actual repro steps?

@cartermp
Copy link
Contributor Author

@AlekseyTs The text of the bug report is from VSTS generated from here: https://developercommunity.visualstudio.com/content/problem/3246/c-extension-methods-written-in-f-can-no-longer-be.html

I can't seem to find the attached solution which repros this - I'll try something on my machine.

@jtmueller
Copy link

jtmueller commented Nov 29, 2016

@cartermp, @AlekseyTs I'm the one who wrote up the VSTS bug report being quoted, and I can say with certainty that adding the assembly-level ExtensionAttribute does not help. It seems to have more to do with resolving generic parameters than what attributes are present.

Here's the repro solution I had attached to the VSTS bug report. You can build that code in VS 2015, but not in VS 2017 RC, and the assembly-level Extension attribute is present.

ExtensionMethods_Repro.zip

@forki
Copy link
Contributor

forki commented Nov 29, 2016

@jaredpar maybe C# team should take a look as well.

@jaredpar
Copy link
Member

@forki @AlekseyTs is on the C# team.

@forki
Copy link
Contributor

forki commented Nov 29, 2016

Good. Thx

@AlekseyTs
Copy link

It looks like the C# compiler error is caused by a behavior change in F# compiler. Here is how the extension methods look like when compiled in VS2015:

.method public static bool  EqualsIgnoreCase(string this,
                                             string other) cil managed

.method public static bool  EqualsIgnoreCase(string this,
                                             string other,
                                             bool ignoreCulture) cil managed

And this is how they look when compiled by VS2017:

.method public static !!b  EqualsIgnoreCase<a,b>(string this,
                                                 !!a other) cil managed
{
  .custom instance void [mscorlib]System.Runtime.CompilerServices.ExtensionAttribute::.ctor() = ( 01 00 00 00 ) 
  // Code size       9 (0x9)
  .maxstack  8
  IL_0000:  nop
  IL_0001:  ldarg.0
  IL_0002:  ldarg.1
  IL_0003:  starg.s    other
  IL_0005:  starg.s    this
  IL_0007:  br.s       IL_0000
} // end of method CsStringEx::EqualsIgnoreCase

.method public static !!e  EqualsIgnoreCase<c,d,e>(string this,
                                                   !!c other,
                                                   !!d ignoreCulture) cil managed
{
  .custom instance void [mscorlib]System.Runtime.CompilerServices.ExtensionAttribute::.ctor() = ( 01 00 00 00 ) 
  // Code size       12 (0xc)
  .maxstack  8
  IL_0000:  nop
  IL_0001:  ldarg.0
  IL_0002:  ldarg.1
  IL_0003:  ldarg.2
  IL_0004:  starg.s    ignoreCulture
  IL_0006:  starg.s    other
  IL_0008:  starg.s    this
  IL_000a:  br.s       IL_0000
} // end of method CsStringEx::EqualsIgnoreCase

C# compiler is unable to infer the last type parameter because it is not used in any of the parameters. BTW, the IL of the generic methods looks suspicious to me as well.

@jaredpar
Copy link
Member

@dsyme, @cartermp based on @AlekseyTs analysis this seems like a regression in the F# compiler. Are you all seeing the same output?

@cartermp
Copy link
Contributor Author

Yep, getting the same output on my machine as well. Seems like a regression to me. @KevinRansom for FYI.

@dsyme
Copy link
Contributor

dsyme commented Dec 27, 2016

@cartermp @jaredpar Yes, this is a known-change-of-behavior.

Whether we should keep the new behavior ("Extensions are in scope within their own definition") or try to revert to the old behavior needs to be decided.

Analysis is the same as this: #1296 (comment)

@cartermp
Copy link
Contributor Author

cartermp commented Jan 3, 2017

@dsyme I'm of the opinion that we ought to revert to the old behavior. We don't have any numbers on usage, but this does seem like a tricky breaking change.

I'll prepare a fix

Is there a fix on hand? We're fast-approaching code freeze, so if we're not able to get something in then we'll need to message the fact that the F# 4.1 compiler has this breaking change.

@dsyme
Copy link
Contributor

dsyme commented Jan 3, 2017

@cartermp I will search for a fix if I can find time (in theory am still on vacation). However will be very subtle, so we should be prepared for this to be a breaking change (justified as a bug fix). To my knowledge it would be the first time we've taken such a fix intentionally.

@cartermp
Copy link
Contributor Author

cartermp commented Jan 3, 2017

Sounds good.

@cartermp cartermp added this to the VS 2017 RTM milestone Jan 6, 2017
@cartermp
Copy link
Contributor Author

cartermp commented Jan 6, 2017

Tagging as RTM - this will be resolved either via fix or notifying that F# 4.1 includes this breaking change.

@dsyme
Copy link
Contributor

dsyme commented Feb 2, 2017

@cartermp This can now be resolved as by-design

@cartermp
Copy link
Contributor Author

cartermp commented Feb 2, 2017

Okay. We'll take note of this in the blog post when we announce F# 4.1

@cartermp cartermp closed this as completed Feb 2, 2017
@smoothdeveloper
Copy link
Contributor

@cartermp, I think this is a something for C# to state as F# didn't change behaviour while C# did, VB.NET has same limitation since day 1 and requires explicit attribute on the assembly as well.

What we should announce in future is that the attribute is automatically added by compiler like VB & C# are doing 😃

@AlekseyTs
Copy link

as F# didn't change behaviour while C# did

@smoothdeveloper This issue is about a change in F# behavior as explained here #1802 (comment).

@smoothdeveloper
Copy link
Contributor

smoothdeveloper commented Feb 2, 2017

@AlekseyTs I see, thanks I thought the notice of change was about the code in the issue itself. I actually don't understand how the code became generic in new F# as only this overload could match: https://msdn.microsoft.com/en-us/library/t4411bks(v=vs.110).aspx

@jkone27
Copy link
Contributor

jkone27 commented May 28, 2019

for now i see a "workaround" is to use (in C#)
using static namespace.module.extensiontype;

this makes the extension method usable in C# side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants