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

Adjust base access with explicit base according to latest LDM decisions #34121

Merged

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Mar 14, 2019

  • Restrict accessed members to immediate members of the specified type.
  • Change accessibility of explicit implementations in interfaces to ‘protected’.

Relates to LDM notes: https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-02-27.md#collision-of-lookup-rules-and-decisions-for-base

Related to #32054.

- Restrict accessed members to immediate members of the specified type.
- Change accessibility of explicit implementations in interfaces to ‘protected’.
@AlekseyTs AlekseyTs requested a review from a team as a code owner March 14, 2019 18:01
@AlekseyTs AlekseyTs added the Feature - Default Interface Impl Default Interface Implementation label Mar 14, 2019
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

@RikkiGibson
Copy link
Contributor

Looks like a transient failure in Windows_Desktop_Spanish_Unit_Tests?

@AlekseyTs
Copy link
Contributor Author

Looks like a transient failure in Windows_Desktop_Spanish_Unit_Tests?

The 4 CI jobs are expected to fail due to infrastructure issues

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Mar 14, 2019

}

consider removing the main method in tests when it will not be run/doesn't itself contain the code of interest to the test. #WontFix


Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:55597 in af91c73. [](commit_id = af91c73, deletion_comment = False)

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Contributor Author

}

consider removing the main method in tests when it will not be run/doesn't itself contain the code of interest to the test.

We plan to try supporting these scenarios in the future, it will be a lot easier to convert to success checks if the code is preserved.


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


Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:55597 in af91c73. [](commit_id = af91c73, deletion_comment = False)

@gafter gafter self-assigned this Mar 15, 2019
return true;
}
}
else if (!checkOnlyAccessThroughInterface && method.IsAbstract)

if (member.IsAbstract)
Copy link
Member

@jcouv jcouv Mar 15, 2019

Choose a reason for hiding this comment

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

if (member.IsAbstract) [](start = 16, length = 22)

There was no impact of removing checkOnlyAccessThroughInterface (used to be set for assigning to base event)? Feels like we're producing an extra diagnostic. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no impact of removing checkOnlyAccessThroughInterface (used to be set for assigning to base event)? Feels like we're producing an extra diagnostic.

That was compensated by the change in Binder.CheckEventValueKind


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

@jcouv
Copy link
Member

jcouv commented Mar 15, 2019

static void Main()

nit: could remove
Also in some tests below #WontFix


Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:42971 in af91c73. [](commit_id = af91c73, deletion_comment = False)

@AlekseyTs
Copy link
Contributor Author

static void Main()

We plan to try supporting these scenarios in the future, it will be a lot easier to convert to success checks if the code is preserved.


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


Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:42971 in af91c73. [](commit_id = af91c73, deletion_comment = False)

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

@AlekseyTs AlekseyTs merged commit ab2b64e into dotnet:features/DefaultInterfaceImplementation Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants