Skip to content

ERR_ImplBadTupleNames is now being reported in correct class #24438

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.

Sign up for GitHub

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

Merged
t-camaia merged 6 commits into from
Jan 30, 2018
Merged

ERR_ImplBadTupleNames is now being reported in correct class #24438

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba19c27
ERR_ImplBadTupleNames is now being reported in correct class
t-camaia Jan 25, 2018
660b6ce
Local function added. ERR_ImplBadTupleNames being reported in the int…
t-camaia Jan 26, 2018
179ee37
snt gets conditional access. Other errors being reported in the inter…
t-camaia Jan 26, 2018
e80dc55
Local function renamed and moved. ReportAnyMismatchedConstraints meth…
t-camaia Jan 30, 2018
9085534
GetDiagnosticLocation is not a local function anymore
t-camaia Jan 30, 2018
9f762a0
GetDiagnosticLocation renamed
t-camaia Jan 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,11 +1048,11 @@ private static void ReportImplicitImplementationMatchDiagnostics(Symbol interfac

if (interfaceMethodIsAccessor && !implicitImplIsAccessor && !interfaceMethod.IsIndexedPropertyAccessor())
{
diagnostics.Add(ErrorCode.ERR_MethodImplementingAccessor, implicitImpl.Locations[0], implicitImpl, interfaceMethod, implementingType);
diagnostics.Add(ErrorCode.ERR_MethodImplementingAccessor, GetDiagnosticLocation(implicitImpl), implicitImpl, interfaceMethod, implementingType);
}
else if (!interfaceMethodIsAccessor && implicitImplIsAccessor)
{
diagnostics.Add(ErrorCode.ERR_AccessorImplementingMethod, implicitImpl.Locations[0], implicitImpl, interfaceMethod, implementingType);
diagnostics.Add(ErrorCode.ERR_AccessorImplementingMethod, GetDiagnosticLocation(implicitImpl), implicitImpl, interfaceMethod, implementingType);
}
else
{
Expand All @@ -1061,7 +1061,7 @@ private static void ReportImplicitImplementationMatchDiagnostics(Symbol interfac
if (implicitImplMethod.IsConditional)
{
// CS0629: Conditional member '{0}' cannot implement interface member '{1}' in type '{2}'
diagnostics.Add(ErrorCode.ERR_InterfaceImplementedByConditional, implicitImpl.Locations[0], implicitImpl, interfaceMethod, implementingType);
diagnostics.Add(ErrorCode.ERR_InterfaceImplementedByConditional, GetDiagnosticLocation(implicitImpl), implicitImpl, interfaceMethod, implementingType);
}
else
{
Expand All @@ -1073,7 +1073,7 @@ private static void ReportImplicitImplementationMatchDiagnostics(Symbol interfac
if (implicitImpl.ContainsTupleNames() && MemberSignatureComparer.ConsideringTupleNamesCreatesDifference(implicitImpl, interfaceMember))
{
// 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);
diagnostics.Add(ErrorCode.ERR_ImplBadTupleNames, GetDiagnosticLocation(implicitImpl), implicitImpl, interfaceMember);
}

// In constructed types, it is possible to see multiple members with the same (runtime) signature.
Expand All @@ -1090,10 +1090,24 @@ private static void ReportImplicitImplementationMatchDiagnostics(Symbol interfac
else if (MemberSignatureComparer.RuntimeImplicitImplementationComparer.Equals(interfaceMember, member) && !member.IsAccessor())
{
// CONSIDER: Dev10 does not seem to report this for indexers or their accessors.
diagnostics.Add(ErrorCode.WRN_MultipleRuntimeImplementationMatches, member.Locations[0], member, interfaceMember, implementingType);
diagnostics.Add(ErrorCode.WRN_MultipleRuntimeImplementationMatches, GetDiagnosticLocation(member), member, interfaceMember, implementingType);
}
}
}

Location GetDiagnosticLocation(Symbol member)
Copy link
Member

@jaredpar jaredpar Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mark as private

Copy link
Contributor Author

@t-camaia t-camaia Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@t-camaia t-camaia Jan 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@AlekseyTs AlekseyTs Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@t-camaia t-camaia Jan 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@AlekseyTs AlekseyTs Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

/// <summary>
Expand All @@ -1107,7 +1121,7 @@ private static void ReportImplicitImplementationMismatchDiagnostics(Symbol inter
{
var @interface = interfaceMember.ContainingType;
SourceMemberContainerTypeSymbol snt = implementingType as SourceMemberContainerTypeSymbol;
interfaceLocation = snt.GetImplementsLocation(@interface) ?? implementingType.Locations[0];
interfaceLocation = snt?.GetImplementsLocation(@interface) ?? implementingType.Locations[0];
}
else
{
Expand Down
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenImplicitImplementationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1749,8 +1749,9 @@ public static void Main()
Derived.Method()");

comp.VerifyDiagnostics(
// (17,17): warning CS1956: Member 'Derived<int, string>.Method(int)' implements interface member 'Interface<int>.Method(int)' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "Method").WithArguments("Derived<int, string>.Method(int)", "Interface<int>.Method(int)", "Derived")); // No errors
// (20,58): warning CS1956: Member 'Derived<int, string>.Method(int)' implements interface member 'Interface<int>.Method(int)' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
// class Derived : Derived<int, string>, Interface<string>, Interface<int>
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "Interface<int>").WithArguments("Derived<int, string>.Method(int)", "Interface<int>.Method(int)", "Derived").WithLocation(20, 58)); // No errors
}

[Fact]
Expand Down
18 changes: 9 additions & 9 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22997,9 +22997,9 @@ class Derived : Base, I<(int notA, int notB)>
";
var comp = CreateStandardCompilation(source, references: s_valueTupleRefs);
comp.VerifyDiagnostics(
// (8,27): error CS8141: The tuple element names in the signature of method 'Base.M()' must match the tuple element names of interface method 'I<(int notA, int notB)>.M()' (including on the return type).
// public (int a, int b) M() { return (1, 2); }
Diagnostic(ErrorCode.ERR_ImplBadTupleNames, "M").WithArguments("Base.M()", "I<(int notA, int notB)>.M()").WithLocation(8, 27)
// (10,23): error CS8141: The tuple element names in the signature of method 'Base.M()' must match the tuple element names of interface method 'I<(int notA, int notB)>.M()' (including on the return type).
// class Derived : Base, I<(int notA, int notB)>
Diagnostic(ErrorCode.ERR_ImplBadTupleNames, "I<(int notA, int notB)>").WithArguments("Base.M()", "I<(int notA, int notB)>.M()").WithLocation(10, 23)
);
}

Expand All @@ -23023,9 +23023,9 @@ class Derived : Base, I<(int notA, int notB)>
";
var comp = CreateStandardCompilation(source, references: s_valueTupleRefs);
comp.VerifyDiagnostics(
// (8,27): error CS8141: The tuple element names in the signature of method 'Base.M()' must match the tuple element names of interface method 'I<(int notA, int notB)>.M()' (including on the return type).
// public (int a, int b) M() { return (1, 2); }
Diagnostic(ErrorCode.ERR_ImplBadTupleNames, "M").WithArguments("Base.M()", "I<(int notA, int notB)>.M()").WithLocation(8, 27)
// (10,23): error CS8141: The tuple element names in the signature of method 'Base.M()' must match the tuple element names of interface method 'I<(int notA, int notB)>.M()' (including on the return type).
// class Derived : Base, I<(int notA, int notB)>
Diagnostic(ErrorCode.ERR_ImplBadTupleNames, "I<(int notA, int notB)>").WithArguments("Base.M()", "I<(int notA, int notB)>.M()").WithLocation(10, 23)
);
}

Expand All @@ -23049,9 +23049,9 @@ class Derived : Base, I<(int, int)>
";
var comp = CreateStandardCompilation(source, references: s_valueTupleRefs);
comp.VerifyDiagnostics(
// (8,27): error CS8141: The tuple element names in the signature of method 'Base.M()' must match the tuple element names of interface method 'I<(int, int)>.M()' (including on the return type).
// public (int a, int b) M() { return (1, 2); }
Diagnostic(ErrorCode.ERR_ImplBadTupleNames, "M").WithArguments("Base.M()", "I<(int, int)>.M()").WithLocation(8, 27)
// (10,23): error CS8141: The tuple element names in the signature of method 'Base.M()' must match the tuple element names of interface method 'I<(int, int)>.M()' (including on the return type).
// class Derived : Base, I<(int, int)>
Diagnostic(ErrorCode.ERR_ImplBadTupleNames, "I<(int, int)>").WithArguments("Base.M()", "I<(int, int)>.M()").WithLocation(10, 23)
);
}

Expand Down
21 changes: 12 additions & 9 deletions src/Compilers/CSharp/Test/Semantic/Semantics/InheritanceBindingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4247,9 +4247,9 @@ public class Derived : Base<int>, Interface<int, int>
}
";
CompileAndVerifyDiagnostics(text, new ErrorDescription[] {
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 12, Column = 17, IsWarning = true }, //Both Base methods implement Interface.Method(int)
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 12, Column = 17, IsWarning = true }, //Both Base methods implement Interface.Method(T)
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 12, Column = 17, IsWarning = true }, //Both Base methods implement Interface.Method(U)
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 15, Column = 35, IsWarning = true }, //Both Base methods implement Interface.Method(int)
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 15, Column = 35, IsWarning = true }, //Both Base methods implement Interface.Method(T)
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 15, Column = 35, IsWarning = true }, //Both Base methods implement Interface.Method(U)
});
}

