ERR_ImplBadTupleNames is now being reported in correct class by t-camaia · Pull Request #24438 · dotnet/roslyn

t-camaia · 2018-01-25T01:02:20Z

An if statement was added to check whether we should report ERR_ImplBadTupleNames in the method or in the class declaration. Some tests were also updated.

Fixes #20897.

jaredpar · 2018-01-25T02:14:35Z

@t-camaia can you retarget this PR to dev15.7.x?

AlekseyTs · 2018-01-25T18:52:14Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

+                }
+                else
+                {
+                    diagnostics.Add(ErrorCode.ERR_ImplBadTupleNames, implementingType.Locations[0], implicitImpl, interfaceMember);


implementingType.Locations[0] [](start = 69, length = 29)

I think the location should be the interface reference in the base list, we probably already have a helper that finds it. #Closed

Take a look at the logic around error location in ReportImplicitImplementationMismatchDiagnostics method in this file. See if we can share it with this method. #Closed

AlekseyTs · 2018-01-25T18:53:56Z

    private static void ReportImplicitImplementationMatchDiagnostics(Symbol interfaceMember, TypeSymbol implementingType, Symbol implicitImpl, DiagnosticBag diagnostics)

It looks like other errors reported in this function have the same issue, I think we should fix all of them #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs:1041 in ba19c27. [](commit_id = ba19c27, deletion_comment = False)

AlekseyTs · 2018-01-25T18:56:01Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

            {
                // it is ok to implement implicitly with no tuple names, for compatibility with C# 6, but otherwise names should match
-                diagnostics.Add(ErrorCode.ERR_ImplBadTupleNames, implicitImpl.Locations[0], implicitImpl, interfaceMember);
+                if (implicitImpl.ContainingType == implementingType)


if (implicitImpl.ContainingType == implementingType) [](start = 16, length = 52)

Rather than adding a branching that pretty much duplicates the code with some minor variation, consider adding a local function that will return the right location based on conditional logic inside it. #Closed

AlekseyTs · 2018-01-25T18:57:01Z

Done with review pass (iteration 1) #Closed

…erface reference

t-camaia · 2018-01-26T19:44:19Z

@AlekseyTs I believe the other errors could only happen with explicit implementations, which means they are always being reported in the expected location. Do you have any particular scenario in mind?

AlekseyTs · 2018-01-26T20:55:44Z

I believe the other errors could only happen with explicit implementations, which means they are always being reported in the expected location.

The function is named ReportImplicitImplementationMatchDiagnostics I would be surprised if it was reporting errors for an explicit implementation #Closed

AlekseyTs · 2018-01-26T21:02:49Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

+                    {
+                        var @interface = interfaceMember.ContainingType;
+                        SourceMemberContainerTypeSymbol snt = implementingType as SourceMemberContainerTypeSymbol;
+                        return snt.GetImplementsLocation(@interface) ?? implementingType.Locations[0];


snt. [](start = 31, length = 4)

It feels like this access should be conditional because snt can be null. We should fix the other call site as well #Closed

This could be rewritten as return (implementingType as SourceMemberContainerTypeSymbol)?.GetImplementsLocation(@interface) ?? implementingType.Locations[0]

AlekseyTs · 2018-01-26T21:03:38Z

Done with review pass (iteration 2) #Closed

…face reference. Tests updated.

jaredpar · 2018-01-29T19:30:37Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

                }
            }
+
+            Location GetDiagnosticLocation(Symbol member)


Please mark as private

It's only a local function. I believe it's not possible to mark it with access modifiers.

I'll put it at the beginning of the method to avoid confusion.

AlekseyTs · 2018-01-29T23:05:09Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

                }
            }
+
+            Location GetDiagnosticLocation(Symbol member)


Symbol member [](start = 43, length = 13)

It feels strange that we are passing a member, but not passing implementingType and interfaceMember . Consider either passing all, or neither. #Closed

We are only passing a member because WRN_MultipleRuntimeImplementationMatches may be reported in a location different from implicitImpl.

AlekseyTs · 2018-01-29T23:06:24Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

                }
            }
+
+            Location GetDiagnosticLocation(Symbol member)


GetDiagnosticLocation [](start = 21, length = 21)

I think our naming convention for local function requires them to start with small letter, i.e. "getDiagnosticLocation". #Closed

AlekseyTs · 2018-01-29T23:19:52Z

                    ReportAnyMismatchedConstraints(interfaceMethod, implementingType, implicitImplMethod, diagnostics);

It looks like we might want to adjust location for errors reported by this function as well. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs:1068 in 179ee37. [](commit_id = 179ee37, deletion_comment = False)

AlekseyTs · 2018-01-29T23:21:29Z

Done with review pass (iteration 3) #Closed

…od report location updated. Tests updated.

