[Broken Build]: Error IDE0040: Accessibility modifiers required

[bj8sk]

Issue Description

I get Error IDE0040: Accessibility modifiers required on lot's of interfaces in my build overnight on hosted agents, why did you add a breaking changes in a minor upgrade with 9.0.2?

I suspect it's this one: https://github.com/dotnet/roslyn/issues/74245

Steps to Reproduce

The hosted agents yesterday on 9.0.102 works fine. the new builds today fetched dotnet 9.0.200, and fails all over the place.

Expected Behavior

I expect builds not to break on a minor upgrade.

Actual Behavior

Builds break.

Ask us questions

No response

[BenjaminBrienen]

why did you add a breaking changes in a minor upgrade with 9.0.2

Fixing a bug is not a breaking change

[baronfel]

@BenjaminBrienen we do consider fixing bugs that will break builds unexpectedly as a breaking change from the perspective of deciding when the change should roll out. Analysis bugs are especially thorny here - we ideally would like updates to new SDK feature bands to not break users, and so if we had applied our principles uniformly there's a real chance that we would have delayed this enforcement to .NET 10 TFMs.

@bj8sk did you have your editorconfig set up to request errors for this diagnostic, or was this a change in default editorconfig configuration that caught you?

[bj8sk]

Have this line in .editorconfig for a while without issues until today: dotnet_diagnostic.IDE0040.severity = error # Accessibility modifiers required

[CyrusNajmabadi]

Have this line in .editorconfig for a while without issues until today:

What items are reporting issues where they are missing accessibility modifiers?

[CyrusNajmabadi]

we ideally would like updates to new SDK feature bands to not break users, and so if we had applied our principles uniformly

Note: These are Roslyn IDE warnings. And we have never had an 'bug fixes cannot be made' when it comes to analyzers. If SDK pulls in our analyzers and has their own bar here, then i think some system will have to happen on the SDK to not update analyzers until an appropriate release happens. :)

[jaredpar]

These are Roslyn IDE warnings.

They are also compiler warnings and hence fall into the build diagnostics category.

[CyrusNajmabadi]

As mentioned, we've never had a "bug fixes cannot be made" when it comes to analyzers. We've likely fixed thousands of issues over the years which change what analyzers report. Analyzers have always been suggestions from us, and up to users as to how they integrate those suggestions into their experiences. We've genuinely never had a constraint (since the beginning) on being able to update our analyzers in this way.

Note: SDK may have such rules. But our agreement with SDK on consuming us was that we would continue development of our features as we have so far, as we don't want to add limitations and constraints on our mainline scenarios. If SDK has limitations/constraints, i think we'll have to figure out what to do on that end. Perhaps not taking Roslyn IDE analyzer updates, except on certain cadences.

[jaredpar]

Perhaps not taking Roslyn IDE analyzer updates, except on certain cadences.

That is not realistic for many other reasons.

We've genuinely never had a constraint (since the beginning) on being able to update our analyzers in this way.

No one has said this constraint exists. @baronfel noted that analyzers, and compilers, can be thorny cases here. When the change is impactful though we give consideration to adding an entry in the breaking changes documentation so customers are aware.

[CyrusNajmabadi]

we give consideration to adding an entry in the breaking changes documentation so customers are aware.

I think we could do that. But it would def be a challenge as every analyzer bugfix seems to apply here. This is because any new suggestion that appears could break if a user has elevated that suggestion to a warning. Similarly, any suggestion that is removed could break a user if they have suppressed that suggestion, and have a warning on for unnecessary suppressions. :)

I'm primarily saying that we've been shipping and changing analyzers constantly (both in Roslyn and Roslyn-Analyzers), and haven't ever been adding such documentation. So while i'm not opposed to discussion on this or going that route, I'm def leaning more to: If we haven't needed this yet, do we really need it now?

Put in other words: what has changed to cause us to feel that we need to adjust our processes now? Thanks!

[jaredpar]

Put in other words: what has changed to cause us to feel that we need to adjust our processes now?