Expand All @@ -4276,12 +4276,15 @@ public class Derived : Base<int>, Interface<int, int>
";
// CONSIDER: Dev10 doesn't report these warnings - not sure why
CreateStandardCompilation(text).VerifyDiagnostics(
// (12,17): warning CS1956: Member 'Base<int>.this[int]' implements interface member 'Interface<int, int>.this[int]' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "this").WithArguments("Base<int>.this[int]", "Interface<int, int>.this[int]", "Derived"),
// (12,17): warning CS1956: Member 'Base<int>.this[int]' implements interface member 'Interface<int, int>.this[int]' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "this").WithArguments("Base<int>.this[int]", "Interface<int, int>.this[int]", "Derived"),
// (12,17): warning CS1956: Member 'Base<int>.this[int]' implements interface member 'Interface<int, int>.this[int]' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "this").WithArguments("Base<int>.this[int]", "Interface<int, int>.this[int]", "Derived"));
// (15,35): warning CS1956: Member 'Base<int>.this[int]' implements interface member 'Interface<int, int>.this[int]' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
// public class Derived : Base<int>, Interface<int, int>
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "Interface<int, int>").WithArguments("Base<int>.this[int]", "Interface<int, int>.this[int]", "Derived").WithLocation(15, 35),
// (15,35): warning CS1956: Member 'Base<int>.this[int]' implements interface member 'Interface<int, int>.this[int]' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
// public class Derived : Base<int>, Interface<int, int>
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "Interface<int, int>").WithArguments("Base<int>.this[int]", "Interface<int, int>.this[int]", "Derived").WithLocation(15, 35),
// (15,35): warning CS1956: Member 'Base<int>.this[int]' implements interface member 'Interface<int, int>.this[int]' in type 'Derived'. There are multiple matches for the interface member at run-time. It is implementation dependent which method will be called.
// public class Derived : Base<int>, Interface<int, int>
Diagnostic(ErrorCode.WRN_MultipleRuntimeImplementationMatches, "Interface<int, int>").WithArguments("Base<int>.this[int]", "Interface<int, int>.this[int]", "Derived").WithLocation(15, 35));
}

