Centralize recognized feature flags by jcouv · Pull Request #81591 · dotnet/roslyn

jcouv · 2025-12-09T02:12:13Z

The compiler recognizes some magic feature flag strings. This PR moves those known strings to a centralized location and funnels most of the feature flag checks through common helpers.

333fred · 2025-12-09T22:08:18Z

src/Compilers/Core/Portable/CommandLine/Feature.cs

This file makes it clear that we have sinned in our naming.

333fred · 2025-12-09T22:11:59Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

            }
        }

+        /// <remarks>Known feature flags should use <see cref="FeatureFlag"/></remarks>


Is there a reflection-based assert we could write to verify this? #Resolved

We have some tests that pass arbitrary feature flags into the APIs, so I didn't tighten this with assertions.
Let me take a look at how many tests would need updating.

333fred · 2025-12-09T22:12:40Z

src/Compilers/CSharp/Test/Emit2/Emit/EditAndContinue/EditAndContinueTests.cs

            var testData0 = new CompilationTestData();
            var bytes0 = compilation0.EmitToArray(testData: testData0);
            var generation0 = CreateInitialBaseline(compilation0,
-                ModuleMetadata.CreateFromImage(bytes0),


Whitespace changes? #Resolved

Thanks. Will revert.
~~No sure what's happening here. For some reason the file after update looks correct in VS...~~ Stupid mistake, confused two worktrees

AlekseyTs · 2025-12-10T01:38:10Z

src/Compilers/Core/Portable/CommandLine/FeatureFlag.cs

+
+namespace Microsoft.CodeAnalysis;
+
+internal static class FeatureFlag


FeatureFlag

Consider adding a comment for each of the "flags" #Resolved

BTW, the members do not look like flags. I think we used to refer to them simply as features or "compiler features".

Renamed to "Feature" to remove "Flag"
For comments, that's beyond what I intend for this PR. There's more work we could do here, for instance we have an open issue to remove pdb-path-determinism, but I'll leave that for later.

AlekseyTs · 2025-12-10T01:47:54Z

Done with review pass (commit 1)

jjonescz · 2025-12-10T10:28:17Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

                if (!_lazyEmitNullablePublicOnly.HasValue())
                {
-                    bool value = SyntaxTrees.FirstOrDefault()?.Options?.Features?.ContainsKey("nullablePublicOnly") == true;
+                    bool value = SyntaxTrees.FirstOrDefault()?.Options?.HasFeature(CodeAnalysis.Feature.NullablePublicOnly) == true;


This doesn't seem strictly equivalent to the previous code - null value in the feature dictionary would previously be accepted.

There are other changes like this in the PR (replacing Features?.ContainsKey with HasFeature).

But I guess the new behavior is better, just wanted to point out this is a small breaking change. #Resolved

Yup, that's correct. That's intended. I normalized the behavior

jjonescz · 2025-12-10T10:34:15Z

src/Compilers/CSharp/Test/Emit/Emit/EmitErrorTests.cs

+            Assert.Equal("experimental-data-section-string-literals", Feature.ExperimentalDataSectionStringLiterals);
            var expectedDiagnostics = new[]
            {
                // (15,26): error CS8103: Combined length of user strings used by the program exceeds allowed limit. Try to decrease use of string literals or try the EXPERIMENTAL feature flag 'experimental-data-section-string-literals'.


Probably orthogonal to this PR, but consider adding a parameter for the feature flag name in the error message (instead of just the assert above), so we also ensure it's not getting localized. #WontFix

jjonescz · 2025-12-10T10:36:26Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableContextTests.cs

                new[] { ".cctor", "F1" };
 #endif

+            Assert.Equal("run-nullable-analysis", Feature.RunNullableAnalysis);


This assert looks unnecessary. #Resolved

jjonescz · 2025-12-10T10:39:04Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

+        internal bool HasFeature(string feature)
+        {
+            CodeAnalysis.Feature.AssertValidFeature(feature);
+            return _features.TryGetValue(feature, out var value) && value is not null;


Perhaps we could simplify this to just:

internal bool HasFeature(string feature) => Feature(feature) != null; ``` #Resolved

RikkiGibson · 2025-12-10T21:36:45Z

src/Compilers/Core/Portable/CommandLine/Feature.cs

+    internal const string InterceptorsNamespaces = "InterceptorsNamespaces";
+    internal const string NoRefSafetyRulesAttribute = "noRefSafetyRulesAttribute";
+    internal const string DisableLengthBasedSwitch = "disable-length-based-switch";
+    internal const string ExperimentalDataSectionStringLiterals = "experimental-data-section-string-literals";


Just curious, was there any specific reason for the ordering? Was this maybe the order the flags were encountered during your refactoring pass? #Resolved

Random and aesthetics :-P

RikkiGibson · 2025-12-10T21:38:15Z

src/Compilers/Core/Portable/Compilation/ParseOptions.cs

        /// <summary>
        /// Enable some experimental language features for testing.
        /// </summary>
+        // Note: use Feature entry for known feature flags.


I guess this is intentionally not included in the doc comment, since this is talking about an internal API with respect to this public API? #Resolved

Right, the known values are only internal but this API is public

RikkiGibson · 2025-12-10T21:38:50Z

Thank you!

Test plan: #81207 Only one trivial conflict due to a variable rename from #81651 (in PEMethodSymbol.cs on line 1042). Plus need to fixup the feature flag otherwise it fails on assert as an unknown feature flag due to #81591 - commit 0ae3719.

jcouv self-assigned this Dec 9, 2025

jcouv added the Area-Compilers label Dec 9, 2025

jcouv force-pushed the feature-flags branch from a816363 to 620559e Compare December 9, 2025 18:48

Centralize recognized feature flags

ce0fc60

jcouv force-pushed the feature-flags branch from 620559e to ce0fc60 Compare December 9, 2025 19:28

jcouv marked this pull request as ready for review December 9, 2025 21:04

jcouv requested a review from a team as a code owner December 9, 2025 21:04

333fred reviewed Dec 9, 2025

View reviewed changes

Revert whitespaces

6ce94c4

AlekseyTs reviewed Dec 10, 2025

View reviewed changes

Add assertion for feature flag checks

6599a89

Rename to "Feature"

9ab13a2

333fred approved these changes Dec 10, 2025

View reviewed changes

jjonescz approved these changes Dec 10, 2025

View reviewed changes

jcouv marked this pull request as draft December 10, 2025 17:17

Fix VB build

9035461

jcouv marked this pull request as ready for review December 10, 2025 20:12

Rename file to match type name

3e3a65d

jcouv enabled auto-merge (squash) December 10, 2025 21:21

RikkiGibson reviewed Dec 10, 2025

View reviewed changes

RikkiGibson approved these changes Dec 10, 2025

View reviewed changes

jcouv merged commit 7da6b9c into dotnet:main Dec 10, 2025
24 of 25 checks passed

dotnet-policy-service bot added this to the Next milestone Dec 10, 2025

dotnet-maestro bot mentioned this pull request Dec 11, 2025

[release/10.0.2xx] Source code updates from dotnet/roslyn dotnet/dotnet#3816

Merged

dotnet-bot mentioned this pull request Dec 12, 2025

[Automated] PRs inserted in VS build main-11311.198 #81672

Closed

dotnet-maestro bot mentioned this pull request Dec 18, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3957

Merged

jjonescz mentioned this pull request Dec 19, 2025

Merge branch 'main' into features/UnsafeEvolution #81768

Merged

davidwengier modified the milestones: Next, 18.3 Jan 6, 2026

davidwengier mentioned this pull request Jan 12, 2026

Update Razor and Roslyn dotnet/vscode-csharp#8877

Merged


		namespace Microsoft.CodeAnalysis;

		internal static class FeatureFlag

Conversation

jcouv commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

333fred Dec 9, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

333fred Dec 9, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 9, 2025 • 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!

AlekseyTs Dec 10, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 10, 2025

Uh oh!

jjonescz Dec 10, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 10, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 10, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 10, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Dec 10, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Dec 10, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Dec 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jcouv commented Dec 9, 2025 •

edited

Loading

333fred Dec 9, 2025 •

edited by jcouv

Loading

333fred Dec 9, 2025 •

edited by jcouv

Loading

jcouv Dec 9, 2025 •

edited

Loading

AlekseyTs Dec 10, 2025 •

edited by jcouv

Loading

jjonescz Dec 10, 2025 •

edited by jcouv

Loading

jjonescz Dec 10, 2025 •

edited by jcouv

Loading

jjonescz Dec 10, 2025 •

edited by jcouv

Loading

jjonescz Dec 10, 2025 •

edited by jcouv

Loading

RikkiGibson Dec 10, 2025 •

edited by jcouv

Loading

RikkiGibson Dec 10, 2025 •

edited by jcouv

Loading