Wire through editorconfig #27232

agocke · 2018-05-29T22:15:07Z

This PR provides a new command line compiler option: "/analyzerconfig:" that
takes a set of "analyzer config" files (supersets of EditorConfig files)
that the compiler will consider for diagnostic configuration.

jaredpar · 2018-05-30T00:44:40Z

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

@@ -244,6 +246,231 @@ internal SourceText TryReadFileContent(CommandLineSourceFile file, IList<Diagnos
            }
        }

+        internal static ImmutableArray<ImmutableDictionary<string, ReportDiagnostic>> GetAnalyzerConfigOptions(
+            IReadOnlyList<string> sourcePaths,


Why IReadOnlyList<string> here? Next method takes an ImmutableArray<string> for a paths parameter. #Resolved

This one is used for testing and I pass in an array instead of an ImmutableArray because the syntax is shorter and clearer. #Resolved

jaredpar · 2018-05-30T00:47:08Z

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

+                        diagId = diagId.Substring(0, diagId.Length - DiagnosticOptionSuffix.Length);
+
+                        ReportDiagnostic? severity = null;
+                        switch (CaseInsensitiveComparison.ToLower(value))


switch (CaseInsensitiveComparison.ToLower(value)) [](start = 23, length = 50)

How often are we going to be parsing this? Seems like we're creating an extra allocation per property here. Know it's annoying but may want to consider wiriting out this if by hand.

jaredpar · 2018-05-30T00:47:50Z

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

+                        key.EndsWith(DiagnosticOptionSuffix, StringComparison.Ordinal))
+                    {
+                        string diagId = key.Substring(DiagnosticOptionPrefix.Length);
+                        diagId = diagId.Substring(0, diagId.Length - DiagnosticOptionSuffix.Length);


Can't this be done as a single Substring operation?

jaredpar · 2018-05-30T00:50:31Z

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

+
+            readEditorConfigFiles(analyzerConfigPaths, analyzerConfigs);
+
+            if (diagnostics.HasAnyErrors())


Shouldn't this check be done first? The code above has no impact on the content of this DiagnosticBag instance.

Ah I see now. That's really subtle as you capture some of the context (diagnostics) but pass some explicitly (analyzerConfigs). I think you should be consistent here and do one or the other.

In reply to: 191616102 [](ancestors = 191616102)

jaredpar · 2018-05-30T00:53:05Z

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

+                    string fileContent = TryReadFileContent(configPath, diagnostics, out string normalizedPath);
+                    if (fileContent is null)
+                    {
+                        // Error reading a file. Bail out and report error.


Don't see how an error gets reported here.

jaredpar · 2018-05-30T00:54:58Z

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

+
+                    processedDirs.Add(directory);
+                    var forwardPath = PathWithForwardSlashSeparators(directory);
+                    var editorConfig = EditorConfig.Parse(fileContent, forwardPath);


How do errors get reported on this path?

There are never any parse errors for EditorConfig files. Any invalid sections are skipped and no errors are reported.

agocke · 2018-05-31T22:10:02Z

test windows_debug_spanish_unit32_prtest please

Provides a new command line compiler option: "/analyzerconfig:" that takes a set of "analyzer config" files (supersets of EditorConfig files) that the compiler will consider for diagnostic configuration.

gafter · 2018-06-07T00:30:01Z

This PR is dependent on #27119 and contains commits from that PR.

I think this is no longer true? #Resolved

agocke · 2018-06-07T00:54:06Z

Yup! Merged commits have been removed to assist reviewing

agocke · 2018-06-07T18:12:08Z

@jaredpar Any more comments here?

jaredpar · 2018-06-07T19:54:34Z

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

+                (e1, e2) => e1.Directory.Length.CompareTo(e2.Directory.Length)));
+
+            var allDiagnosticOptions =
+                ArrayBuilder<ImmutableDictionary<string, ReportDiagnostic>>.GetInstance(sourcePaths.Count);


Even if the intent is ordinal comparison consider using an explicit comparer here. This makes it clear to later developers that the scenario was considered.

I think you're thinking of the ImmutableDictionary. This is an ImmutableArray of ImmutableDictionaries, so the only comparer I expect to work here is reference comparison. The later creation of the ImmutableDictionaries should have explicit comparers.

gafter · 2018-06-07T20:20:23Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5339,4 +5339,13 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_OutVariableCannotBeByRef" xml:space="preserve">
    <value>An out variable cannot be declared as a ref local</value>
  </data>
+  <data name="WRN_InvalidSeverityInAnalyzerConfig" xml:space="preserve">
+    <value>The diagnostic '{0}' was given an invalid severity '{1}' in analyzer config file.</value>


in analyzer config file [](start = 68, length = 23)

