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

Error for using positional member that is hidden #52660

Merged
merged 11 commits into from
Apr 19, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,17 @@
return a ?? (b ? 1 : 2);
}
```

3. https://github.com/dotnet/roslyn/issues/52630 In C# 9 (.NET 5, Visual Studio 16.9), it is possible that a record uses a hidden member from a base type as a positional member. In Visual Studio 16.10, this is now an error:
```csharp
record Base
{
public int I { get; init; }
}
record Derived(int I) // error: The positional member `Base.I` found corresponding to parameter `I` is hidden.
Copy link
Member

@Youssef1313 Youssef1313 Apr 16, 2021

Choose a reason for hiding this comment

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

Suggested change
record Derived(int I) // error: The positional member `Base.I` found corresponding to parameter `I` is hidden.
record Derived(int I) // error: The positional member 'Base.I' found corresponding to this parameter is hidden.
``` #Resolved

: Base
{
public int I() { return 0; }
}
```

3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6606,4 +6606,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_InheritingFromRecordWithSealedToString" xml:space="preserve">
<value>Inheriting from a record with a sealed 'Object.ToString' is not supported in C# {0}. Please use language version '{1}' or greater.</value>
</data>
<data name="ERR_HiddenPositionalMember" xml:space="preserve">
<value>The positional member '{0}' found corresponding to this parameter is hidden.</value>
</data>
</root>
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1934,7 +1934,8 @@ internal enum ErrorCode

#region diagnostics introduced for C# 10.0

ERR_InheritingFromRecordWithSealedToString = 8912
ERR_InheritingFromRecordWithSealedToString = 8912,
ERR_HiddenPositionalMember = 8913,

#endregion

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3497,7 +3497,7 @@ void addDeconstruct(SynthesizedRecordConstructor ctor, ImmutableArray<PropertySy

if (!memberSignatures.TryGetValue(targetMethod, out Symbol? existingDeconstructMethod))
{
members.Add(new SynthesizedRecordDeconstruct(this, ctor, properties, memberOffset: members.Count, diagnostics));
members.Add(new SynthesizedRecordDeconstruct(this, ctor, properties, paramList, memberOffset: members.Count, diagnostics));
}
else
{
Expand Down Expand Up @@ -3721,15 +3721,15 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
}
if (existingMember is null)
{
addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: false, diagnostics));
addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: false, diagnostics), param);
}
else if (existingMember is PropertySymbol { IsStatic: false, GetMethod: { } } prop
&& prop.TypeWithAnnotations.Equals(param.TypeWithAnnotations, TypeCompareKind.AllIgnoreOptions))
{
// There already exists a member corresponding to the candidate synthesized property.
if (isInherited && prop.IsAbstract)
{
addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: true, diagnostics));
addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: true, diagnostics), param);
}
else
{
Expand All @@ -3745,23 +3745,23 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
param.TypeWithAnnotations,
param.Name);
}

void addProperty(SynthesizedRecordPropertySymbol property)
{
existingOrAddedMembers.Add(property);
members.Add(property);
Debug.Assert(property.GetMethod is object);
Debug.Assert(property.SetMethod is object);
members.Add(property.GetMethod);
members.Add(property.SetMethod);
members.Add(property.BackingField);

builder.AddInstanceInitializerForPositionalMembers(new FieldOrPropertyInitializer(property.BackingField, paramList.Parameters[param.Ordinal]));
addedCount++;
}
}

return existingOrAddedMembers.ToImmutableAndFree();

void addProperty(SynthesizedRecordPropertySymbol property, ParameterSymbol param)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
existingOrAddedMembers.Add(property);
members.Add(property);
Debug.Assert(property.GetMethod is object);
Debug.Assert(property.SetMethod is object);
members.Add(property.GetMethod);
members.Add(property.SetMethod);
members.Add(property.BackingField);

builder.AddInstanceInitializerForPositionalMembers(new FieldOrPropertyInitializer(property.BackingField, paramList.Parameters[param.Ordinal]));
addedCount++;
}
}

void addObjectEquals(MethodSymbol thisEquals)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using Microsoft.Cci;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand All @@ -17,18 +15,22 @@ internal sealed class SynthesizedRecordDeconstruct : SynthesizedRecordOrdinaryMe
{
private readonly SynthesizedRecordConstructor _ctor;
private readonly ImmutableArray<PropertySymbol> _properties;
private readonly ParameterListSyntax _parameters;

public SynthesizedRecordDeconstruct(
SourceMemberContainerTypeSymbol containingType,
SynthesizedRecordConstructor ctor,
ImmutableArray<PropertySymbol> properties,
ParameterListSyntax parameters,
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
int memberOffset,
BindingDiagnosticBag diagnostics)
: base(containingType, WellKnownMemberNames.DeconstructMethodName, hasBody: true, memberOffset, diagnostics)
{
Debug.Assert(properties.All(prop => prop.GetMethod is object));
Debug.Assert(ctor.ParameterCount == parameters.ParameterCount);
_ctor = ctor;
_properties = properties;
_parameters = parameters;
}

protected override DeclarationModifiers MakeDeclarationModifiers(DeclarationModifiers allowedModifiers, BindingDiagnosticBag diagnostics)
Expand Down Expand Up @@ -59,6 +61,27 @@ protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymb

protected override int GetParameterCountFromSyntax() => _ctor.ParameterCount;

protected override void MethodChecks(BindingDiagnosticBag diagnostics)
{
base.MethodChecks(diagnostics);

if (ParameterCount == _properties.Length)
Copy link
Member

@Youssef1313 Youssef1313 Apr 16, 2021

Choose a reason for hiding this comment

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

  • When can the this condition be false?

  • What if the Deconstruct method is declared explicitly:

    public record Base
    {
        public int I { get; set; } = 42;
        public Base(int ignored) { }
    
        public void Deconstruct(out string s, out string s2) { s = s2 = "";  }
    }
    public record C(int I) : Base(I)
    {
        public void I() { }
    
        public void Deconstruct(out string s, out string s2) { s = s2 = "";  }
    }
    ``` #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

  • the condition is false when addProperties in SourceMemberContainerSymbol doesn't find a property for each parameter (we produce ERR_BadRecordMemberForPositionalParameter in that case).
  • thanks. I need to think more about scenarios where Deconstruct is declared in source. We should produce a diagnostic too, but currently don't.

In reply to: 614614357 [](ancestors = 614614357)

{
for (int i = 0; i < _properties.Length; i++)
{
var property = _properties[i];
NamedTypeSymbol containingType = this.ContainingType;
Copy link
Member

@Youssef1313 Youssef1313 Apr 16, 2021

Choose a reason for hiding this comment

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

Could be moved outside the loop? #Resolved


if (property.ContainingType != (object)containingType &&
!containingType.GetMembers(property.Name).IsEmpty)
{
// The positional member was inherited but is hidden by a member of the current record type
diagnostics.Add(ErrorCode.ERR_HiddenPositionalMember, _parameters.Parameters[i].Identifier.GetLocation(), property);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

internal override void GenerateMethodBody(TypeCompilationState compilationState, BindingDiagnosticBag diagnostics)
{
var F = new SyntheticBoundNodeFactory(this, ContainingType.GetNonNullSyntaxNode(), compilationState, diagnostics);
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading