Skip to content

Swift: add SetContent for data flow#13838

Merged
geoffw0 merged 10 commits intogithub:mainfrom
rdmarsh2:rdmarsh2/swift/set-content
Aug 7, 2023
Merged

Swift: add SetContent for data flow#13838
geoffw0 merged 10 commits intogithub:mainfrom
rdmarsh2:rdmarsh2/swift/set-content

Conversation

@rdmarsh2
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Set.qll Outdated
Comment thread swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Set.qll Outdated
Comment thread swift/ql/test/library-tests/dataflow/dataflow/test.swift Outdated
@MathiasVP
Copy link
Copy Markdown
Contributor

Also small nitpick: How about just calling this CollectionContent? There's no reason why this Content can't be extended for non-Set things in a future PR.

@geoffw0
Copy link
Copy Markdown
Contributor

geoffw0 commented Jul 31, 2023

How about just calling this CollectionContent?

I like this idea.

@rdmarsh2 rdmarsh2 marked this pull request as ready for review August 3, 2023 21:10
@rdmarsh2 rdmarsh2 requested a review from a team as a code owner August 3, 2023 21:10
cs.isSingleton(c) and
result = "EnumElement[" + c.getSignature() + "]"
)
or
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this, I missed it in #13795 .


let set2 = Set([source()])
sink(arg: set2.randomElement()!) // $ MISSING: flow=703
sink(arg: set2.randomElement()!) // $ flow=706
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉

@MathiasVP
Copy link
Copy Markdown
Contributor

Do you mind rebasing this on the latest main so that we get a smaller diff @rdmarsh2?

TEnumContent(ParamDecl f) { exists(EnumElementDecl d | d.getAParam() = f) } or
TArrayContent()
TArrayContent() or
TSetContent()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still think we should call this CollectionContent (see #13838 (comment))

@rdmarsh2 rdmarsh2 force-pushed the rdmarsh2/swift/set-content branch from 3bcf28e to 024c5cf Compare August 4, 2023 18:44
Comment thread swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll Outdated
Comment thread swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll Outdated
Comment thread swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll Outdated
Comment thread swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImplSpecific.qll Outdated
Comment thread swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Collection.qll Outdated
Comment thread swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Set.qll Outdated
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
MathiasVP
MathiasVP previously approved these changes Aug 7, 2023
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM once we have a successful DCA run!

Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

This looks great, and I could use the readStep fix in two of my open PRs and CollectionContent in one, so the sooner we merge this the better!

@geoffw0
Copy link
Copy Markdown
Contributor

geoffw0 commented Aug 7, 2023

DCA LGTM. Merging.

@geoffw0 geoffw0 merged commit 022a066 into github:main Aug 7, 2023
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