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

Enable throw helper analyzers #45954

Merged
merged 15 commits into from
Jan 10, 2023
Merged

Enable throw helper analyzers #45954

merged 15 commits into from
Jan 10, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 9, 2023

Fixes #43482
Fixes #43503

Enable analyzers for validating null and argument ranges, object disposed, etc.

These helpers aren't available on older frameworks. Complications from projects that target netframework462 and netstandard2.0 mean that:

  • Added custom throw helpers and use them in those projects
  • Throw helpers copy logic from the throw methods on ArgumentNullException and other exceptions. The helpers call through to those methods internally if available.
  • Some ArgumentOutOfRangeException throw helpers use generic math. I've made the custom method throw helpers just take int. If there are places where other types are used, then we can add those as needed. You can see that ArgumentOutOfRangeThrowHelper.cs.

This PR currently only fixes a portion of warnings from aspnetcore. I want to get feedback on the approach before continuing with the rest of the repo.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 9, 2023

@stephentoub I copied the analyzers you enabled here.

How did you handle projects that target older frameworks? Or was it not an issue in dotnet/runtime projects?

@stephentoub
Copy link
Member

How did you handle projects that target older frameworks?

For projects that only target older frameworks, it's not an issue, since the analyzers key off of the APIs being available.

For projects that multitarget and thus where the analyzers would trigger for some builds but the methods aren't available for others, I effectively suppressed the analyzers:
https://github.com/dotnet/runtime/blob/5da4a9e919dcee35f831ab69b6e475baaf798875/src/libraries/Directory.Build.targets#L37-L40
If/when we have support for extension statics, we can better address this by using polyfills for those statics in all projects.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 9, 2023

For projects that only target older frameworks, it's not an issue, since the analyzers key off of the APIs being available.

Ok! I'll do the same for additional projects in this PR. At the very least, if we tackle them in aspnetcore, it can be in a follow-up PR. That helps reduce the size of this PR.

For projects that multitarget and thus where the analyzers would trigger for some builds but the methods aren't available for others, I effectively suppressed the analyzers: dotnet/runtime@5da4a9e/src/libraries/Directory.Build.targets#L37-L40 If/when we have support for extension statics, we can better address this by using polyfills for those statics in all projects.

That works well, except when the fixer updates shared source files that are reused in a multitargeted project. We have a lot of small shared source files in aspnetcore. I could suppress analyzers in the shared source, but adding the throw helpers is relatively easy.

@mitchdenny
Copy link
Member

LGTM. The only nit: I had was somewhat inconsistent whitespace between throws invocations (probably a side effect of how the code was in the first place). Thanks for the explanation on why you needed the throw helpers, was scratching my head when I first looked at it.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 9, 2023

@mitchdenny Whitespace lines between throw helpers removed

There were about 800 😮‍💨

regex: (\s)*ArgumentNull[^;]*;[\n]+[\n]+(\s)*ArgumentNull

Copy link
Member

@mgravell mgravell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a doozy to review; nit: a bit late now, but I'd probably have said just do the ANEs in one big beast of a PR, as that is going to be the majority and the simplest - then a separate PR for the rest, allowing us to actually focus on the others where there may be more subtlety required, without losing focus (resumes scrolling)

@JamesNK JamesNK mentioned this pull request Jan 9, 2023
1 task
@mgravell
Copy link
Member

mgravell commented Jan 9, 2023

One thing I can't tell from the PR: the analyzer that suggests this - does it ever make the suggestion for Nullable<T>, because that would add a box. In a way it is a shame we aren't using the helper class throughout, because then we could perhaps (and I'm not suggesting this is a good idea) validate against that in DEBUG builds:

#if DEBUG
public static void ThrowIfNull<T>(T value, ...) where T : class
#else
public static void ThrowIfNull(object value, ...)
#endif

@stephentoub
Copy link
Member

stephentoub commented Jan 9, 2023

does it ever make the suggestion for Nullable, because that would add a box

The analyzer is fairly conservative and requires that the parameter type be a reference type:
https://github.com/dotnet/roslyn-analyzers/blob/0b2bc22f087f4a42915a3f5d7049a0e71a179423/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs#L184
so that rules out nullable. Note that it also rules out unconstrained generics, even though the JIT generally does a good job of eliding the boxing in such cases.

@mgravell
Copy link
Member

mgravell commented Jan 9, 2023

eyeballed all, and my browser officially hates this PR, but: nothing nefarious or worrying found; a few nits/comments added above

Base automatically changed from wtgodbe/net8SDK to main January 9, 2023 18:43
@wtgodbe wtgodbe requested review from a team, dougbu and wtgodbe as code owners January 9, 2023 18:43
@JamesNK JamesNK force-pushed the jamesnk/throwhelpers branch from 7c3de47 to d5e79bd Compare January 9, 2023 22:55
@mitchdenny
Copy link
Member

Exactly one CPU core was harmed in the reviewing of this pull request.

@JamesNK JamesNK merged commit c096dbb into main Jan 10, 2023
@JamesNK JamesNK deleted the jamesnk/throwhelpers branch January 10, 2023 00:52
@ghost ghost added this to the 8.0-preview1 milestone Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants