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

V4 Code Analyzer for detecting NPE with Collection Initializers #3478

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

normj
Copy link
Member

@normj normj commented Sep 19, 2024

Description

Add code analyzer for detecting collection initializers outside of an object initializer that could cause a null pointer exception for generated model collection properties. The main code to review is the AbstractNullCollectionInitializerAnalyzer class. A lot of the other code is refreshing our code analysis project system since it hasn't been updated in years.

Also did some code cleanup updating to newer versions of CodeAnalysis packages and fixing obsolete warnings.

This commit does not included the generated code to keep the PR size readable. Each service's CodeAnalysis project will get the following code generated into it which indicates to the base class the model namespace.

namespace Amazon.XRay.CodeAnalysis
{
    [DiagnosticAnalyzer(LanguageNames.CSharp)]
    public class NullCollectionInitializerAnalyzer : AbstractNullCollectionInitializerAnalyzer
    {
        protected override string GetModelNamespace()
        {
            return "Amazon.XRay.Model";
        }
    }
}

Testing

Added new unit tests.
Dry run pending

Screenshots (if appropriate)

image

… object initializer that could cause a null pointer exception for generated model collection properties.
@@ -0,0 +1,26 @@
using Microsoft.CodeAnalysis.CSharp.Testing;
Copy link
Member Author

@normj normj Sep 19, 2024

Choose a reason for hiding this comment

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

Everything in the unit test project's Verifiers folder is code you get today when you create a new code analysis with the VS template. I have copied it here so for my new unit test I can take advantage of it. I suggest treating the Verifiers folder as generated code that can be ignored.

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis" Version="3.0.0" />
</ItemGroup>
<ItemGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

This mock code analysis project is what the unit test uses.

@@ -149,6 +150,12 @@ private void GeneratePropertyValueAnalyzer(string codeAnalysisRoot, ServiceConfi
GeneratorDriver.WriteFile(codeAnalysisRoot, "Generated", "PropertyValueAssignmentAnalyzer.cs", text);
}

private void GenerateNullCollectionInitializerAnalyzer(string codeAnalysisRoot, ServiceConfiguration serviceConfiguration)
Copy link
Member Author

Choose a reason for hiding this comment

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

Each service's code analysis project gets a NullCollectionInitializerAnalyzer that identifies the model namespace to the base class.

{
public abstract class AbstractNullCollectionInitializerAnalyzer : DiagnosticAnalyzer
{
public const string DiagnosticId = "AWSSDK2000";
Copy link
Member Author

Choose a reason for hiding this comment

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

My thought process is we could reserve AWSSDK2XXX for any code analysis rules dealing with nullability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add this reasoning as a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dscpinheiro dscpinheiro added the v4 label Sep 23, 2024
Comment on lines +23 to +25
<ItemGroup>
<PackageReference Include="AWSSDK.DynamoDBv2" Version="4.0.0-preview.2" />
</ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this package reference? Should this be a project reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought process was I don't really need the SDK source to test this feature and didn't want to bring in a service package and core to have be part of the dev loop recompilation. I'm never debugging the the SDK I just need the types. If you feel strongly about using project references I can switch over but does add more projects to be compiled for the dev cycle.

Copy link
Contributor

Choose a reason for hiding this comment

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

No that makes sense.

{
public abstract class AbstractNullCollectionInitializerAnalyzer : DiagnosticAnalyzer
{
public const string DiagnosticId = "AWSSDK2000";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add this reasoning as a comment?

@normj normj merged commit c39b13c into v4-development Sep 25, 2024
1 check passed
@normj normj deleted the normj/code-analysis branch September 25, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants