Fixes #18240 - "Generate class ..." causes inconsistent accessibility by d0pare · Pull Request #18455 · dotnet/roslyn

d0pare · 2017-04-05T05:55:24Z

Currently if nested class is protected then we generate internal class which can cause compilation error if protected class is under public class.
This PR also fixes that issue.

sharwell · 2017-04-05T14:29:18Z

😎 Welcome to the project!

d0pare · 2017-04-05T14:45:58Z

Thank you 😄

sharwell · 2017-04-05T14:52:49Z

src/EditorFeatures/CSharpTest/Diagnostics/GenerateType/GenerateTypeTests.cs

💡 Another good test would be the following. Unlike other types, if you omit the accessibility keyword from an interface member (which is actually a requirement in this location), the result is effectively public instead of private.

public interface A { void B<T>() where T : D; }

Pilchie · 2017-04-05T15:05:19Z

src/Features/CSharp/Portable/GenerateType/CSharpGenerateTypeService.cs

Nit: Parameters should be indented 4 past private here.

Pilchie · 2017-04-05T15:06:38Z

src/Features/CSharp/Portable/GenerateType/CSharpGenerateTypeService.cs

Nit: space after // and use Sentence case for comments.

Pilchie · 2017-04-05T15:07:45Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

Nit: 4 spaces, not 3.

Pilchie · 2017-04-05T15:09:24Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

Sentence case, and rationalize with the numbering in the rest of the comments in this method.

Pilchie · 2017-04-05T15:11:58Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

Spelling - typeDeclarations

Pilchie · 2017-04-05T15:12:13Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

Nit: space after foreach

Pilchie · 2017-04-05T15:12:23Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

Nit: space after if

Pilchie · 2017-04-05T15:13:12Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

Consider: Switching the sense of this and renaming to something like: AllContainingTypesArePublic.

Pilchie · 2017-04-05T15:14:11Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

Related to @sharwell's comment - what is DeclaredAccessibility for interface members?

It is always public

sharwell

I don't see any red flags here. I'm requesting a second review from @CyrusNajmabadi or someone else who's more familiar with the nuances of the extension methods you modified.

I noticed some small things which fall into a category of "I would typically just fix these when I merged the pull request":

The indentation has some discrepancies in a couple places. Also there are two cases where a space is missing after the keyword in foreach( and if(.
The tests cover many cases, but there are a few remaining. Considering how many cases were missing from the original code it's hard to complain here. 😆

I notice that only one of two available code fixes gets tested. I'm assuming this is due to a limitation in the test infrastructure where it's somewhere between non-trivial and impossible to test code fixes that affect other files. When I was working on StyleCop Analyzers, we faced a similar situation for a long time. Eventually @otac0n figured out a way to test these cases in DotNetAnalyzers/StyleCopAnalyzers#2140, but assuming it's not already in place we're talking about a lot of work down in the depths of the test infrastructure to implement something similar here.

d0pare · 2017-04-05T17:25:15Z

Created new commit (will squash at the merge hopefully 😄) for requested action items from code review.
Added 2 more tests and renamed new tests' names to more meaningful names.

Pilchie

Thanks for the style fixes. LGTM, but I'd love @CyrusNajmabadi to take a quick look too.

d0pare · 2017-04-05T17:55:11Z

Squashed commits for next review

sharwell · 2017-04-05T18:16:09Z

src/EditorFeatures/CSharpTest/Diagnostics/GenerateType/GenerateTypeTests.cs

❗️ This one should be internal because Employee is internal. It should only generate a public type here if the interface is also public.

CyrusNajmabadi · 2017-04-05T18:22:01Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

When can you have a TypeConstraint that is not parented by a TypeParameterConstraintClause?

I cannot imagine case when TypeConstraint cannot be parented by TypeParameterConstraintClause. Just putted for sanity, will remove it.

CyrusNajmabadi · 2017-04-05T18:22:38Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

this step does not seem necessary. Can you clarify why you need to do it?

Same type is passed to DetermineAccessibilityConstraint which has type = GetOutermostType(type);

My point is this: immediately after you make this call, you then call type.GetAncestors<TypeDeclarationSyntax>(). You then do not use type again. So this call, and the overwriting of 'type' seems unnecessary since it will not affect the results you get from type.GetAncestors<TypeDeclarationSyntax>() :)

Ah, sorry my bad. Will fix this.

No worries :)

CyrusNajmabadi · 2017-04-05T18:23:29Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

This check doesn't match your method name. For example, you'll return 'true' if one of the containing types i protected.

That may be what you want, but in tha t case the method name should be AllContainingTypesArePublicOrProtected.

I will disagree because topmost type is either public or internal. Protected can be only in the middle.

AllContainingTypeAreNotPrivateOrInternal then :)

Actually, rgd:

I will disagree because topmost type is either public or internal. Protected can be only in the middle.

But you still return false, even if the topmost type is internal. So it's correct to say "AllContainingTypesArePublicOrProtected".

