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

CA2208 Instantiate argument exceptions correctly #7187

Closed
wants to merge 6 commits into from

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Dec 30, 2021

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 10, 2022
@elachlan
Copy link
Contributor Author

elachlan commented Jan 10, 2022

Build is failing on dependency microsoft.codeanalysis.collections:

/home/vsts/.nuget/packages/microsoft.codeanalysis.collections/4.0.0-4.21379.20/contentFiles/cs/netstandard2.0/ImmutableSegmentedList`1.cs(346,23): error CA2208: (NETCORE_ENGINEERING_TELEMETRY=Build) Call the ArgumentException constructor that contains a message and/or paramName parameter
/home/vsts/.nuget/packages/microsoft.codeanalysis.collections/4.0.0-4.21379.20/contentFiles/cs/netstandard2.0/ImmutableSegmentedList`1+ValueBuilder.cs(249,27): error CA2208: (NETCORE_ENGINEERING_TELEMETRY=Build) Call the ArgumentException constructor that contains a message and/or paramName parameter

https://github.com/dotnet/roslyn/blob/24ef0f30f29298be003d2d7d5de29f9906c37611/src/Dependencies/Collections/ImmutableSegmentedList%601.cs#L346

https://github.com/dotnet/roslyn/blob/24ef0f30f29298be003d2d7d5de29f9906c37611/src/Dependencies/Collections/ImmutableSegmentedList%601%2BValueBuilder.cs#L249

@elachlan
Copy link
Contributor Author

Related Roslyn issue:
dotnet/roslyn#45286

@Forgind
Copy link
Member

Forgind commented Jan 11, 2022

So we're waiting for that issue to be resolved? That looks outside the MSBuild layer.

@Forgind Forgind removed the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 11, 2022
@elachlan
Copy link
Contributor Author

Yes, I am not sure how to tell the analyzer not to analyze code like this. So I am attempting to fix the upstream code.

@Youssef1313
Copy link
Member

Youssef1313 commented Jan 11, 2022

@elachlan Those errors should be suppressible via editorconfig per dotnet/roslyn#55992, probably something like:

[**/microsoft.codeanalysis.collections/**/*.cs]
# CA2208: Instantiate argument exceptions correctly
dotnet_diagnostic.CA2208.severity = none

If it doesn't work (I haven't tested), then dotnet/roslyn#55992 should probably re-considered.

FYI @jaredpar

@elachlan
Copy link
Contributor Author

.editorconfig didn't work. But I don't think we are using IncludeContentInPack.

@Youssef1313
Copy link
Member

@elachlan I can't think of how Microsoft.CodeAnalysis.Collections contents are seen by the compiler without, somehow, IncludeContentInPack being set (on Roslyn side I mean).

Since you can confirm that .editorconfig doesn't work. I'd like @jaredpar to re-consider something for dotnet/roslyn#55992

@elachlan
Copy link
Contributor Author

See the commit adding it here:
f1cd160

@Youssef1313
Copy link
Member

Youssef1313 commented Jan 11, 2022

@elachlan I'm referring to roslyn side, the package itself has its source code included in pack:

image

That's why I think dotnet/roslyn#55992 is relevant.

@elachlan elachlan marked this pull request as draft January 12, 2022 07:34
@elachlan
Copy link
Contributor Author

@sharwell in dotnet/roslyn#55992 it was pointed out that globalconfig defined analyzers apply to source code supplied by nuget packages as well. Where as .editorconfig files do not apply analyzers to against the packages.

This means I will unfortunately need to move back to using .editorconfigs. Unless you know of a better way?

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

Successfully merging this pull request may close these issues.

4 participants