Skip to content

C#: Update query to handle static field writes from properties. #12334

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 4 commits into from
Feb 28, 2023

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Feb 28, 2023

It turns out that the query cs/static-field-written-by-instance reports false positives if a static property writes to static field.
That is, the example below leads to a false positive. This is fixed in this PR.

static object backingField;
static object StaticProp
{
    get
    {
        return backingField ?? (backingField = new object()); // OK
    }
}

mbg
mbg previously approved these changes Feb 28, 2023
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

One question and one suggestion, otherwise looks good.

m.fromSource()
select fw.(VariableAccess), "Write to static field from instance method or constructor."
c = fw.getEnclosingCallable() and
not exists(Member m | m = c or m = c.(Accessor).getDeclaration() | m.isStatic()) and
Copy link
Member

Choose a reason for hiding this comment

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

Question: could this be implemented like this:

Suggested change
not exists(Member m | m = c or m = c.(Accessor).getDeclaration() | m.isStatic()) and
not (c.(Member).isStatic() or c.(Accessor).getDeclaration().isStatic()) and

and would that (potentially) be better for performance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even shorter

not [c.(Member), c.(Accessor).getDeclaration()].isStatic()

Copy link
Contributor Author

@michaelnebel michaelnebel Feb 28, 2023

Choose a reason for hiding this comment

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

Yes, it can be implemented like this, but I don't think that it will have any effect on performance.
I prefer the original solution as it re-uses the isStatic logic across the relevant members.
I prefer Toms suggestion.

@michaelnebel michaelnebel force-pushed the csharp/staticinitialisers branch from 6e7778b to 8bf81ab Compare February 28, 2023 12:23
@michaelnebel michaelnebel requested review from hvitved and mbg February 28, 2023 12:23
@michaelnebel michaelnebel force-pushed the csharp/staticinitialisers branch from 8bf81ab to 5174662 Compare February 28, 2023 12:38
@michaelnebel michaelnebel merged commit 734001b into github:main Feb 28, 2023
@michaelnebel michaelnebel deleted the csharp/staticinitialisers branch February 28, 2023 13:10
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.

3 participants