Got your point.

CyrusNajmabadi · 2017-04-05T18:23:54Z

src/Features/CSharp/Portable/GenerateType/CSharpGenerateTypeService.cs

nit, consider use ? : expression to simplify this.

CyrusNajmabadi · 2017-04-05T18:24:31Z

src/EditorFeatures/CSharpTest/Diagnostics/GenerateType/GenerateTypeTests.cs

new tests shoudl include a [WorkItem(bugnumber, "github-issue-link")] attribute on them.

📝 For this issue, they'll have this form:

[WorkItem(18240, "https://github.com/dotnet/roslyn/issues/18240")]

CyrusNajmabadi · 2017-04-05T18:26:40Z

src/EditorFeatures/CSharpTest/Diagnostics/GenerateType/GenerateTypeTests.cs

this feels strange. Command should not be public here.

CyrusNajmabadi · 2017-04-05T18:27:01Z

src/EditorFeatures/CSharpTest/Diagnostics/GenerateType/GenerateTypeTests.cs

i don't think command should be public here either.

d0pare · 2017-04-05T19:18:11Z

Fixed all issues from review

CyrusNajmabadi · 2017-04-05T19:39:08Z

src/Features/CSharp/Portable/GenerateType/CSharpGenerateTypeService.cs

nit: try to wrap lines longer than what github can display. our general patter for ?: is

accessibility = AllContainingTypesArePublicOrProtected(state, semanticModel, cancellationToken) ? Accessibility.Public : Accessibility.Internal;

333fred · 2017-04-05T21:10:32Z

Currently investigating vsi failures. Don't read anything into vsi failures for now.

d0pare · 2017-04-07T20:07:14Z

Rebased onto roslyn/master

sharwell · 2018-04-24T16:00:17Z

@dopare This one has been sitting and I'd like to get it prepared for merge. Would you like to handle the updates, or would you like me to "take over" the pull request since it's likely to require more preparation given the amount of time it sat for?

sharwell

Two items to verify before merge:

Retest against current master branch (retest is requested, but may require rebase)
Add tests for protected private and update implementation if necessary

d0pare · 2018-04-24T16:02:33Z

@sharwell Please take over. Currently I don't have enough time to make updates.

jinujoseph · 2018-06-03T01:34:44Z

approved to merge for 15.8.preview3

jinujoseph · 2018-06-04T07:57:12Z

test windows_release_vs-integration_prtest

sharwell · 2018-06-04T11:49:22Z

@dopare Thank you! 😄

d0pare · 2018-06-04T12:11:15Z

Holy moly, finally 😄

dnfclas added the cla-already-signed label Apr 5, 2017

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 5, 2017

sharwell self-requested a review April 5, 2017 14:31

sharwell reviewed Apr 5, 2017

View reviewed changes

Pilchie added the Area-IDE label Apr 5, 2017

Pilchie requested changes Apr 5, 2017

View reviewed changes

sharwell approved these changes Apr 5, 2017

View reviewed changes

Pilchie approved these changes Apr 5, 2017

View reviewed changes

Pilchie requested a review from CyrusNajmabadi April 5, 2017 17:42

sharwell suggested changes Apr 5, 2017

View reviewed changes

CyrusNajmabadi reviewed Apr 5, 2017

View reviewed changes

sharwell approved these changes Apr 5, 2017

View reviewed changes

Fixes #18240 - "Generate class ..." causes inconsistent accessibility

940c372

Pilchie assigned sharwell Sep 8, 2017

sharwell suggested changes Apr 24, 2018

View reviewed changes

sharwell added this to the 15.8 milestone May 25, 2018

sharwell added 3 commits May 25, 2018 14:02

Merge remote-tracking branch 'dotnet/master' into dopare/master

557fe3e

Use a Theory to consolidate test cases

4a0e0e2

Fix Generate Type accessibility for 'protected private'

a9c5ed5

d0pare requested a review from a team as a code owner May 25, 2018 19:24

sharwell approved these changes May 25, 2018

View reviewed changes

jinujoseph closed this Jun 3, 2018

jinujoseph reopened this Jun 3, 2018

jinujoseph added the Approved to merge label Jun 3, 2018

sharwell merged commit 00047b9 into dotnet:master Jun 4, 2018

Conversation

d0pare commented Apr 5, 2017

Uh oh!

sharwell commented Apr 5, 2017

Uh oh!

d0pare commented Apr 5, 2017

Uh oh!

sharwell Apr 5, 2017 • edited by Pilchie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

d0pare commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pilchie left a comment

Choose a reason for hiding this comment

Uh oh!

d0pare commented Apr 5, 2017

Uh oh!

sharwell Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Apr 5, 2017 •

edited by Pilchie

Loading

d0pare commented Apr 5, 2017 •

edited

Loading

sharwell Apr 5, 2017 •

edited

Loading

sharwell left a comment •

edited

Loading