EditorConfig comments cannot follow a property value #51625

sharwell · 2021-03-03T15:10:02Z

Related to editorconfig/specification#23 and editorconfig/specification#31

⚠️ This is a breaking change for users who previously wrote comments at the end of lines in .editorconfig. I wasn't able to find a specific example but @Youssef1313 found this one:

roslyn/src/Features/Lsif/Generator/.editorconfig

Lines 4 to 5 in 7464573

    
           dotnet_diagnostic.CA2007.severity = none # Disable warnings about ConfigureAwait, since this is a console app 
        
           dotnet_diagnostic.VSTHRD111.severity = none # Disable warnings about ConfigureAwait, since this is a console app

💭 I do notice that all cases we've found that seem to be affected involve a dotnet_diagnostic line. Since this is a compiler-controlled property interpretation, we do have the option of modifying the interpretation of that value to ignore text after a ; or #.

Related to editorconfig/specification#23 Fixes dotnet#44596

Youssef1313

~~Won't this break existing editorconfigs that used # as a comment after a value?~~ Just noticed you already stated that.

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

Youssef1313

Would you update this as well?

Youssef1313

Syntax highlighting of editorconfig in Visual Studio should reflect this.
Not sure if it belongs to this repository or it's an internal.

Also we might want VSCode to reflect this in syntax highlighting as well:

Should we file an issue to vscode repo?

Youssef1313

Needs a parallel change in roslyn-analyzers:

https://github.com/dotnet/roslyn-analyzers/blob/caacbbec9a2dff07d98ee14e99f1a8dd5d36e4f5/src/Utilities/Compiler/Options/EditorConfigParser.cs#L19

Youssef1313

Few popular repositories that are going to be affected:

sharwell · 2021-03-03T16:03:46Z

Needs a parallel change in roslyn-analyzers:

roslyn-analyzers already doesn't follow the spec, so I'd rather delete the custom support there than try to maintain it.

jmarolf

Lets not merge this until we get confirmation that this is actually the correct behavior. There seems to be some belief that this is a spec defect

jcouv · 2021-03-23T19:07:13Z

src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs

@@ -177,15 +177,18 @@ public void SpacesInProperties()
                properties);
        }

-        [Fact]
-        public void EndOfLineComments()
+        [Theory]


Please document the breaking change in https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20post%20DotNet%205.md #Closed

➡️ Documented for .NET 9

jcouv · 2021-03-23T19:12:16Z

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

+            var trimmed = value.AsSpan();
+            var commentStartIndex = trimmed.IndexOfAny(commentStartCharacters);
+            if (commentStartIndex >= 0)
+                trimmed = trimmed[0..commentStartIndex].TrimEnd();


As far as I understand, this change is to maintain the existing behavior and I assume we already have tests covering this. Is that right? #Closed

➡️ Yes, and tests have now been added.

jcouv · 2021-03-23T19:14:06Z

Just saw Jon's comment above. I'll mark this PR as draft for now.
Did we start a conversation to confirm the desired behavior?

Youssef1313 · 2023-01-01T07:24:51Z

Lets not merge this until we get confirmation that this is actually the correct behavior. There seems to be some belief that this is a spec defect

@jmarolf @jcouv @sharwell The spec is now updated editorconfig/specification#31 and it's clear what the correct behavior is.

Can this get reviewed and merged?

Youssef1313 · 2023-01-01T07:48:08Z

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

@@ -21,7 +21,7 @@ public sealed partial class AnalyzerConfig
        // Matches EditorConfig section header such as "[*.{js,py}]", see https://editorconfig.org for details
        private static readonly Regex s_sectionMatcher = new Regex(@"^\s*\[(([^#;]|\\#|\\;)+)\]\s*([#;].*)?$", RegexOptions.Compiled);
        // Matches EditorConfig property such as "indent_style = space", see https://editorconfig.org for details
-        private static readonly Regex s_propertyMatcher = new Regex(@"^\s*([\w\.\-_]+)\s*[=:]\s*(.*?)\s*([#;].*)?$", RegexOptions.Compiled);
+        private static readonly Regex s_propertyMatcher = new Regex(@"^\s*([\w\.\-_]+)\s*[=:]\s*(.*?)\s*$", RegexOptions.Compiled);


[=:]

Why are we considering both = and : to separate the key and the value? I don't think key : value should be allowed.

➡️ I have no idea, but that seems beyond the scope of this change.

AnalogFeelings · 2024-01-05T04:14:19Z

Any progress on this? This issue makes inserting the GPL license text impossible as it gets truncated.

CyrusNajmabadi · 2024-01-08T15:22:29Z

src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs

+                "refactoring" => ReportDiagnostic.Hidden,
+                "suggestion" => ReportDiagnostic.Info,
+                "warning" => ReportDiagnostic.Warn,
+                "error" => ReportDiagnostic.Error,


would prefer these just be arguments to this test method, so the switch isn't needed.

would prefer these just be arguments to this test method, so the switch isn't needed.

➡️ The current form is intentional. Arguments to the test method are input state and are used to define the unique ID of the test itself. Expected results should not be inputs, and changing an expected outcome should not be part of the test identity.

I'd agree with Cyrus here -- we normally put the expected as an input too...

we normally put the expected as an input too

➡️ I try to fix these when they occur in a method I'm working on. Test parameterization should only ever exist on the inputs, not the outputs. The entire test identity and reporting strategy (e.g. test historical pass rates, red/green developer iterations in TDD) is built around this premise.

sharwell · 2024-01-08T21:10:29Z

Few popular repositories that are going to be affected:

https://github.com/xamarin/xamarin-android/blob/aaa37c3df94490ee9befb8381f4dd7aa18226355/.editorconfig#L218

https://github.com/DotNetAnalyzers/IDisposableAnalyzers/blob/c8e3f323ff7d31d8271c9a013f1932d6f743e2de/.editorconfig#L207

https://github.com/dotnet/project-system/blob/7ab8083659d0d54722463aab3990efaf37b4d894/.editorconfig#L232

Note that none of these will be affected now that dotnet_diagnostic.*.severity parsing has been updated to ignore text that was previously a comment.

jasonmalinowski · 2024-01-19T01:52:39Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 9.md