This is a case where the change had a non-trivial impact on a real world scenario. At that point it is more likely that it could impact other customers and raises to the level of put in a breaking change notification to increase visibility. This is not a change in process, this is how we've operated for some time.

[CyrusNajmabadi]

This is a case where the change had a non-trivial impact on a real world scenario.

Could you clarify what distinguishes that from other reports we have in the past where people have reported we're now reporting more and our position has been that that's how IDE analyzers work?

Basically, I'm not seeing what's different here. Or what set of rules i would apply that would cause us to not say anything about all the prior occurrences, but would here?

this is how we've operated for some time.

I think i'm pushing back on that. I could be wrong. But i don't recall any other times when we've done this for IDE analyzers, despite routinely updating/augmenting them in ways that impact codebases.

That we can update our analyzers and cause new suggestions to raised is a virtue that we have taken advantage of numerous times. For example, analyzers detecting broader sets of patterns to be able to help customers refine.

and raises to the level of put in a breaking change notification to increase visibility

Note: i have no problem with this. My general thought though is simply that the best thing in that case is that every change to analyzers taht impacts their results goes into the breaking changes doc.

[jaredpar]

Could you clarify what distinguishes that from other reports we have in the past where people have reported we're now reporting more and our position has been that that's how IDE analyzers work?

IDE analyzers do not get any sort of special treatment here. They are a part of build that produces diagnostics. No one at that level has the license to change diagnostics as they desire. Instead they need to fall into the existing structure with which we introduce and change existing diagnostics.

In terms of what distinguishes this from other cases: whether to add a breaking change notice is hand wavy. At the moment we're pretty sensitive to breaks and hence erroring on the side of do over do not.

But i don't recall any other times when we've done this for IDE analyzers, despite routinely updating/augmenting them in ways that impact codebases.

The IDE has made at least four changes in the last 10 months to back out new diagnostics that adversely impacted customer builds. This is not the level of back out but potentially in the level of "add a notice". True the IDE hasn't yet had to add a notice for impactful changes, mostly because they've backed out changes instead, but that doesn't mean the IDE is immune from them.

That we can update our analyzers and cause new suggestions to raised is a virtue that we have taken advantage of numerous times. For example, analyzers detecting broader sets of patterns to be able to help customers refine.

Every time you do that you're taking a calculated risk.

[tj-concentrix]

This is an issue for us too because we have this set to a warning and our pipeline builds set --warnaserror. So after upgrading to 9.0.2, any interface member which has a public modifier is now a warning & our builds are failing.

We're using the default value of for_non_interface_members, which is documented as "Prefer accessibility modifiers except for public interface members."

In the past, this has meant "this is not enforced one way or the other for interfaces" which is what we want. But now, the meaning has changed so that they're explicitly not allowed.

So my questions are:

1. Was this an unintended consequence of the linked bugfix, or was this always what is was "supposed" to mean?
2. Is it possible via .editorconfig to configure this rule so that it's a warning for non-interfaces but none for interfaces?

[CyrusNajmabadi]

Was this an unintended consequence

No. This was the intended way for this to work.

or was this always what is was "supposed" to mean?

Yup. It was always intended to work this way.

Is it possible via .editorconfig to configure this rule so that it's a warning for non-interfaces but none for interfaces?

No.

But if you have for_non_interface_members, then why do you have accessibility modifiers on interfaces? :)

If you do want accessibility modifiers on interfaces, tehn you can just set this option to always instead. for_non_interface_members exists precisely for the common .Net coding pattern where everything must have accessibility mods except interface members, since those didn't previously allow modifiers, and have been understood to be public since the start of C# :)

[tj-concentrix]

But if you have for_non_interface_members, then why do you have accessibility modifiers on interfaces? :)

We want to leave it up to the developers on whether to add them on interface members. Especially when used with default interface implementations, it can make it more clear.

But for all other types, we want to require them, even if they're the default.

If you do want accessibility modifiers on interfaces, tehn you can just set this option to always instead.

That would break all of our existing code that was written before C# 8. Even if we decided to enforce them for consistency everywhere in new code, it's not feasible for us to make this change globally to all the old code.

[CyrusNajmabadi]