"in analyzer config file"? In THE analyzer config file? In AN analyzer config file? Can we identify the offending config file? I presume this message identifies the offending file and line.

Unfortunately the parser does not hold enough information to produce an exact location. I've changed the method to include the directory of the editorconfig file, which should be unambiguous since there is only more than one in a rare error scenario. However, I think producing the full path wouldn't be too hard, it's just that I've refactored this API slightly in a later PR so I'd prefer to fix it later. I've filed https://github.com/dotnet/roslyn/projects/18#card-10413696 to track this issue.

gafter · 2018-06-07T20:21:20Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+    <value>Invalid severity in analyzer config file.</value>
+  </data>
+  <data name="ERR_MultipleAnalyzerConfigsInSameDir" xml:space="preserve">
+    <value>Multiple analyzer configs cannot be in the same directory ('{0}').</value>


analyzer configs [](start = 20, length = 16)

... analyzer config files cannot ...

jaredpar

gafter · 2018-06-07T21:46:00Z

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

+        /// A set of paths to EditorConfig-compatible analyzer config files.
+        /// </summary>
+        public ImmutableArray<string> AnalyzerConfigPaths { get; internal set; }
+


This is confusing. Why do we call them "paths" instead of a filenames? Why do we use string instead of CommandLineSourceFile like we do for AdditionalFiles and EmbeddedFiles?

Perhaps this is a set of paths to directories containing editor config files?

In reply to: 193898640 [](ancestors = 193898640)

A file name usually just refers to the name of the file, i.e. for a file /a/b/c.cs the file name would be c.cs, while the path would be /a/b/c.cs. Right now these happen to be absolute paths, but I didn't want to guarantee that in the API.

As to why I didn't choose to use CommandLineSourceFile -- I'm not sure why we did choose that for AdditionalFiles and EmbeddedFiles. CommandLineSourceFile is a struct with two entries: Path, which is the same as the string here, and IsScript which is a bool that is always false, since by definition only source files can be script files. I have a feeling that the helper for parsing separated paths happened to return CommandLineSourceFiles and these additions couldn't be bothered to convert. Otherwise I see no reason for them just to be paths.

gafter · 2018-06-07T21:57:03Z

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

+
+                        ReportDiagnostic? severity;
+                        if (CaseInsensitiveComparison.Equals(value, "default"))
+                        {


This can be done using a switch on value.ToLowerInvariant(). Since we're looking at ASCII, no fancy Unicode-correct case insensitivity is needed.

Why do we require an exact match for the prefix and suffix of the key, but compare the middle part using relaxed case?

I've changed this to use OrdinalIgnoreCase. Jared suggested not calling ToLower or ToUpper because it will allocate an additional string for each property. The if is more verbose, but avoids that.

As to the mixed casing -- the key is always lower-cased by the EditorConfig parser, while the value is case-preserving, so we compare the value case-insensitively, but the key case-sensitively.

gafter · 2018-06-07T21:57:22Z

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

@@ -244,6 +246,233 @@ internal SourceText TryReadFileContent(CommandLineSourceFile file, IList<Diagnos
            }
        }