+The compiler is updated based on the EditorConfig specification clarification in
+[editorconfig/specification#31](https://github.com/editorconfig/specification/pull/31). This changes the way values are
+passed to analyzers for lines with the following form:


The examples are great, but this might be clear too if we just say "comments now must be on their own line; comments after a value are treated as a part of the value."

➡️ Updated the text

jasonmalinowski · 2024-01-19T01:53:06Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 9.md

+
+## .editorconfig values no longer support trailing comments
+
+***Introduced in Visual Studio 2022 version [TBD]***


This is just a reminder to fix this prior to merge.

❓ Do we know what the value here is going to be?

jasonmalinowski · 2024-01-19T01:54:23Z

src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs

+                "refactoring" => ReportDiagnostic.Hidden,
+                "suggestion" => ReportDiagnostic.Info,
+                "warning" => ReportDiagnostic.Warn,
+                "error" => ReportDiagnostic.Error,


I'd agree with Cyrus here -- we normally put the expected as an input too...

jasonmalinowski · 2024-01-19T01:57:10Z

src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs

+        [InlineData("#")]
+        [InlineData(";")]


I'd consider tweaking the inline data to add another case like:

Suggested change

[InlineData("#")]

[InlineData(";")]

[InlineData("error # ignored text")]

[InlineData("error ; ignored text")]

[InlineData("warning ; ignored text")]

As otherwise this test could incorrectly pass if all parse errors were simply converted to ReportDiagnostic.Error.

➡️ Added a second line inside the test itself.

jasonmalinowski · 2024-01-19T01:59:54Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleHelpers.cs

+        /// files. For code style options maintained by Roslyn, we continue to detect and remove these "comments" for
+        /// backwards compatibility.
+        /// </summary>
+        private static readonly char[] s_commentDelimiters = ['#', ';'];


Did we want to share this code/list with the compiler code doing the same thing? The file path here makes me think we've already got shared files between both layers to use here.

➡️ The sharing here is between the workspaces and code style layers. The compiler maintains its own back-compat separately.

jasonmalinowski · 2024-01-19T02:00:35Z

Overall though I love the approach to maintain compat -- simply treating this as a break would have been far too icky for these scenarios.

ToddGrun · 2024-01-19T15:40:04Z

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

@@ -21,7 +21,7 @@ public sealed partial class AnalyzerConfig
        // Matches EditorConfig section header such as "[*.{js,py}]", see https://editorconfig.org for details
        private static readonly Regex s_sectionMatcher = new Regex(@"^\s*\[(([^#;]|\\#|\\;)+)\]\s*([#;].*)?$", RegexOptions.Compiled);
        // Matches EditorConfig property such as "indent_style = space", see https://editorconfig.org for details
-        private static readonly Regex s_propertyMatcher = new Regex(@"^\s*([\w\.\-_]+)\s*[=:]\s*(.*?)\s*([#;].*)?$", RegexOptions.Compiled);
+        private static readonly Regex s_propertyMatcher = new Regex(@"^\s*([\w\.\-_]+)\s*[=:]\s*(.*?)\s*$", RegexOptions.Compiled);


Probably outside the scope of this PR, but should we be allowing whitespace within the key?

eg:
test value = allowed

It appears that we do not currently support whitespace inside the key, but according to the specification it should be allowed:

Key: The part before the first = (trimmed of whitespace, but including any whitespace in the middle).

Can you file a new issue for this?

Delsin-Yu · 2024-05-20T08:14:25Z

Any movement on this?

raulsntos · 2024-08-24T23:43:19Z

What's the status of this PR? Since the PR documents the breaking change in .NET 9, is it safe to assume it will be merged for that release?

jcouv · 2024-09-30T21:17:34Z

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

@@ -330,33 +330,40 @@ static void freeKey(List<Section> sectionKey, ObjectPool<List<Section>> pool)

        internal static bool TryParseSeverity(string value, out ReportDiagnostic severity)
        {
+            ReadOnlySpan<char> commentStartCharacters = stackalloc char[] { ';', '#' };


nit: consider leaving a comment ("for backcompat, ...")

jcouv · 2024-09-30T21:18:44Z

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

            var comparer = StringComparer.OrdinalIgnoreCase;
-            if (comparer.Equals(value, "default"))
+            if (trimmed.Equals("default".AsSpan(), StringComparison.OrdinalIgnoreCase))


The comparer local on line above (339) no longer seems to be used

jcouv · 2024-09-30T21:21:33Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 9.md

+
+## .editorconfig values no longer support trailing comments
+
+***Introduced in Visual Studio 2022 version 17.[TBD]***


Tagging @jaredpar to advise on which release to pull this change into, since it involves a compat break. Feels like we should hold this off rather than squeeze it late in the release cycle.

We should either update this before merging, or open an issue to track updating it once we know which release it landed in.

jcouv

Compiler changes LGTM Thanks (iteration 6)

jcouv · 2024-10-16T20:03:17Z

@dotnet/roslyn-compiler for a second review. Thanks

jcouv · 2024-10-30T16:58:22Z

@dotnet/roslyn-compiler for a second review. Thanks

chsienki · 2024-10-30T19:00:32Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 9.md

+
+## .editorconfig values no longer support trailing comments
+
+***Introduced in Visual Studio 2022 version 17.[TBD]***


We should either update this before merging, or open an issue to track updating it once we know which release it landed in.

jaredpar

Putting a hold on this given it's a breaking change in editorconfig and we're trying ot discuss what paths for that look like.

EditorConfig comments cannot follow a property value

3b98df1

Related to editorconfig/specification#23 Fixes dotnet#44596

sharwell requested a review from a team as a code owner March 3, 2021 15:10

dotnet-issue-labeler bot added the Area-Compilers label Mar 3, 2021

Youssef1313 reviewed Mar 3, 2021

View reviewed changes

src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Mar 3, 2021

View reviewed changes

jmarolf suggested changes Mar 3, 2021

View reviewed changes

Update parsing of dotnet_diagnostic lines to ignore "trailing comments"

1b6c125

Base automatically changed from master to main March 3, 2021 23:53

sharwell mentioned this pull request Mar 5, 2021

CompilerVisibleProperty fails to escape special characters (# and ;) #51692

Open

jcouv reviewed Mar 23, 2021

View reviewed changes

jcouv marked this pull request as draft March 23, 2021 19:14

jcouv added the Feature - Editor Config label Mar 23, 2021

Youssef1313 mentioned this pull request Apr 8, 2021

Editor config options not populated correctly for elements with special characters #52469

Closed

Youssef1313 mentioned this pull request Dec 2, 2021

Implement Naming Styles for editorconfig UI #58075

Merged

MichaelKetting mentioned this pull request Jan 8, 2022

.editorconfig file_header_template: can't use # (or ;) symbol #44596

Open

Youssef1313 mentioned this pull request Jan 1, 2023

MSBuild properties to analyzer config break in presence of newlines #43970

Open

Youssef1313 reviewed Jan 1, 2023

View reviewed changes

Merge remote-tracking branch 'dotnet/main' into no-comment

ea18e2f

sharwell marked this pull request as ready for review January 5, 2024 21:00

sharwell requested a review from a team as a code owner January 8, 2024 14:19

CyrusNajmabadi reviewed Jan 8, 2024

View reviewed changes

jasonmalinowski reviewed Jan 19, 2024

View reviewed changes

ToddGrun reviewed Jan 19, 2024

View reviewed changes

paulloz mentioned this pull request Feb 22, 2024

Allow splitting GodotDisabledSourceGenerators by comma (Updates ExtensionMethods.cs) godotengine/godot#88682

Open

raulsntos mentioned this pull request May 16, 2024

[Source Generator] Unable to disable multiple source generators using GodotDisabledSourceGenerators property godotengine/godot#91999

Open

sharwell added 2 commits August 16, 2024 13:27

Update tests and breaking change documentation

d797c32

Update CodeStyleOption<T> parsing to ignore trailing comments

bf03a75

sharwell force-pushed the no-comment branch from b869ea5 to bf03a75 Compare August 16, 2024 18:27

dotnet-policy-service bot added the VSCode label Aug 16, 2024

Merge remote-tracking branch 'dotnet/main' into no-comment

be49e52

jcouv reviewed Sep 30, 2024

View reviewed changes

jcouv approved these changes Sep 30, 2024

View reviewed changes

jcouv self-assigned this Sep 30, 2024

chsienki approved these changes Oct 30, 2024

View reviewed changes

jaredpar requested changes Nov 21, 2024

View reviewed changes

jcouv marked this pull request as draft November 21, 2024 19:19

	dotnet_diagnostic.CA2007.severity = none # Disable warnings about ConfigureAwait, since this is a console app
	dotnet_diagnostic.VSTHRD111.severity = none # Disable warnings about ConfigureAwait, since this is a console app


		## .editorconfig values no longer support trailing comments

		*Introduced in Visual Studio 2022 version [TBD]*

-        [InlineData("#")]
-        [InlineData(";")]
+        [InlineData("error # ignored text")]
+        [InlineData("error ; ignored text")]
+        [InlineData("warning ; ignored text")]

EditorConfig comments cannot follow a property value #51625

Are you sure you want to change the base?

EditorConfig comments cannot follow a property value #51625

Conversation

sharwell commented Mar 3, 2021 • edited Loading

Youssef1313 left a comment • edited Loading

Choose a reason for hiding this comment

Youssef1313 left a comment

Choose a reason for hiding this comment

Youssef1313 left a comment • edited Loading

Choose a reason for hiding this comment

Youssef1313 left a comment

Choose a reason for hiding this comment

Youssef1313 left a comment

Choose a reason for hiding this comment

sharwell commented Mar 3, 2021

jmarolf left a comment

Choose a reason for hiding this comment

jcouv Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Mar 23, 2021

Youssef1313 commented Jan 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnalogFeelings commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

sharwell commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Jan 19, 2024

ToddGrun Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Delsin-Yu commented May 20, 2024

raulsntos commented Aug 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Oct 16, 2024

jcouv commented Oct 30, 2024

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

sharwell commented Mar 3, 2021 •

edited

Loading

Youssef1313 left a comment •

edited

Loading

Youssef1313 left a comment •

edited

Loading

jcouv Mar 23, 2021 •

edited

Loading

jcouv Mar 23, 2021 •

edited

Loading

Youssef1313 commented Jan 1, 2023 •

edited

Loading

sharwell Jan 19, 2024 •

edited

Loading

sharwell Jan 19, 2024 •

edited

Loading

ToddGrun Jan 19, 2024 •

edited

Loading