Allow msbuild to pass properties and metadata as analyzerconfig: by chsienki · Pull Request #43617 · dotnet/roslyn

chsienki · 2020-04-23T20:32:52Z

Add new target to evaluate requested properties and metadata
Add new task to turn collected evaluations into a global analyzer config
Add tests

This PR aims to answer the question of how analyzers / generators get information from MSBuild.

This implements the MSBuild -> Editorconfig portion. Consuming the generated editorconfig will be done in a different PR.

The basic idea is that an analyzer/generator can declare the properties and metadata it is interested in via an item group in a targets file (added via the usual nuget mechanism):

<ItemGroup> 
  <!-- Properties -->
  <AnalyzerProperty Include="PropertyName" />
 
  <!-- Metadata -->
  <AnalyzerItemMetadata Include="ItemType" MetadataName="MetadataName" />
</ItemGroup>

For instance, to access the TargetFramework and LangVersion properties:

<ItemGroup> 
  <AnalyzerProperty Include="TargetFramework" />
  <AnalyzerProperty Include="LangVersion" />
</ItemGroup>

Or to access the %(AccessedTime) of @(Compile) items:

<ItemGroup> 
  <AnalyzerItemMetadata Include="Compile" MetadataName="AccessedTime" />
</ItemGroup>

AnalyzerProperty items end up the in the global section of the generated config. Metadata items are placed in merged sections based on the full path of the ItemSpec, with a combination of the ItemType that produced it and the name of the metadata.

Eg:

<ItemGroup> 
  <AnalyzerProperty Include="TargetFramework" />
  <AnalyzerItemMetadata Include="Compile" MetadataName="AccessedTime" />
  <AnalyzerItemMetadata Include="AdditionalFiles" MetadataName="ModifiedTime" />
</ItemGroup>

For a project like:

<Project Sdk='Microsoft.NET.Sdk'>
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>
  
  <ItemGroup>
    <Compile Include="file1.cs" />
    <Compile Include="file2.cs" />
    <AdditionalFiles Include="file1.cs" />
  </ItemGroup>
</Project>

Would produce an editor config like:

is_global = true
msbuild_property.TargetFramework = netstandard2.0

[c:\full\path\to\file1.cs]
msbuild_item.Compile.AccessedTime = 2004-08-14 16:52:36.3168743
msbuild_item.AdditionalFiles.ModifiedTime = 2004-08-14 16:52:36.3168743

[c:\full\path\to\file2.cs]
msbuild_item.Compile.AccessedTime = 2004-08-14 16:52:36.3168743

- Add new target to evaluate requested properties and metadata - Add new task to turn collected evaluations into a global analyzer config - Add tests

chsienki · 2020-04-23T20:37:39Z

Open issue:

We use the %(FullPath) of the ItemSpec so that we don't have to worry about the location of the editorconfig. For items that don't map to an item on disk, this ends up a combination the directory the target is in and the item name.

Is that actually a problem? The retrieval mechanism in the compiler requires a file path to ask about, so it doesn't make any sense to have metadata about non-items on disk. Should we block this at the target level?

I considered filtering based on existence, but it seems possible that a file could get dropped on disk between the target and compilation so that the file would exist by the time lookup occured.

chsienki · 2020-04-23T20:39:12Z

@rainersigwald and @dsplaisted Could you check the MSBuild foo over for me? Happy to walk over it in a call or can send binary logs that show how it runs.

/cc @agocke @mavasani First half of #42219

dsplaisted

Looks good, though I'd like @rainersigwald to take a look too.

Also it would be better to have tests that exercise the logic in the .targets file too. I'm not sure how hard that is with your infrastructure.

rainersigwald · 2020-04-23T21:24:38Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+  <Target Name="GenerateMSBuildAnalyzerConfigFile"
+          BeforeTargets="BeforeCompile;CoreCompile"
+          DependsOnTargets="PrepareForBuild;CoreGenerateAssemblyInfo"
+          Condition="'$(_GeneratedAnalyzerConfigShouldRun)' == 'true'"


I don't think your ordering around this will work--did you validate that the no-items-defined case skips this target?

Generally the condition is evaluated before DependsOn or BeforeTargets run. It doesn't stop BeforeTargets from running . . . but it checks before running them. https://docs.microsoft.com/en-us/visualstudio/msbuild/target-build-order?view=vs-2019#determine-the-target-build-order

