Skip to content

Swift: Unify ArrayContent and CollectionContent#14205

Merged
rdmarsh2 merged 9 commits intogithub:mainfrom
rdmarsh2:rdmarsh2/swift/unify-array-collection-content
Sep 14, 2023
Merged

Swift: Unify ArrayContent and CollectionContent#14205
rdmarsh2 merged 9 commits intogithub:mainfrom
rdmarsh2:rdmarsh2/swift/unify-array-collection-content

Conversation

@rdmarsh2
Copy link
Copy Markdown
Contributor

No description provided.

@rdmarsh2 rdmarsh2 requested a review from a team as a code owner September 13, 2023 13:25
@github-actions github-actions Bot added the Swift label Sep 13, 2023
@MathiasVP
Copy link
Copy Markdown
Contributor

MathiasVP commented Sep 13, 2023

Grep'ing for ArrayContent gives me this one result: https://github.com/rdmarsh2/ql/blob/rdmarsh2/swift/unify-array-collection-content/swift/ql/lib/codeql/swift/security/UnsafeJsEvalQuery.qll#L34. But otherwise this LGTM!

Should this technically have a breaking change note?

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.

Otherwise LGTM. Thank you for doing this!

Needs a DCA run IMO.

or
component.getName() = "ArrayElement" and
content instanceof Content::ArrayContent
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.

May I suggest we don't delete this case, but make it a synonym for CollectionElement. We can carefully ensure we're not using it ourselves, but that way we'll avoid breaking any user code that uses ArrayElement.

Comment thread swift/ql/lib/change-notes/2023-09-13-array-content-unification.md Outdated
Comment thread swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll
Robert Marsh and others added 2 commits September 14, 2023 13:08
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
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!

@rdmarsh2
Copy link
Copy Markdown
Contributor Author

DCA looks good, there's a fair bit of noise but I think this is actually a meaningful speedup overall.

@rdmarsh2 rdmarsh2 merged commit 55546fe into github:main Sep 14, 2023
@rdmarsh2 rdmarsh2 deleted the rdmarsh2/swift/unify-array-collection-content branch September 14, 2023 14:08
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