+        internal static ImmutableArray<ImmutableDictionary<string, ReportDiagnostic>> GetAnalyzerConfigOptions(


GetAnalyzerConfigOptions [](start = 86, length = 24)

I lack context to understand what this method is supposed to do. What are the arguments? Can you add a doc comment please?

gafter · 2018-06-07T22:01:41Z

Is there a design doc that will help me know what this PR is trying to do?

agocke · 2018-06-07T22:41:35Z

@gafter Great point. I will write up a doc to accompany the feature. It will follow closely what's described on this page: https://editorconfig.org/

gafter · 2018-06-08T02:39:28Z

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

@@ -65,8 +71,7 @@ private EditorConfig(Section globalSection, ImmutableArray<Section> namedSection
        /// <summary>
        /// Gets whether this editorconfig is a the topmost editorconfig.
        /// </summary>
-        public bool IsRoot
-            => GlobalSection.Properties.TryGetValue("root", out string val) && val == "true";
+        public bool IsRoot => GlobalSection.Properties.TryGetValue("root", out string val) && val == "true";



If the topmost editorconfig file in the project lacks this property, it should still be considered the root. How do we handle that?

This comment is bad. Root should mean that no properties from higher editorconfigs should apply, not necessarily that there couldn't be editorconfigs higher in the directory tree. This work is tracked at #27644

gafter

agocke added the Area-Compilers label May 29, 2018

agocke added this to the 15.8 milestone May 29, 2018

agocke requested a review from jaredpar May 29, 2018 22:15

agocke requested a review from a team as a code owner May 29, 2018 22:15

agocke force-pushed the wire-through-editorconfig branch from 90a2ddd to d09083f Compare May 29, 2018 22:59

jaredpar reviewed May 30, 2018

View reviewed changes

agocke closed this May 31, 2018

agocke reopened this May 31, 2018

agocke requested a review from a team May 31, 2018 22:11

This was referenced Jun 4, 2018

Update EditorConfig section matcher to reflect special '/' handling #27396

Merged

Add analyzer options threading #27397

Merged

Add build task #27398

Merged

agocke added 7 commits June 5, 2018 17:48

Remove EditorConfig.Combine. The results are incorrect and useless

15ea9ff

Wire-through EditorConfig support to command line

6a0caf1

Provides a new command line compiler option: "/analyzerconfig:" that takes a set of "analyzer config" files (supersets of EditorConfig files) that the compiler will consider for diagnostic configuration.

Remove unused local function

ef74eca

Replace analyzer config CommandLineSourceFile with paths

a76738c

Satisfy the requirements for adding a new compiler warning

5e786ff

Respond to PR comments

efd1e24

Set a culture in the tests to make them portable

8df6338

agocke force-pushed the wire-through-editorconfig branch from 47750e8 to 8df6338 Compare June 6, 2018 01:23

agocke closed this Jun 6, 2018

agocke reopened this Jun 6, 2018

gafter self-assigned this Jun 7, 2018

gafter self-requested a review June 7, 2018 00:30

jaredpar reviewed Jun 7, 2018

View reviewed changes

gafter reviewed Jun 7, 2018

View reviewed changes

jaredpar approved these changes Jun 7, 2018

View reviewed changes

gafter reviewed Jun 7, 2018

View reviewed changes

agocke added 2 commits June 7, 2018 16:14

Add design document for compiler AnalyzerConfig support

60965bd

Add doc comments and respond to PR feedback

8145651

gafter reviewed Jun 8, 2018

View reviewed changes

gafter approved these changes Jun 8, 2018

View reviewed changes

agocke added 2 commits June 8, 2018 00:21

Report analyzer config path in error message

41f93bf

Update error message text

303c4c4

agocke merged commit b7607c3 into dotnet:features/editorconfig-in-compiler Jun 8, 2018

agocke deleted the wire-through-editorconfig branch June 8, 2018 20:02

agocke mentioned this pull request Jun 14, 2018

Flow the list of .editorconfig files through the compiler front end #18210

Closed

agocke removed this from the 15.8 milestone Jun 14, 2018

agocke mentioned this pull request Dec 13, 2018

Code to generate options from EditorConfig files should clear the options if one of the EditorConfig files it sees is the root (to prevent getting options from files above the root). #27644

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wire through editorconfig #27232

Wire through editorconfig #27232

agocke commented May 29, 2018 •

edited

Loading

jaredpar May 30, 2018 •

edited

Loading

agocke May 30, 2018 •

edited by jaredpar

Loading

jaredpar May 30, 2018

jaredpar May 30, 2018

jaredpar May 30, 2018

jaredpar May 30, 2018

jaredpar May 30, 2018

jaredpar May 30, 2018

agocke May 30, 2018

agocke commented May 31, 2018

gafter commented Jun 7, 2018 •

edited

Loading

agocke commented Jun 7, 2018

agocke commented Jun 7, 2018

jaredpar Jun 7, 2018

agocke Jun 8, 2018

gafter Jun 7, 2018 •

edited

Loading

agocke Jun 8, 2018

gafter Jun 7, 2018

jaredpar left a comment

gafter Jun 7, 2018

gafter Jun 7, 2018

agocke Jun 8, 2018

gafter Jun 7, 2018

agocke Jun 8, 2018

gafter Jun 7, 2018

gafter commented Jun 7, 2018

agocke commented Jun 7, 2018

gafter Jun 8, 2018

agocke Jun 8, 2018

gafter left a comment


		readEditorConfigFiles(analyzerConfigPaths, analyzerConfigs);

		if (diagnostics.HasAnyErrors())

Wire through editorconfig #27232

Wire through editorconfig #27232

Conversation

agocke commented May 29, 2018 • edited Loading

jaredpar May 30, 2018 • edited Loading

Choose a reason for hiding this comment

agocke May 30, 2018 • edited by jaredpar Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented May 31, 2018

gafter commented Jun 7, 2018 • edited Loading

agocke commented Jun 7, 2018

agocke commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Jun 7, 2018

agocke commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

agocke commented May 29, 2018 •

edited

Loading

jaredpar May 30, 2018 •

edited

Loading

agocke May 30, 2018 •

edited by jaredpar

Loading

gafter commented Jun 7, 2018 •

edited

Loading

gafter Jun 7, 2018 •

edited

Loading