Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analyzers should be able to specify additional files without using MSBuild #5096

Open
jasonmalinowski opened this issue Sep 9, 2015 · 14 comments

Comments

@jasonmalinowski
Copy link
Member

Right now the only supported way for an analyzer to specify it needs an additional file is to add it to the AdditionalFileItems ItemGroup. There are several problems with this approach:

  1. If you have your NuGet package being consumed by non-MSBuild (say, DNX) there's nowhere you can say you need a file.
  2. If you are deploying your analyzer with a VSIX, you can't modify the project files at all.
  3. It requires people consuming the analyzer to know how to specify the item group of their items, which might not be possible.
  4. Code fixes that want their own file to control settings (see Solution.AddAdditionalDocument does not add a new file with AdditionalFiles item type #4655 for a good example), run into weird issues where it's not clear how the project file should be updated when they want to add an additional file. If the analyzer could specify it's dependency, we punt the entire question of what item group to add the items with.

If there was metadata on the analyzer itself -- consider a .NET attribute that includes a file name glob -- then we could avoid many of these issues. Bugs #4655 and #4425 would go away by virtue, since we have a better system in place.

@ghost ghost added this to the Unknown milestone Sep 9, 2015
@sharwell
Copy link
Member

sharwell commented Sep 9, 2015

I thought the item type was AdditionalFiles. Does AdditionalFileItems have some other meaning or is that a typo?

@sharwell
Copy link
Member

sharwell commented Sep 9, 2015

📝 Also note that AdditionalFiles is not provided by an AvailableItemName anywhere in the build, so you cannot set this value in the Properties window in Visual Studio. You have to unload the project and hand-edit it to the correct value.

@jasonmalinowski
Copy link
Member Author

Well, there's really three ways you can set it:

<ItemGroup Condition="('$(AdditionalFileItemNames)' != '')">
  <AdditionalFileItems Include="$(AdditionalFileItemNames)" />
  <AdditionalFiles Include="@(%(AdditionalFileItems.Identity))" />
</ItemGroup>

You can add a file directly into AdditionalItems, add the name of another item group into AdditionalFileItems, or include that in the property AdditionalFileItemNames.

@tmeschter
Copy link
Contributor

@jasonmalinowski I'm not sure how that would work with the command-line compilers. The compiler would crack the attribute to get the file name glob and then... do what with it? Where would it look for the file? Absolute paths are obviously out, there's no concept of a "project directory" to serve as the root for a search, and you can't assume that the additional file will be at a known location relative to the analyzer assembly.

Not to mention that this would screw with incremental builds as you now have an untracked dependency.

We can solve most, if not all, of the listed problems without adopting a second scheme for specifying additional files.

Speaking of which, @sharwell can you file an issue about AdditionalFiles not being provided by an AvailableItemName? Having to unload projects is painful and that should be an easy fix.

@sharwell
Copy link
Member

sharwell commented Sep 9, 2015

@tmeschter Created #5104.

@jasonmalinowski
Copy link
Member Author

@tmeschter:I admit the project root flag is a problem, but I don't see it as avoidable in our ecosystem today. At least I don't see an alternative for DNX. We already have other needs (path-relative PDBs so we can be deterministic) where we will probably need a project root concept. Maybe @tmat already has a different plan for that.

Not to mention that this would screw with incremental builds as you now have an untracked dependency.

If I was implementing this, my plan would be to split compilation into a cracking of the analyzer to get the paths with a first MSBuild task, and then that being passed to Csc/Vbc tasks. Alternative hosts could do their own thing. Incremental compilation is still fine. Consider that the C#/VB compilers already have "untracked dependencies" in the form of XML doc comment include files, and msbuild already has these issues around .resx or .rc files. @agocke apparently had fun implementing file tracker compat in MSBuild to solve other nasty issues here.

@jasonmalinowski
Copy link
Member Author

Also chatting with @tmat asking what his plan was about PDBs, we realized that since the task would have to exist that gets the file name globs back to MSBuild, then MSBuild could concatenate the project directory and still pass the explicit names to csc today, thereby avoiding the whole project root problem. DNX could read the metadata, do their own concatenation, and pass that along to the compilation.

@tmeschter
Copy link
Contributor

@jasonmalinowski OK, I can see how MSBuild cracking the data and then passing it to the compiler would solve a number of these issues, though I dislike the idea of certain analyzer attributes communicating data to the compiler and other analyzer attributes communicating data to the build system.

But if we're willing to change DNX to support reading the metadata, why don't we just change DNX to support additional files in the first place?

Or, what if we took it a step further, and created a convention where the file name globs are stored somewhere in the analyzer's NuGet package? MSBuild and DNX would need to know how to translate those file globs into actual file paths and pass them to the compiler, but it avoids my above objection, and probably provides a better experience than we currently have with MSBuild (that is, having to craft a .targets file that finds and injects the AdditionalItems).

@jasonmalinowski
Copy link
Member Author

But if we're willing to change DNX to support reading the metadata, why don't we just change DNX to support additional files in the first place?

The problem is not that DNX can or can't support additional files per se. The problem is that our current encoding of wanted additional files is encoded in a file format (MSBuild) that's unreadable.

I dislike the idea of certain analyzer attributes communicating data to the compiler and other analyzer attributes communicating data to the build system

Why is this bad? It seems completely natural to me. The implementer of an analyzer doesn't care about the implementation of the compiler/build system, after all. Yeah, it's weird for us but I hope elegant for them.

Or, what if we took it a step further, and created a convention where the file name globs are stored somewhere in the analyzer's NuGet package?

That would work. But given .NET already has a system for metadata, I don't see why we don't just use it. The file convention is still broken if somebody just wants to send you over a DLL with an analyzer inside of it. I agree that we want to encourage NuGet to distribute analyzers as much as possible, but in my book it's always best to let a technology work without implicit dependencies on the environment. Going to a NuGet convention means we get to revisit this again the moment we need a scenario where additional files must work outside NuGet. By putting the metadata into the assembly itself then we remove the implicit dependency that there's a file system these things are being loaded from in the first place.

@sodeshpa
Copy link

Hello,, Any update on this issue. Is there anyway we can scan .config, .aspx, ascx, asax files withoit specifying in additionalfile in the csproj of every project in the solution.

Thanks

@jmarolf
Copy link
Contributor

jmarolf commented Apr 12, 2019

@sodeshpa on newer version of Visual Studio you can use a Directory.Build.props file so you only need to specify the additional file in one place and it is added to all of your projects automatically.

@tmeschter
Copy link
Contributor

@sodeshpa What are you trying to do? Write an analyzer that will examine the .config, .aspx, etc. files that are part of the project?

How are you planning on distributing this analyzer?

@nschuessler
Copy link
Contributor

nschuessler commented Nov 6, 2021

@sharwell @jasonmalinowski This thread is now 6 years old. Is it possible to do a small fix that doesn't require solving world hunger?

WithAdditionalFile method currently works for adding a file, but it always assumes Content can we have an overload that allows specifying an enum which has the values that are in the IDE dropdown:

public enum AdditionalFileType
{
      None = 0,
      Content = 1,
      EmbeddedResource = 2,
      AnalyzerAdditionalFile = 3,
      ...
}

public class Solution
{
       Solution AddAdditionalDocument(
           DocumentId documentId,
           string name,
           string text,
            ...,
            AdditionalFileType fileType = AdditionalFileType.Content)
}

@sharwell
Copy link
Member

sharwell commented Nov 8, 2021

WithAdditionalFile method currently works for adding a file, but it always assumes Content can we have an overload that allows specifying an enum which has the values that are in the IDE dropdown:

This is filed separately as #4655.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants