[spec] Indicate analyzer assets and respect analyzer filtering in project.assets.json #14455
Conversation
kfikadu
changed the title
There was a problem hiding this comment.
This is looking great.
I may have left some feedback on the spec, but general direction seems fine.
Please go through the feedback and lmk if you have any questions.
Even if this spec is not fully merged yet, I think you can start working on the implementation
| } | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I don't think this happens today, but we might want to talk about if there are any changes to the deps file, or to runtime compilation scenarios that consume nuget packages - like interactive. Could also be out of scope or handled in separate doc.
There was a problem hiding this comment.
After some conversation, we came to the conclusion that if a package is only providing analyzers, then it should not affect the deps.json file
For cases like interactive, do you mean C# interactive?
There was a problem hiding this comment.
Yeah - C# interactive was just an example. Jupyter notebooks is another. I was trying to think if there was anything else that tried to resolve packages or consume the project.assets.json that might need to change. I don't expect the design to change as a result of those, but it might be good to capture a list of things that might change when project.assets.json changes and inform those folks as part of this, so that they might make changes to react if necessary.
| ### Functional explanation | ||
|
|
||
| Enable analyzer assets to respect asset filtering options like other asset types. | ||
| When enabled via `<RestoreEnableAnalyzerAssets>true</RestoreEnableAnalyzerAssets>`: |
There was a problem hiding this comment.
We might need to or the valuies of this property across projects tfms.
Basically, if one of them is true, then it's enabled, but if they're all empty or false, then it's disabled.
There was a problem hiding this comment.
Do we have a naming pattern to indicate a Global setting? I'm still not sure reading this setting's name how customers will know it applies to all projects once enabled in any project.
There was a problem hiding this comment.
There's no concept of global, I meant to write across TFMs.
Basically similar to what we do NuGetAudit and package pruning.
|
|
||
| |Pattern|Description| | ||
| |---|---| | ||
| |`analyzers/dotnet/{language}/{name}.dll`|Language-specific analyzers| |
There was a problem hiding this comment.
Is the name actually used for matching?
Basically the lib folder says use a *.dll, but the build folder requires the package id.
I think for analyzers it makes sense to just include all, but double checking.
There was a problem hiding this comment.
The name itself isn't used for the matching; I put it there as a placeholder for the actual name of the analyzers.
There was a problem hiding this comment.
I'd just add *.dll in this case.
| Update SDK asset resolution logic (e.g., ResolvePackageAssets.cs) to: | ||
| - Read analyzer assets directly from the "analyzers" group in "targets", instead of scanning all files in "libraries". | ||
| - Only load analyzers that are listed in this group. | ||
| - When `<RestoreEnableAnalyzerAssets>` is set to true, the SDK will only use the analyzer group that is in the assets file to determine which analyzers to include. |
There was a problem hiding this comment.
Given that we'll want to or this value, we should probably express that in the assets file, but we can talk about it in the implementation, no need to address it at this point.
There was a problem hiding this comment.
I'm curious about the 'or'-ing - project.assets.json are per-project, so why would a per-project flag not be enough to satisfy the detection criteria? It seems like Restore itself can know across all of the projects if it needs to track analyzer data or not, but can know per-project if it needs to emit the analyzer data to the assets file.
There was a problem hiding this comment.
per-project flag not be enough to satisfy the detection criteria?
The spec says we want to enable it for the new projects based on the tfms targeted, so we need to or it for the multi targeted projects.
| - `analyzers/dotnet/cs/MyAnalyzer.dll` (C# analyzer) | ||
| - `analyzers/dotnet/vb/MyAnalyzer.dll` (VB.NET analyzer) |
There was a problem hiding this comment.
(some of this is feedback about the existing analyzers selection)
This path doesn't mention TargetFramework at all - my intuition is that analyzers are specific to
- TFM
- language
- compiler name (and optionally version)
as well. Does this need to be modeled here? And how does selection of analyzers assets get impacted by TFM? cc @ericstj / @jaredpar for thoughts.
There was a problem hiding this comment.
Here's a reference https://learn.microsoft.com/en-us/nuget/guides/analyzers-conventions#analyzers-path-format I
Framework selection is not all that useful, and I suspect might lead to loading issues since Roslyn will prefer a cached assembly of the same name (so folks would need to be sure to name / version each analyzer assembly differently).
We recommend that analyzers and source-generators just test for supporting API in order to have behavior that differs on different frameworks.
Still the docs imply that at one point we imagined having a framework selection even though we don't use it. It's worth revisiting that now that we're changing how this works.
We do have compiler API version and make use of it today -- https://github.com/dotnet/sdk/blob/cdf3a40287e5e21a09cb44a6214e3efa8e032e66/src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs#L1078
There was a problem hiding this comment.
We recommend that analyzers and source-generators just test for supporting API in order to have behavior that differs on different frameworks.
Agree. Essentially an analyzer that has TFM specific diagnostics should always load but only offer the diagnostics when the TFM is being targeted by the compilation.
We do have compiler API version and make use of it today --
Believe the compiler API version should be modeled above.
| Update SDK asset resolution logic (e.g., ResolvePackageAssets.cs) to: | ||
| - Read analyzer assets directly from the "analyzers" group in "targets", instead of scanning all files in "libraries". | ||
| - Only load analyzers that are listed in this group. | ||
| - When `<RestoreEnableAnalyzerAssets>` is set to true, the SDK will only use the analyzer group that is in the assets file to determine which analyzers to include. |
There was a problem hiding this comment.
I'm curious about the 'or'-ing - project.assets.json are per-project, so why would a per-project flag not be enough to satisfy the detection criteria? It seems like Restore itself can know across all of the projects if it needs to track analyzer data or not, but can know per-project if it needs to emit the analyzer data to the assets file.
|
|
||
| The current rollout will be to opt-in for .NET 10. | ||
| For .NET 11, it is going to be opt-in for all frameworks and enabled by default for projects that target .NET 11. | ||
| The last step will be to remove the feature flag. |
There was a problem hiding this comment.
At what point would the "last step" happen? A future SDK version?
There was a problem hiding this comment.
As of right now, we want to keep it as a feature flag for .NET 11, and we are considering enabling it as a default for .NET 12.
| - Requires coordinated changes to NuGet and SDK | ||
| - Breaking change when enabled by default | ||
| - Potential minor performance impact during restore | ||
| - Older tools may not understand the new project.assets.json format |
There was a problem hiding this comment.
it would be nice to investigate this a bit further - are older versions of the nuget libraries around parsing the assets file resilient to additive changes to the json content? if not, it might be good to make them forward-compatible in this way so that future changes aren't breaking in this way.
nkolev92
dismissed
their stale review
There's some feedback from folks around the analyzers folder structure.
Fixes: #6279
Rendered MD