[Fact]
Expand Down
51 changes: 51 additions & 0 deletions src/Compilers/CSharp/Test/Symbol/Symbols/AccessorOverriddenOrHiddenMembersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,57 @@ class Derived : Base, I
Assert.Equal(derivedGetter, derivedClass.FindImplementationForInterfaceMember(interfaceMethod));
}

[Fact]
public void ImplementingAccessorWithNonAccessorMayReportInInterfaceReference()
{
var source = @"
interface I<T>
{
T P { get; }
}
class Base
{
public int get_P() { return 1; }
}
class Derived : Base, I<int> // CS0470 must be reported in ""I<int>""
{
}
";

var comp = CreateStandardCompilation(source);
comp.VerifyDiagnostics(
// (10,23): error CS0470: Method 'Base.get_P()' cannot implement interface accessor 'I<int>.P.get' for type 'Derived'. Use an explicit interface implementation.
// class Derived : Base, I<int>
Diagnostic(ErrorCode.ERR_MethodImplementingAccessor, "I<int>").WithArguments("Base.get_P()", "I<int>.P.get", "Derived").WithLocation(10, 23),
// (10,23): error CS0535: 'Derived' does not implement interface member 'I<int>.P'
// class Derived : Base, I<int>
Diagnostic(ErrorCode.ERR_UnimplementedInterfaceMember, "I<int>").WithArguments("Derived", "I<int>.P").WithLocation(10, 23));
}