In this case, the purpose of the rules is to remove choice, so code is entirely consistent and there is only one allowed way to write accessibility modifiers for a member. Sorry!

[CyrusNajmabadi]

No one at that level has the license to change diagnostics as they desire. Instead they need to fall into the existing structure with which we introduce and change existing diagnostics.

We have no existing structure for that on the IDE. There likely have been many dozens to hundreds of analyzer diagnostic changes per year.

It's been a normal part of analyzers to bugfix and have existing rules expand appropriately (often based precisely on users asking for more cases to be found).

True the IDE hasn't yet had to add a notice for impactful changes, mostly because they've backed out changes instead, but that doesn't mean the IDE is immune from them.

I'm fine with this, as I mentioned. I'm just saying that we've never done this and have no process or documentation in place for this.

I'm not pushing back here. I'm simply stating that if we want to have these rules, that we will have to write them up, disseminate them, and make it part of our review and doc process.

I'm stating this so we're on the same page here about where we currently are vs where it sounds like you'd like us to be.

[mduu]

We (and I guess many teams on the planet) have hundreds of broken CI/CD pipelines now as we thread "warnings-as-errors" (otherwise nobody will look at warnings). I think changes that break builds should not come with a point release of the compiler.

[Nasicus]

To clarify the error / bug which now happens, since nobody posted an example here yet.

We have this code:

internal interface IOrderHistoryRepository
{
    Task<HashSet<long>> GetBoughtProductIdsAsync(long userId, IReadOnlyList<long> productIds);
}

together with this rule

dotnet_diagnostic.IDE0040.severity = error # Accessibility modifiers required

up until now this was fine.

However now we get this:

##[error]#13 10.09 /app/whatever/IOrderHistoryRepository.cs(6,25): error IDE0040: Accessibility modifiers required (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0040) [/app/whatever/Dg.ProductUpdates.csproj]

