warning for mismatched param names #3527

dsyme · 2017-08-30T23:50:22Z

This is the proposed fix for #3467

Basically we will now show a warning when signature files and implementation files have mismatched argument names

This is WIP because wee must fix up the (many) occurrences of this problem in FSharp.Core and the compiler itself.

We also have to decide if the warning is on or off by default. Given the severity of the problem for the debugging experience (See linked bug) I'm sure it should be on by default - even if that means upgrading F# version requires adding a --nowarn or changing code

KevinRansom · 2017-08-31T08:03:03Z

We can put it in new project templates and enable it by default in the coreclr projects.
It's too breaking to turn it on in the compiler by default.

KevinRansom

Needs some tests case too.

KevinRansom · 2017-08-31T08:08:42Z

src/fsharp/FSharp.Core/FSharp.Core.fsproj

I like maxerrors ... keeps the insanity down.

KevinRansom · 2017-08-31T08:13:06Z

src/fsharp/SignatureConformance.fs

should the comparison be locale aware, or be StringCompare.Ordinal?

Everywhere else in signature conformance checking we just use default F# string equality i.e. System.String.Equals(string1,string2), e.g.

if implTycon.LogicalName <> sigTycon.LogicalName then (errorR (Error (FSComp.SR.DefinitionsInSigAndImplNotCompatibleNamesDiffer(implTycon.TypeOrMeasureKind.ToString(),sigTycon.LogicalName,implTycon.LogicalName),m)); false) else if implTycon.CompiledName <> sigTycon.CompiledName then (errorR (Error (FSComp.SR.DefinitionsInSigAndImplNotCompatibleNamesDiffer(implTycon.TypeOrMeasureKind.ToString(),sigTycon.CompiledName,implTycon.CompiledName),m)); false) else

If we change it in one place we'd need to change it everywhere

Yes ... but what is the right thing to do?

It may be that locale aware case sensitive is the right thing to do. It's not clear to me is all.

I think we can't change this at this stage - all the name lookups in the compiler are based on exact string equality.

Also, part of the intent of using the signature names is to make sure that the .NET/IL/C#/debug and F# views of named arguments are the same. We don't know what equality check is used by visual studio when you mouse-hover over an identifier and it tries to look it up.

In any case, it's an orthogonal issue to this PR

saul · 2017-08-31T09:06:13Z

@dsyme isn't the better fix to use the implementation parameter names in IL instead of the signature parameter names? Then we don't even need the warning - it will just work by default.

dsyme · 2017-08-31T09:32:55Z

@dsyme is the fix here to use the implementation parameter names in IL instead of the signature parameter names? Then we don't even need the warning - it will just work by default.

That would be a breaking change since the C#/.NET view of consuming the assembly would change.

Also the C#/.NET view would be logically different to the F# view, e.g. for named arguments, which greatly disturbs me.

dsyme · 2017-08-31T09:34:18Z

We can put it in new project templates and enable it by default in the coreclr projects. It's too breaking to turn it on in the compiler by default.

I'm ok with that approach.

saul · 2017-08-31T10:44:58Z

How does C# get around this for interface methods that have different parameter names than the parameter names on the same method in an implementing class?

dsyme · 2017-08-31T15:44:05Z

I've

made the warning off by default
added tests
added a Visual Studio CodeFix for this, it works very nicely, see Align signature/implementation argument names in FSharp.Core #3531 for cases where I have been using it heavily

Still need to adjust the templates

dsyme · 2017-08-31T15:45:43Z

@saul For C#

no effect on debugging since they use the names of the implementation.

signature and implementation may not match, without warning, e.g. you get

A.M x = XX, y = YY
B.M x = XX, y = YY
B.M x = YY, y = XX

from

using System;

namespace ConsoleApp1
{
    class A
    {
        public virtual void M(string x, string y) { System.Console.WriteLine("A.M x = {0}, y = {1}", x, y);  }
    }
    class B : A
    {
        public override void M(string y, string x) { System.Console.WriteLine("B.M x = {0}, y = {1}", x, y); }
    }
    class Program
    {
        static void Main(string[] args)
        {
            (new A()).M(x: "XX", y: "YY");
            (new B()).M(x: "XX", y: "YY");
            ((A)(new B())).M(x: "XX", y: "YY");

            System.Console.ReadLine();
        }
    }
}

FWIW I think the optional feature is justified just on the basis of code hygiene and correctness alone

dsyme · 2017-08-31T15:46:15Z

@saul I can definitely see the case for also implementing an optional warning on overrides

saul · 2017-08-31T15:55:58Z

I think I'm being blind but I can't fix the codefix in this PR?

dsyme · 2017-08-31T16:01:49Z

@saul Yes, missed a file - now added

saul · 2017-08-31T16:08:39Z

vsintegration/src/FSharp.Editor/CodeFix/RenameParamToMatchSignature.fs

Can we not use this logic? I saw this used somewhere else and this isn't correct - there's many cases when you want to use ``

@saul Yes, I agree. I will find the right compiler helpe

saul · 2017-08-31T16:08:51Z

vsintegration/src/FSharp.Editor/CodeFix/RenameParamToMatchSignature.fs

What if my variable name ends in '?

Maybe something like: [^']+'(.+)'[^']+'(.+)'[^']+

Edit: actually it looks like .+'(.+)'.+'(.+)'.+ also works.

@saul Thanks

Re variable name ending in ' - good point. Not sure what to do about this

saul · 2017-08-31T16:09:52Z

Cheers Don. Looks like we're also missing the FSharp.Editor.fsproj change, too?

dsyme · 2017-08-31T16:14:06Z

This is ready once CI is green, subject to review

@KevinRansom I presume the F# templates for .NET SDK are in the .NET SDK repo?

dsyme · 2017-08-31T16:22:04Z

Cheers Don. Looks like we're also missing the FSharp.Editor.fsproj change, too?

@saul See https://github.com/Microsoft/visualfsharp/pull/3527/files#diff-2c15e8a33254afafc0e96c4f069e0aadR96

dsyme · 2017-08-31T16:32:19Z

@saul Thanks for the feedback so far. If you could give it a go that would be great

saul

At work at the minute so this is just by eye! Looks great though - glad we’ve fixed one of the debugging oddities.

KevinRansom · 2017-08-31T20:43:46Z

@dsyme for coreclr put it in here: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.props
@dsyme lose the template changes, and create an issue to add it and assign it to me. I will add it in.

dsyme · 2017-08-31T22:31:17Z

@dotnet-bot test Windows_NT Release_ci_part1 Build please

saul · 2017-09-01T08:48:01Z

vsintegration/ProjectTemplates/ConsoleProject/Template/ConsoleApplication.fsproj

Is the comment really necessary? Also I think this should be: <OtherFlags>$(OtherFlags) --warnon:3218</OtherFlags> . We don't want to override any flags that were previously set (e.g. if there is an <Import> at the top of the project). That's bitten me before.

Yes to both, but @KevinRansom asked me to drop these changes in any case, he'll do them separately

dsyme · 2017-09-01T12:53:39Z

@KevinRansom This is ready - when you merge can you create the tracking issue for templates? Thanks

…o params1

Issue opened: dotnet#3539

msftclas added the cla-not-required label Aug 30, 2017

KevinRansom reviewed Aug 31, 2017

View reviewed changes

dsyme mentioned this pull request Aug 31, 2017

Align signature/implementation argument names in FSharp.Core #3531

Merged

saul reviewed Aug 31, 2017

View reviewed changes

dsyme changed the title ~~[WIP] warning for mismatched param names~~ warning for mismatched param names Aug 31, 2017

saul approved these changes Aug 31, 2017

View reviewed changes

saul reviewed Sep 1, 2017

View reviewed changes

implement check and codefix for mismatched parameters

ff34908

dsyme force-pushed the params1 branch from 3542c93 to ff34908 Compare September 1, 2017 10:15

Merge branch 'master' of http://github.com/Microsoft/visualfsharp int…

c10a2ca

…o params1

KevinRansom mentioned this pull request Sep 1, 2017

Enable -- warning for mismatched param names by default in new desktop templates and the netsdk #3539

Closed

KevinRansom merged commit 0f259ed into dotnet:master Sep 1, 2017

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022

implement check and codefix for mismatched parameters (dotnet#3527)

ac1b856

Issue opened: dotnet#3539

warning for mismatched param names #3527

warning for mismatched param names #3527

Uh oh!

Conversation

dsyme commented Aug 30, 2017

Uh oh!

KevinRansom commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saul commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

saul commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

saul commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saul Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saul commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

saul left a comment

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Aug 31, 2017

Uh oh!

dsyme commented Aug 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme commented Sep 1, 2017

Uh oh!

Reviewers

Assignees

Labels

KevinRansom commented Aug 31, 2017 •

edited

Loading

saul commented Aug 31, 2017 •

edited

Loading

dsyme commented Aug 31, 2017 •

edited

Loading

saul Aug 31, 2017 •

edited

Loading