Skip to content

Report an error for an implicit implementation of an inaccessible interface members #61717

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

Jump to bottom
Merged
AlekseyTs merged 6 commits into from
Jun 13, 2022
Merged

Report an error for an implicit implementation of an inaccessible interface members #61717

AlekseyTs merged 6 commits into from
Jun 13, 2022

Conversation

@AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Jun 6, 2022 •
edited by jcouv
Loading

Closes #60885.

@AlekseyTs AlekseyTs requested review from a team as code owners June 6, 2022 15:20
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 6, 2022

@dotnet/roslyn-compiler Please review.

3 similar comments
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 7, 2022

@dotnet/roslyn-compiler Please review.

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 8, 2022

@dotnet/roslyn-compiler Please review.

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 8, 2022

@dotnet/roslyn-compiler Please review.

jcouv
jcouv reviewed Jun 8, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs Outdated
bool suppressRegularValidation = false;

if (!canBeImplementedImplicitlyInCSharp9 && interfaceMember.Kind == SymbolKind.Method &&
(object)implementingBaseOpt == null) // Otherwise any approprite errors are going to be reported for the base.
Copy link
Member

@jcouv jcouv Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

approprite

typo: appropriate (existed in old code) #Closed

@jcouv jcouv self-assigned this Jun 8, 2022
jcouv
jcouv approved these changes Jun 8, 2022
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

AlekseyTs
AlekseyTs commented Jun 8, 2022
View reviewed changes
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 8, 2022

@dotnet/roslyn-compiler For the second review.

@runfoapp runfoapp bot mentioned this pull request Jun 8, 2022
Open
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 9, 2022

@dotnet/roslyn-compiler For the second review.

@jcouv
Copy link
Member

jcouv commented Jun 9, 2022 •
edited
Loading

Probably needs a corresponding spec update as well.
Also, do we need to document a breaking change?

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 10, 2022

@dotnet/roslyn-compiler For the second review.

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Jun 13, 2022

@333fred, @RikkiGibson, @dotnet/roslyn-compiler For the second review.

333fred
333fred reviewed Jun 13, 2022
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs Outdated

private void ValidatePropertyModifiers_11_02(string source1, string source2,
DiagnosticDescription[] expectedIn9,
params DiagnosticDescription[] expectedAccrossAssemplyBoundaries)
Copy link
Member

@333fred 333fred Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

expectedAccrossAssemplyBoundaries -> expectedAcrossAssemblyBoundaries

Copy link
Member

@333fred 333fred Jun 13, 2022

Choose a reason for hiding this comment

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

Worth doing a find/replace across the entire file, this was copy/pasted a few times.

333fred
333fred approved these changes Jun 13, 2022
View reviewed changes
Copy link
Member

@333fred 333fred 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 2), minor spelling mistake.

@AlekseyTs AlekseyTs enabled auto-merge (squash) June 13, 2022 18:23
@AlekseyTs AlekseyTs disabled auto-merge June 13, 2022 19:31
@AlekseyTs AlekseyTs enabled auto-merge (squash) June 13, 2022 19:39
@AlekseyTs AlekseyTs merged commit dada467 into dotnet:main Jun 13, 2022
@ghost ghost added this to the Next milestone Jun 13, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Default Interface Impl Default Interface Implementation Feature - StaticAbstractMembersInInterfaces

Projects

None yet

Milestone

17.3 P3

Development

Successfully merging this pull request may close these issues.

Inconsistency around accessibility checks for interface implementations

4 participants

@AlekseyTs @jcouv @333fred @RikkiGibson