As a quick fix we now disabled it like this (thought that's also not optimal - but at least we can build again):

# Rule IDE0040: Add accessibility modifiers
# https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0040
dotnet_diagnostic.IDE0040.severity = none
# options
dotnet_style_require_accessibility_modifiers = for_non_interface_members:warning

[m-gasser]

In my opinion, changing the behavior of an analyzer rule in a feature release is totally fine. If we configure analyzer rule violations to be build breaks, then breaking the build is what you opted in for!

What bothers me is how the rule now behaves, because now the configuration options that are available completely disallow, what we actually want. We ideally would like this behavior:

• Accessibility modifiers for non interface members are mandatory
• Accessibility modifiers for nested types inside interface members are mandatory
• Accessibility modifiers for members (methods, properties...) are not enforced but allowed.

We now configured for_non_interface_members but this enforced us to remove accessibility modifiers for nested types. We cannot enforce them there, we can not even have them there optionally for clarity. If we want to be able to add them for clarity, we have to remove the rule IDE0040 completely. So the issue we face is not that there was a fix to the rule in the feature release, but that the rule now disallows being configured in a way, that matches what we want. We are now in a situation, where we do not know how we even want the rule to be configured. Is there a chance that in a future update we can get the behavior we want or do we have to give up the special handling of interface members and enforce the accessibility modifiers everywhere?

[m-gasser]

For those that hit sudden build breaks over night, we also struggled with this. We concluded that this actually is an invalid configuration on our end. Feature updates can contain breaks or even bugs and you do not want those to be rolled out at a random point in time. The solution is to configure rollForward:latestPatch in global.json and when using Docker, use the images with the version containing the feature release. Renovate and Dependabot are able to update those versions, so you do not miss updates but you gain control over when the update is done.

[CyrusNajmabadi]

Is there a chance that in a future update we can get the behavior we want

Absolutely. Please file an issue at dotnet/Roslyn along for the type of enforcement you want (painful detail is recommended), and we can consider it.

[m-gasser]

Please file an issue at dotnet/Roslyn along for the type of enforcement you want (painful detail is recommended), and we can consider it.

I created https://github.com/dotnet/roslyn/issues/77253. Let me know if it is not detailed enough and I will fill in more details.

[Bluezen]

Hello, We've encountered the issue with our "warnings-as-errors" project. As a temporary fix, I considered doing something similar to @Nasicus's suggestion:

# IDE0040: Add accessibility modifiers
dotnet_diagnostic.ide0040.severity = warning
dotnet_style_require_accessibility_modifiers = for_non_interface_members

My understanding was that for_non_interface_members would suppress the IDE0040 warning within interfaces. However, the warning is now appearing on interface members that already have an accessibility modifier:

public interface IMyInterface
{
    Task WorkAsync(); // No more IDE0040, as expected, thanks to for_non_interface_members

    public int MyProperty { get; }  // ⚠️ IDE0040: Add accessibility modifiers
}

This is confusing because removing the accessibility modifier resolves the warning. It seems for_non_interface_members is intended to discourage the use of accessibility modifiers on interface members in this situation. Is that the correct behavior? That seems counterintuitive, as the name of the style rule suggests it's meant to add them.

[CyrusNajmabadi]

Here's how the option works. We did not want a simple "always requires redundant modifiers" and "always disallow redundant modifiers". That's because we knew we would have a problem the moment interfaces could have modifiers on their members.

At that point we would have a problem since the "always" option would now force everyone to add modifiers to interface members as people had been writing interface members without modifiers for 20 years, that would get much hi against the grain of the common coding pattern you see in .net

As such, we made the default for this option "for non-interface members". That effectively says: if it's not an interface member require it. But carve out interface members to work the same way they did since the early days of c# (where you do not provide the public accessibility modifier).

So the options let you either be fully strict, where redundant accessibility modifiers must be supplied. Or, the alternative is to take that everywhere except redundant interface member accessibility modifiers (for historical pleasantness).

That's really it. A basic rule, with an optional carve-out to match how c# was written for decades.

[CyrusNajmabadi]

Put more simply "for_non_interface_members" is the name for "dotnet classic" mode. Where all non interface members always have explicit accessibility and pubic interface members do not.

"always" is then the stricter mode , where all members (no matter what) need explicit accessibility.

We figured most of the ecosystem would want the first one, which is why we default to that, not "always".

[zdenek-jelinek]

I think the previous asker was, like me, flabbergasted by the message stating "IDE0040: Accessibility modifiers required", implying that modifiers are possibly missing on the offending interface members, when they in fact, need to be removed.

[CyrusNajmabadi]

That is a bug which has been fixed.

[zdenek-jelinek]

Oh, I see - https://github.com/dotnet/roslyn/pull/77188. Thanks!

[tj-concentrix]

For the time being, until I figure out how we'll handle this on our end I've created the following rules in .editorconfig to continue to completely exclude interfaces from analysis, based on the fact that we require them to begin with a capital I plus another capital letter. Not an elegant solution but it works!

[*.cs]
dotnet_style_require_accessibility_modifiers = for_non_interface_members:warning

[I[ABCDEFGHIJKLMZOPQRSTUVWXYZ]*.cs]
dotnet_diagnostic.IDE0040.severity = none

[moerwald]

It broke our builds, so this bugfix was a breaking change.

I'd:

public ISomeInterface
{
    public void SomeMethod();
}

Why do i get the error "IDE0040: Accessibility modifiers required" ? So the bug can't be fixed. There should be a mesage like NO Accessibility modifiers required.

[CyrusNajmabadi]

There should be a mesage like NO Accessibility modifiers required.

The message has been fixed and will be in the next release of the sdk.

so this bugfix was a breaking change

All analyzer changes are breaking changes. That's been the norm since we started shipping analyzers. The contract has always been though that by turning these into warnings and by setting "treat warnings as errors", the user is opting into that break happening as bugs get fixed.

[tumicz]

Looks like the same warnings are throwed now even when building with .net 8.0.406, but forcing lower .net sdk build does not help.

[mducharm]

Looks like the same warnings are throwed now even when building with .net 8.0.406, but forcing lower .net sdk build does not help.

We updated our UseDotNet@2 pipeline task to use 8.0.404, and that seems to avoid the above problem