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

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 15, 2021

Fixes #52630

Relates to test plan: #51199
Spec change: dotnet/csharplang#4673

@jcouv jcouv marked this pull request as ready for review April 16, 2021 04:25
@jcouv jcouv requested a review from a team as a code owner April 16, 2021 04:25
{
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

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

{
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)

@jcouv jcouv marked this pull request as draft April 16, 2021 17:41
@jcouv jcouv marked this pull request as ready for review April 16, 2021 22:16
@jcouv jcouv requested a review from AlekseyTs April 16, 2021 22:18

if (isInherited)
{
validateNotHidden(prop, param);
Copy link
Member

@Youssef1313 Youssef1313 Apr 17, 2021

Choose a reason for hiding this comment

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

If prop is abstract, it won't be validated, is that intended? i.e:

abstract record Base
{
    public abstract int I { get; init; }
}

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

#Resolved

Copy link
Member Author

@jcouv jcouv Apr 17, 2021

Choose a reason for hiding this comment

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

In that scenario Derived defined a concrete property I and a method I() (that's an error). The positional member is Derived.I so we didn't pick a hidden member, so the new error doesn't come into play.
Added tests to illustrate. Thanks for the scenario. #Resolved

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 19, 2021

Done with review pass (commit 9)


In reply to: 822644422

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 11)

@jcouv jcouv merged commit 3abea95 into dotnet:main Apr 19, 2021
@ghost ghost modified the milestones: 16.10, Next Apr 19, 2021
@jcouv jcouv deleted the hidden-member branch April 19, 2021 21:54
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Records: Should produce an error when member we pick as positional member is hidden
5 participants