[Fact]
public void ImplementingNonAccessorWithAccessorMayReportInInterfaceReference()
{
var source = @"
interface I<T>
{
T get_P();
}
class Base
{
public int P { get; }
}
class Derived : Base, I<int> // CS0686 must be reported in ""I<int>""
{
}
";

var comp = CreateStandardCompilation(source);
comp.VerifyDiagnostics(
// (10,23): error CS0686: Accessor 'Base.P.get' cannot implement interface member 'I<int>.get_P()' for type 'Derived'. Use an explicit interface implementation.
// class Derived : Base, I<int>
Diagnostic(ErrorCode.ERR_AccessorImplementingMethod, "I<int>").WithArguments("Base.P.get", "I<int>.get_P()", "Derived").WithLocation(10, 23));
}

[Fact]
public void PropertyHidesBetterImplementation()
{
Expand Down
31 changes: 29 additions & 2 deletions src/Compilers/CSharp/Test/Symbol/Symbols/SymbolErrorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10941,7 +10941,7 @@ class C
// CS0625: See AttributeTests_StructLayout.ExplicitFieldLayout_Errors

[Fact]
public void CS0629ERR_InterfaceImplementedByConditional()
public void CS0629ERR_InterfaceImplementedByConditional01()
{
var text = @"interface MyInterface
{
Expand All @@ -10964,6 +10964,33 @@ public static void Main()
new ErrorDescription { Code = (int)ErrorCode.ERR_InterfaceImplementedByConditional, Line = 9, Column = 17 });
}

[Fact]
public void CS0629ERR_InterfaceImplementedByConditional02()
{
var source = @"
using System.Diagnostics;

interface I<T>
{
void M(T x);
}
class Base
{
[Conditional(""debug"")]
public void M(int x) {}
}
class Derived : Base, I<int>
{
}
";

var comp = CreateStandardCompilation(source);
comp.VerifyDiagnostics(
// (13,23): error CS0629: Conditional member 'Base.M(int)' cannot implement interface member 'I<int>.M(int)' in type 'Derived'
// class Derived : Base, I<int>
Diagnostic(ErrorCode.ERR_InterfaceImplementedByConditional, "I<int>").WithArguments("Base.M(int)", "I<int>.M(int)", "Derived").WithLocation(13, 23));
}

[Fact]
public void CS0633ERR_BadArgumentToAttribute()
{
Expand Down Expand Up @@ -17819,7 +17846,7 @@ static void Main()
}
";
var comp = DiagnosticsUtils.VerifyErrorsAndGetCompilationWithMscorlib(text,
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 9, Column = 24, IsWarning = true });
new ErrorDescription { Code = (int)ErrorCode.WRN_MultipleRuntimeImplementationMatches, Line = 20, Column = 33, IsWarning = true });
}

[Fact]
Expand Down