Skip to content

[release/6.0.2xx] Fix analyzer nullref on assembly attribute #2530

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

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Jan 20, 2022

The analyzer runs for property assignment operations, including those in
attributes. To check whether the property assignment should warn, we
look for RUC on the containing symbol. However, assembly-level attributes
are contained in the global namespace, which has a null containing type.

Customer Impact

Assembly-level attribute with property assignment causes a crash in the RequiresAttribute analyzers. #2563 hits what is likely this issue.

Testing

Unit tests added in this change. This was originally discovered as part of running the analyzer over the existing linker tests as part of #2523.

Risk

Very low risk - the change adds an extra null check without impacting other code paths.

The analyzer runs for property assignment operations, including those in
attributes. To check whether the property assignment should warn, we
look for RUC on the containing symbol. However, assembly-level attributes
are contained in the global namespace, which has a null containing type.
Copy link
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

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

LGTM

@agocke
Copy link
Member

agocke commented Jan 21, 2022

This will have to go through QB approval. Could you make the change in main first? I'm not sure it will meet the bar without a customer report.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. Please take for consideration in 6.0.2xx

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

Successfully merging this pull request may close these issues.

5 participants