AlekseyTs · 2018-01-30T06:24:04Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

                    else
                    {
-                        ReportAnyMismatchedConstraints(interfaceMethod, implementingType, implicitImplMethod, diagnostics);
+                        ReportAnyMismatchedConstraints(interfaceMethod, implementingType, implicitImplMethod, getDiagnosticLocation(implicitImpl), diagnostics);


getDiagnosticLocation(implicitImpl) [](start = 110, length = 35)

We should avoid eager calculation of location before we determined that there is any diagnostics to report. We probably should make getDiagnosticLocation a real (non-local) static function and call it inside ReportAnyMismatchedConstraints when location is needed. #Closed

AlekseyTs · 2018-01-30T06:29:34Z

Done with review pass (iteration 4) #Closed

mavasani · 2018-01-30T19:08:16Z

@dotnet-bot retest this please

AlekseyTs · 2018-01-30T20:15:26Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

            }
        }

+        private static Location GetDiagnosticLocation(Symbol interfaceMember, TypeSymbol implementingType, Symbol member)


GetDiagnosticLocation [](start = 32, length = 21)

GetImplicitImplementationDiagnosticLocation?

AlekseyTs

LGTM (iteration 5)

t-camaia requested a review from a team as a code owner January 25, 2018 01:02

jaredpar approved these changes Jan 25, 2018

View reviewed changes

t-camaia changed the base branch from features/compiler to dev15.7.x January 25, 2018 02:22

t-camaia changed the base branch from dev15.7.x to features/compiler January 25, 2018 02:25

t-camaia force-pushed the tuple_bug branch from 708fa00 to afccd28 Compare January 25, 2018 02:41

t-camaia requested review from a team as code owners January 25, 2018 02:41

t-camaia changed the base branch from features/compiler to dev15.7.x January 25, 2018 02:41

ERR_ImplBadTupleNames is now being reported in correct class

ba19c27

t-camaia force-pushed the tuple_bug branch from afccd28 to ba19c27 Compare January 25, 2018 18:18

AlekseyTs reviewed Jan 25, 2018

View reviewed changes

Local function added. ERR_ImplBadTupleNames being reported in the int…

660b6ce

…erface reference

AlekseyTs reviewed Jan 26, 2018

View reviewed changes

snt gets conditional access. Other errors being reported in the inter…

179ee37

…face reference. Tests updated.

jaredpar reviewed Jan 29, 2018

View reviewed changes

AlekseyTs reviewed Jan 29, 2018

View reviewed changes

Local function renamed and moved. ReportAnyMismatchedConstraints meth…

e80dc55

…od report location updated. Tests updated.

AlekseyTs reviewed Jan 30, 2018

View reviewed changes

GetDiagnosticLocation is not a local function anymore

9085534

cston approved these changes Jan 30, 2018

View reviewed changes

AlekseyTs reviewed Jan 30, 2018

View reviewed changes

AlekseyTs approved these changes Jan 30, 2018

View reviewed changes

GetDiagnosticLocation renamed

9f762a0

t-camaia merged commit ceb656c into dotnet:dev15.7.x Jan 30, 2018

t-camaia deleted the tuple_bug branch January 30, 2018 22:45

jcouv added the Area-Compilers label Jan 30, 2018

jcouv added this to the 15.7 milestone Jan 30, 2018

Conversation

t-camaia commented Jan 25, 2018

Uh oh!

jaredpar commented Jan 25, 2018

Uh oh!

AlekseyTs Jan 25, 2018 • 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!

AlekseyTs Jan 25, 2018 • 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!

AlekseyTs commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jan 25, 2018 • 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!

AlekseyTs commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-camaia commented Jan 26, 2018

Uh oh!

AlekseyTs commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jan 26, 2018 • 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!

agocke Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

t-camaia Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

t-camaia Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 29, 2018 • 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!

t-camaia Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 29, 2018 • 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!

AlekseyTs commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jan 30, 2018 • 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!

AlekseyTs commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mavasani commented Jan 30, 2018

Uh oh!

AlekseyTs Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

AlekseyTs Jan 25, 2018 •

edited

Loading

AlekseyTs Jan 25, 2018 •

edited

Loading

AlekseyTs commented Jan 25, 2018 •

edited

Loading

AlekseyTs Jan 25, 2018 •

edited

Loading

AlekseyTs commented Jan 25, 2018 •

edited

Loading

AlekseyTs commented Jan 26, 2018 •

edited

Loading

AlekseyTs Jan 26, 2018 •

edited

Loading

AlekseyTs commented Jan 26, 2018 •

edited

Loading

AlekseyTs Jan 29, 2018 •

edited

Loading

AlekseyTs Jan 29, 2018 •

edited

Loading

AlekseyTs commented Jan 29, 2018 •

edited

Loading

AlekseyTs commented Jan 29, 2018 •

edited

Loading

AlekseyTs Jan 30, 2018 •

edited

Loading

AlekseyTs commented Jan 30, 2018 •

edited

Loading