<Project> <PropertyGroup> <Condition>true</Condition> </PropertyGroup> <Target Name="entry" Condition="$(Condition) == true" /> <Target Name="before" BeforeTargets="entry"> <PropertyGroup> <Condition>false</Condition> </PropertyGroup> </Target> </Project>

$ msbuild -v:d .\foo.proj Microsoft (R) Build Engine version 16.7.0-dev-20221-01+73c84c6bd for .NET Framework Copyright (C) Microsoft Corporation. All rights reserved. Build started 4/23/2020 4:22:53 PM. 0>Process = "C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\Current\Bin\MSBuild.exe" MSBuild executable path = "C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\Current\Bin\MSBuild.exe" Command line arguments = ""C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\\MSBuild\Current\Bin\MSBuild.exe" -v:d .\foo.proj" Current directory = "S:\msbuild" MSBuild version = "16.7.0-dev-20221-01+73c84c6bd" 1>Project "S:\msbuild\foo.proj" on node 1 (default targets). 1>Building with tools version "Current". 1>Target "before" in project "S:\msbuild\foo.proj" (target "entry" depends on it): 1>Done building target "before" in project "foo.proj". 1>Target "entry" in project "S:\msbuild\foo.proj" (entry point): 1>Done building target "entry" in project "foo.proj". 1>Done Building Project "S:\msbuild\foo.proj" (default targets). Build succeeded. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:00.23

Instead, you can order via DependsOn:

<Project> <PropertyGroup> <Condition>true</Condition> </PropertyGroup> <Target Name="OrderingOnly" DependsOnTargets="SetCondition;DoWork" /> <Target Name="DoWork" Condition="$(Condition) == true" /> <Target Name="SetCondition"> <PropertyGroup> <Condition>false</Condition> </PropertyGroup> </Target> </Project>

$ msbuild -v:d .\bar.proj Microsoft (R) Build Engine version 16.7.0-dev-20221-01+73c84c6bd for .NET Framework Copyright (C) Microsoft Corporation. All rights reserved. Build started 4/23/2020 4:24:02 PM. 0>Process = "C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\Current\Bin\MSBuild.exe" MSBuild executable path = "C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\Current\Bin\MSBuild.exe" Command line arguments = ""C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\\MSBuild\Current\Bin\MSBuild.exe" -v:d .\bar.proj" Current directory = "S:\msbuild" MSBuild version = "16.7.0-dev-20221-01+73c84c6bd" 1>Project "S:\msbuild\bar.proj" on node 1 (default targets). 1>Building with tools version "Current". 1>Target "SetCondition" in project "S:\msbuild\bar.proj" (target "OrderingOnly" depends on it): 1>Done building target "SetCondition" in project "bar.proj". Target "DoWork" skipped, due to false condition; ($(Condition) == true) was evaluated as (false == true). 1>Target "OrderingOnly" in project "S:\msbuild\bar.proj" (entry point): 1>Done building target "OrderingOnly" in project "bar.proj". 1>Done Building Project "S:\msbuild\bar.proj" (default targets). Build succeeded. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:00.24

#Resolved

Interesting, I did try and it seems to work as expected, but it took a few iterations/variations. Using a root target that depends on both seems better. #Resolved

rainersigwald · 2020-04-23T21:53:01Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+      </_GeneratedAnalyzerConfigItem>
+
+      <!-- Transform the collected items into their full path -->
+      <_GeneratedAnalyzerConfigMetadata Include="@(_GeneratedAnalyzerConfigItem->'%(FullPath)')" />


Is this better than extracting FullPath inside your task? #Resolved

I was originally trying to do as much as possible outside the task, but yeah, it seems like given we're always going to run the task always anyway it would be more efficient to handle it in there. #Resolved

rainersigwald · 2020-04-23T22:11:32Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+    </ItemGroup>
+
+    <!-- Transform the collected properties and items into an editor config file -->
+    <GenerateMSBuildAnalyzerConfig


I don't love that this is unconditional, but it's pretty simple and I can't think of a better way to do it. #Resolved

- Fix target ordering - Do full path in task - Add full path tests

chsienki · 2020-04-29T23:17:24Z

@dotnet/roslyn-compiler for compiler side review please :)

jcouv · 2020-05-04T16:38:38Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+
+namespace Microsoft.CodeAnalysis.BuildTasks
+{
+    public class GenerateMSBuildAnalyzerConfig : Task


GenerateMSBuildAnalyzerConfig [](start = 17, length = 29)

I would be great to capture the documentation you wrote in OP description here. #Closed

jcouv · 2020-05-04T16:40:42Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+using Microsoft.Build.Framework;
+using Microsoft.Build.Utilities;
+
+namespace Microsoft.CodeAnalysis.BuildTasks


namespace [](start = 0, length = 9)

nit: #nullable enable? #Closed

Nullable is enabled at the project level for this assembly. Stylistically, are we still putting in source?

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

Interesting, didn't realize we'd done that. #Closed

Project-level sounds good to me. Thanks #Closed

jcouv · 2020-05-04T16:44:31Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+                builder.Append("msbuild_property.")
+                       .Append(prop.ItemSpec)
+                       .Append(" = ")
+                       .AppendLine(prop.GetMetadata("Value")); //TODO: do we need to encode/escape this?


TODO [](start = 65, length = 4)

I agree that there is an encoding/escaping question here (and other concats below). But we don't add TODO markers in the code. Some options: (1) we add encoding, or (2) add a check (fail gracefully), or (3) create a tracking issue (link here). #Closed

Good catch, I didn't notice that was still even in there. I'll write some tests to verify the behavior and either fix or open an issue.

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

Filed #43970 to track, and added a test that covers it.

In reply to: 419782382 [](ancestors = 419782382,419574615)

jcouv · 2020-05-04T16:56:51Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+    Requested properties/items are requested via item groups like:
+
+      <AnalyzerProperty Include="PropertyNameToEval" />
+      <AnalyzerItemMetadata Include="ItemType" MetadataName="MetaDataToRetrieve" />


MetaData [](start = 61, length = 8)

nit: "Metadata" #Closed

agocke · 2020-05-04T19:25:53Z

@chsienki The editorconfig spec says that all directory separators in section names must be forward slashes. I think we should abide by that, even though using absolute paths is technically illegal, just to prevent other parsers potentially falling over. #Resolved

jcouv · 2020-05-04T19:33:56Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+
+  <Target Name="GenerateMSBuildAnalyzerConfigFileShouldRun"
+          BeforeTargets="GenerateMSBuildAnalyzerConfigFile"
+          Condition="'$(Language)'=='VB' or '$(Language)'=='C#'">


Just curious, why do we have these language checks? Would F# potentially also want to benefit from generating such editorconfig files, by re-using our task and/or targets? #Closed

Hmm, good point, I'm not sure that we actually need them.

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

jcouv · 2020-05-04T19:36:35Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+  </PropertyGroup>
+
+  <Target Name="GenerateMSBuildAnalyzerConfigFileShouldRun"
+          BeforeTargets="GenerateMSBuildAnalyzerConfigFile"


BeforeTargets [](start = 10, length = 13)

Sorry if this is a basic question, but do we need this BeforeTargets given that GenerateMSBuildAnalyzerConfigFile lists this target (GenerateMSBuildAnalyzerConfigFileShouldRun) in its DependsOnTargets list already? #Resolved

Nope, just a hangover from the first commit. Fixed.

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

jcouv · 2020-05-04T19:42:40Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+    </PropertyGroup>
+  </Target>
+
+  <Target Name="GenerateMSBuildAnalyzerConfigFile"


GenerateMSBuildAnalyzerConfigFile [](start = 16, length = 33)

The name of this target seems confusing, since it's also used as a property above. Or is this dual meaning how things are typically done in MSBuild? #Closed

Yeah, its weird, but there is precedent in other targets to have a target/property named the same thing.

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

jcouv · 2020-05-04T19:57:08Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+
+      <!-- Record that we'll write a file, and add it to the editorconfig inputs -->
+      <FileWrites Include="$(GeneratedMSBuildAnalyzerConfigFile)" />
+      <EditorConfigFiles Include="$(GeneratedMSBuildAnalyzerConfigFile)"  />


/> [](start = 74, length = 2)

formatting nit: double space before #Closed

jcouv · 2020-05-04T20:02:33Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+          DependsOnTargets="PrepareForBuild;GenerateMSBuildAnalyzerConfigFileShouldRun;GenerateMSBuildAnalyzerConfigFileCore"
+          Condition="'$(Language)'=='VB' or '$(Language)'=='C#'" />
+
+  <Target Name="GenerateMSBuildAnalyzerConfigFileShouldRun">


GenerateMSBuildAnalyzerConfigFileShouldRun [](start = 16, length = 42)

It seems we have two targets named GenerateMSBuildAnalyzerConfigFileShouldRun with similar definitions. Is that intentional? Not sure how that works #Closed

Copy paste mistake. (the second one just overrides the first so works the same). Fixed.

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

jcouv · 2020-05-04T20:07:30Z

src/Compilers/Core/MSBuildTaskTests/GenerateMSBuildAnalyzerConfigTests.cs

+        [Fact]
+        public void DuplicateItemSpecsAreCombinedInSections()
+        {
+            TaskItem item1 = new TaskItem("c:\\file1.cs", new Dictionary<string, string> { { "ItemType", "Compile" }, { "MetadataName", "ToRetrieve" }, { "ToRetrieve", "abc123" } });


MetadataName [](start = 121, length = 12)

Just to confirm, "ItemType" and "MetadataName" are handled/provided by MSBuild infrastructure, so we don't have to worry about casing variations, correct? #Closed

Yes.

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

jcouv · 2020-05-04T20:10:48Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

    <EditorConfigFiles Include="@(PotentialEditorConfigFiles->Exists())" Condition="'$(DiscoverEditorConfigFiles)' != 'false'" />
  </ItemGroup>

+  <!--


Tested with a test project, lots of combinations of values and the msbuild log viewer. Basically copy the updated targets into a directory.build.targets, and change the task assembly path to the Roslyn source version. We should definitely think about investing in some sort of structured tests for these files.

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

jcouv

Done with review pass (iteration 2)

jaredpar · 2020-05-04T21:51:14Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+using Microsoft.Build.Framework;
+using Microsoft.Build.Utilities;
+
+namespace Microsoft.CodeAnalysis.BuildTasks


Interesting, didn't realize we'd done that. #Closed

jaredpar · 2020-05-04T21:51:27Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+
+namespace Microsoft.CodeAnalysis.BuildTasks
+{
+    public class GenerateMSBuildAnalyzerConfig : Task


Suggested change

public class GenerateMSBuildAnalyzerConfig : Task

public sealed class GenerateMSBuildAnalyzerConfig : Task

``` #Resolved

jaredpar · 2020-05-04T21:52:48Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+            MetadataItems = new ITaskItem[0];
+            PropertyItems = new ITaskItem[0];


Suggested change

MetadataItems = new ITaskItem[0];

PropertyItems = new ITaskItem[0];

MetadataItems = Array.Empty<ITaskItem>();

PropertyItems = Array.Empty<ITaskItem>();

``` #Resolved

jaredpar · 2020-05-04T21:54:01Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+            }
+
+            // group the metadata items by their full path
+            var groupedItems = MetadataItems.GroupBy(i => i.GetMetadata("FullPath"));


Is it possible for GetMetadata("FullPath") to return null?

My understanding is that it's 'well known metadata item' (https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-well-known-item-metadata?view=vs-2019) so will always have a value. @rainersigwald to verify that though.

There is a test ItemIsNotFullyQualifiedPath that codifies what happens when there isn't a real item on disk.

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

Yes, follows the "you asked for meaningless garbage? I'll give you string.Empty" philosophy of MSBuild.

(It's not a good philosophy. But it's the one we have.)

jaredpar · 2020-05-04T21:54:40Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+            foreach (var prop in PropertyItems)
+            {
+                builder.Append("msbuild_property.")
+                       .Append(prop.ItemSpec)


What does ItemSpec return in this context? #Resolved

The name of the actual property. ItemSpec is what's in the Include attribute. We include the properties on line 103 of Microsoft.Managed.Core.targets split out by their values.

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

chsienki · 2020-05-04T23:17:10Z

Done. The actual parsing of these is in the other PR #42219. There we treat the sections differently if it's a global config; really we're building a new thing (a global analyzer config) that just happens to look a bit like an editor config. But I agree we should make it as interoperable as possible.

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

jcouv · 2020-05-05T02:49:04Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+    /// 
+    /// Each of the <see cref="MetadataItems"/> will be transformed into a new section in the generated config file. The section
+    /// header will be the full path of the item (generated via its<see cref="ITaskItem.ItemSpec"/>), and each section will have a 
+    /// set of <c>msbuild_metadata.<em>ItemType</em>.<em>MetadataName</em> = <em>RetreivedMetadataValue</em></c>, one per <c>ItemType</c>


Retreived [](start = 81, length = 9)

nit: typo "Retrieved" #Resolved

jcouv · 2020-05-05T02:53:10Z

src/Compilers/Core/Portable/FileSystem/PathUtilities.cs

        /// in the given string with '/'. Otherwise, returns the string.
        /// </summary>
+        /// <remarks>
+        /// This method is equivalent to Microsoft.CodeAnalysis.BuildTasks.GenerateMSBuildAnalyzerConfig.NormalizeWithForwardSlash


Microsoft.CodeAnalysis.BuildTasks.GenerateMSBuildAnalyzerConfig.NormalizeWithForwardSlash [](start = 41, length = 89)

nit: consider making it a link (<see I think). Same on other end #WontFix

You can't sadly, because they don't reference each other (why we have to copy). It issues a warning that it can't be resolved.

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

jcouv

LGTM Thanks (iteration 3) with couple nits

jaredpar · 2020-05-05T03:26:32Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+                    builder.Append("msbuild_item.")
+                           .Append(item.GetMetadata("ItemType"))
+                           .Append(".")
+                           .Append(metadataName)


Will metadataName always be a valid identifier for a editor config value name here? #Resolved

Yes. Editorconfig keys are opaque strings; the only disallowed characters are whitespace and '=' (see https://editorconfig-specification.readthedocs.io/en/latest/#id3).

metadataName is the value of either an xml element or attribute name (note: the name, not it's value). The XML spec disallows '=' or ' ' as identifier characters (https://www.w3.org/TR/2008/REC-xml-20081126/#NT-Name) which means it will always be valid.

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

jaredpar · 2020-05-05T03:42:14Z

src/Compilers/Core/MSBuildTaskTests/GenerateMSBuildAnalyzerConfigTests.cs

+            Assert.Equal(@"is_global = true
+
+[c:/file1.cs]
+msbuild_item.. = 


Is this legal editor config? #Resolved

Yes. It's just a string. It's semantically uninteresting to us, but valid.

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

jaredpar · 2020-05-05T03:43:26Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+    /// The Microsoft.Managed.Core.targets calls this task with the collected results of the <c>AnalyzerProperty</c> and 
+    /// <c>AnalyzerItemMetadata</c> item groups. 
+    /// </remarks>
+    public sealed class GenerateMSBuildAnalyzerConfig : Task


One part I'm unclear on is what is the responsibility of this type with respect to generating a well formed / correct editorconfig file? Is the expectation that this Task is best effort, guaranteed to produce correct editor config, etc ...?

It should be a valid config, although there is the newline bug that means the files it produces can be incorrect. Correct parsing of editorconfig is supposed to just ignore lines it can't understand though, so even then, it'll be valid you just can't read the whole property.

Want me to add something to the task description that explains it?

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

Possibly add a remarks section essentially noting that there will always be cases where we can produce invalid editorconfig files. We can definitely get the 99% case covered but I'm sure there are ways that developers could do evil things to produce an invalid ones.

Another question, @jasonmalinowski brought this up in chat. When the editor config is invalid how will the customer find out? Will there just be an error message in the diagonstics for example?

An editorconfig parsed is supposed to ignore any lines it doesn't understand when parsing: I've checked and we can handle a malformed config; we just ignore anything we can't parse. Of course that isn't particularly helpful for the user to understand when things aren't working. We 'blessed' the diagnostic options as having warnings when we don't understand them, and I expect we'll do the same for the msbuild properties generated here. The task should always generate something we can recognize (msbuild_property / msbuild_metadata) so when it comes to parsing those (another PR) will probably issue warnings for things we can't understand.

In reply to: 420302735 [](ancestors = 420302735,419847995)

An editorconfig parsed is supposed to ignore any lines it doesn't understand when parsing: I've checked and we can handle a malformed config; we just ignore anything we can't parse. Of course that isn't particularly helpful for the user to understand when things aren't working. We 'blessed' the diagnostic options as having warnings when we don't understand them, and I expect we'll do the same for the msbuild properties generated here. The task should always generate something we can recognize (msbuild_property / msbuild_metadata) so when it comes to parsing those (another PR) will probably issue warnings for things we can't understand.

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

jaredpar · 2020-05-05T03:44:54Z

src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets

+    <GeneratedMSBuildAnalyzerConfigFile Condition="'$(GeneratedMSBuildAnalyzerConfigFile)' ==''">$(IntermediateOutputPath)$(MSBuildProjectName).GeneratedMSBuildAnalyzerConfig.editorconfig</GeneratedMSBuildAnalyzerConfigFile>
+    <GenerateMSBuildAnalyzerConfigFile Condition="'$(GenerateMSBuildAnalyzerConfigFile)' == ''">true</GenerateMSBuildAnalyzerConfigFile>


Suggested change

<GeneratedMSBuildAnalyzerConfigFile Condition="'$(GeneratedMSBuildAnalyzerConfigFile)' ==''">$(IntermediateOutputPath)$(MSBuildProjectName).GeneratedMSBuildAnalyzerConfig.editorconfig</GeneratedMSBuildAnalyzerConfigFile>

<GenerateMSBuildAnalyzerConfigFile Condition="'$(GenerateMSBuildAnalyzerConfigFile)' == ''">true</GenerateMSBuildAnalyzerConfigFile>

<GeneratedMSBuildAnalyzerConfigFile Condition="'$(GeneratedMSBuildAnalyzerConfigFile)' == ''">$(IntermediateOutputPath)$(MSBuildProjectName).GeneratedMSBuildAnalyzerConfig.editorconfig</GeneratedMSBuildAnalyzerConfigFile>

<GenerateMSBuildAnalyzerConfigFile Condition="'$(GenerateMSBuildAnalyzerConfigFile)' ==

''">true</GenerateMSBuildAnalyzerConfigFile>

Sorry, that was driving me crazy. #Resolved

jaredpar · 2020-05-05T03:49:42Z

src/Compilers/Core/MSBuildTaskTests/GenerateMSBuildAnalyzerConfigTests.cs

+[{Path.GetFullPath("..\\file2.cs").Replace('\\', '/')}]
+msbuild_item.Compile.ToRetrieve = abc123
+
+[{Path.GetFullPath("someDir\\otherDir\\thirdDir\\..\\file3.cs").Replace('\\', '/')}]


How is this test functioning? I can't see where these files are written to disk hence can't see how this not throwing. #Resolved

It's not writing anything to disk. MSBuild takes the value of the ItemSpec and appends the Working Directory to it when you ask for the full path. The tests here are just doing that same calculation so it works without having to hard code absolute paths.

I'll update the test to make it a little clearer what's going on.

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

jaredpar · 2020-05-05T19:24:14Z

src/Compilers/Core/MSBuildTask/GenerateMSBuildAnalyzerConfig.cs

+    /// The Microsoft.Managed.Core.targets calls this task with the collected results of the <c>AnalyzerProperty</c> and 
+    /// <c>AnalyzerItemMetadata</c> item groups. 
+    /// </remarks>
+    public sealed class GenerateMSBuildAnalyzerConfig : Task


Possibly add a remarks section essentially noting that there will always be cases where we can produce invalid editorconfig files. We can definitely get the 99% case covered but I'm sure there are ways that developers could do evil things to produce an invalid ones.

Another question, @jasonmalinowski brought this up in chat. When the editor config is invalid how will the customer find out? Will there just be an error message in the diagonstics for example?

* upstream/master: (495 commits) Simplify PEPropertySymbol constructor (dotnet#43990) Remove duplicate calls to ThrowIfCancellationRequested Allow msbuild to pass properties and metadata as analyzerconfig: (dotnet#43617) Use a private exception type for error types encountered instead of ArgumentException. Remove unnecessary check Code review feedback Adding tests and expanding side effect cases Expose specific members of System.[U]IntPtr from native integer types (dotnet#43766) Remove trailing space from BoundDecisionDagNode Don't cache FileInfo instances. Add 16.7 Preview 1 to the publish data config Bump prerelease version for 16.7 preview 2 Only run code style analyzers when nullable warnings are enabled Remove Code review feedback Lint Undo Fix regression in global:: qualified constant in a switch case. Fixes dotnet#43960 Undo Simplify ...

Allow msbuild to pass properties and metadata as analyerconfig:

3197bfe

- Add new target to evaluate requested properties and metadata - Add new task to turn collected evaluations into a global analyzer config - Add tests

chsienki requested a review from a team as a code owner April 23, 2020 20:32

chsienki added Area-Compilers Feature - Source Generators Source Generators labels Apr 23, 2020

dsplaisted approved these changes Apr 23, 2020

View reviewed changes

mavasani mentioned this pull request Apr 23, 2020

Replace String.IndexOf() == -1 by !String.Contains dotnet/runtime#35350

Closed

rainersigwald reviewed Apr 23, 2020

View reviewed changes

PR Feedback:

8d39857

- Fix target ordering - Do full path in task - Add full path tests

rainersigwald approved these changes Apr 28, 2020

View reviewed changes

jcouv reviewed May 4, 2020

View reviewed changes

jaredpar reviewed May 4, 2020

View reviewed changes

PR Feedback

3c7b2ca

jcouv reviewed May 5, 2020

View reviewed changes

jcouv approved these changes May 5, 2020

View reviewed changes

jcouv self-assigned this May 5, 2020

jaredpar reviewed May 5, 2020

View reviewed changes

PR Feedback

27d60a1

jaredpar approved these changes May 5, 2020

View reviewed changes

chsienki merged commit 0853fde into dotnet:master May 6, 2020

ghost added this to the Next milestone May 6, 2020

chsienki deleted the msbuild_to_editorconfig branch May 6, 2020 01:33

kzu mentioned this pull request May 9, 2020

Source Generators: design-time completion/intellisense is never fixed #44093

Closed

chsienki mentioned this pull request May 12, 2020

XAML: Provide access to project properties and capabilities #44133

Closed

JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020

Bouke mentioned this pull request Oct 8, 2020

GeneratedMSBuildEditorConfig.editorconfig added to project root #48430

Closed

vsfeedback mentioned this pull request Oct 21, 2020

GeneratedMSBuildEditorConfig.editorconfig added to project root #48826

Closed

chsienki restored the msbuild_to_editorconfig branch April 1, 2021 18:45

	public class GenerateMSBuildAnalyzerConfig : Task
	public sealed class GenerateMSBuildAnalyzerConfig : Task
	``` #Resolved

		MetadataItems = new ITaskItem[0];
		PropertyItems = new ITaskItem[0];

-            MetadataItems = new ITaskItem[0];
-            PropertyItems = new ITaskItem[0];
+            MetadataItems = Array.Empty<ITaskItem>();
+            PropertyItems = Array.Empty<ITaskItem>();
+``` #Resolved

		<GeneratedMSBuildAnalyzerConfigFile Condition="'$(GeneratedMSBuildAnalyzerConfigFile)' ==''">$(IntermediateOutputPath)$(MSBuildProjectName).GeneratedMSBuildAnalyzerConfig.editorconfig</GeneratedMSBuildAnalyzerConfigFile>
		<GenerateMSBuildAnalyzerConfigFile Condition="'$(GenerateMSBuildAnalyzerConfigFile)' == ''">true</GenerateMSBuildAnalyzerConfigFile>

Conversation

chsienki commented Apr 23, 2020

Uh oh!

chsienki commented Apr 23, 2020

Uh oh!

chsienki commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

rainersigwald Apr 23, 2020 • 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!

chsienki Apr 23, 2020 • 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!

rainersigwald Apr 23, 2020 • edited by chsienki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki Apr 23, 2020 • 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!

rainersigwald Apr 23, 2020 • edited by chsienki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki commented Apr 29, 2020

Uh oh!

jcouv May 4, 2020 • 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!

jcouv May 4, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

jaredpar May 4, 2020 • 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 May 5, 2020 • 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!

jcouv May 4, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 4, 2020 • 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!

agocke commented May 4, 2020 • edited by chsienki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv May 4, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv May 4, 2020 • edited by chsienki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

chsienki commented Apr 23, 2020 •

edited

Loading

rainersigwald Apr 23, 2020 •

edited

Loading

chsienki Apr 23, 2020 •

edited

Loading

rainersigwald Apr 23, 2020 •

edited by chsienki

Loading

chsienki Apr 23, 2020 •

edited

Loading

rainersigwald Apr 23, 2020 •

edited by chsienki

Loading

jcouv May 4, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited

Loading

jaredpar May 4, 2020 •

edited by jcouv

Loading

jcouv May 5, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited

Loading

agocke commented May 4, 2020 •

edited by chsienki

Loading

jcouv May 4, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited by chsienki

Loading

jcouv May 4, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited

Loading

jcouv May 4, 2020 •

edited

Loading

jaredpar May 4, 2020 •

edited by jcouv

Loading

jaredpar May 4, 2020 •

edited by chsienki

Loading

jaredpar May 4, 2020 •

edited by chsienki

Loading

jaredpar May 4, 2020 •

edited by chsienki

Loading