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

Update add/remove accessibility modifiers for interfaces #76324

Merged
merged 6 commits into from
Dec 9, 2024

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Dec 7, 2024

Fixes #74244
Fixes #74245
Fixes #47448

Prior to DIMs (and later language changes to effectively allow any sort of members in an interface), interface members were always public, without the ability to even specify accessibility for this.

This made the default coding for 100% of .net code that interface members did not accessibility listed (users couldn't add it even if they wanted to). This meant that defacto style for interfaces is: no accessibility modifiers listed for public members.

When we added teh 'require accessibility modifiers' feature, we recognized this, and carved out an exception for interfaces so that users would not be in the position where they would say: i want accessibility modifiers and then were forced to add accessibility modifiers to all their interfaces once the language allowed them.

To achieve this we had the following options:

  1. Never. No requirements. User can have (or not have) whatever modifiers they want on a member, and we do not complain.
  2. Always. Modifiers are always required, even if they're the default. This applies to interface members as well.
  3. ForNonInterfaceMembers. This was the default we picked knowing where the language was going. Which this option, we require modifiers on non-interface members, but we ignore interface members since the majority of .net doesn't want to have modifiers on interfaces.
  4. OmitIfDefault. Remove modifiers that match the default for the language. This is for people who do not like writing redundant modifiers, and would prefer to just exclude in that case.

To accomplish the above, we just said: we will not even look at interface members, excluding them (and their children entirely). This meant users didn't get false positives. But it also meant they did not get true positives.

This PR does final work on supporting 'Always/ForNonInterfaceMembers/Omit' even inside an interface. If the user has set 'always', then modifiers like 'public' are required inside an interface, even though they are redundant.

If the user has set ForNonInterfaceMembers then we do remove 'public' from interface members as that's the default, and the user has said they only want to write the default on non-interface members.

If the user has set OmitIfDefault then we remove public from interface members as that is the default.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner December 7, 2024 20:36
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 7, 2024
var isOmit = option == AccessibilityModifiersRequired.OmitIfDefault;
modifierAdded = !isOmit;

if (isOmit)
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 diff is terrible... sorry.

@@ -33,61 +34,74 @@ public override bool ShouldUpdateAccessibilityModifier(
if (name.Kind() == SyntaxKind.None)
return false;

// User has no preference set. Do not add or remove any accessibility modifiers.
if (option == AccessibilityModifiersRequired.Never)
return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

caller checks this. but i found it easier to reason if this code checks all 4 cases itself.

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
3 participants