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

Swift: make extractor compilable with Swift 6 #17699

Merged
merged 58 commits into from
Dec 13, 2024
Merged

Swift: make extractor compilable with Swift 6 #17699

merged 58 commits into from
Dec 13, 2024

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Oct 8, 2024

This updates the Swift extractor to Swift release 6.0.2.

Unfortunately we had to drop support for running on Linux entirely for the time being, and some work further down the line will be required to support some new Swift 6 goodies in analysis.

This also contains updates to how we pull in pre-built artifacts in our build, shifting from dsp-testing/codeql-swift-artifacts releases to LFS files in here, but keeping a mechanism to use dsp-testing/codeql-swift-artifacts releases for initial testing before committing the files to LFS. A good entry point to understand those changes is in the swift/third_party/resources/updating.md file.

@github-actions github-actions bot added the Swift label Oct 8, 2024
@PSchmiedmayer
Copy link

@redsun82 Thank you for working on this; greatly appreciated!

Copy link
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.

Initial thoughts on the library and query test changes. Obviously we need to look a bit deeper into these.

";Collection;true;suffix(from:);;;Argument[-1];ReturnValue;taint",
";Collection;true;suffix(_:);;;Argument[-1].CollectionElement;ReturnValue.CollectionElement;value",

Check warning

Code scanning / CodeQL

Redundant assignment. Warning

The variable row
has previously been assigned
the same value
.
Copy link
Contributor

Choose a reason for hiding this comment

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

It wouldn't do any harm to fix these.

";BidirectionalCollection;true;suffix(_:);;;Argument[-1];ReturnValue;taint",
";BidirectionalCollection;true;suffix(_:);;;Argument[-1].CollectionElement;ReturnValue.CollectionElement;value",
";BidirectionalCollection;true;suffix(from:);;;Argument[-1];ReturnValue;taint",
";BidirectionalCollection;true;suffix(_:);;;Argument[-1].CollectionElement;ReturnValue.CollectionElement;value",

Check warning

Code scanning / CodeQL

Redundant assignment. Warning

The variable row
has previously been assigned
the same value
.
import codeql.swift.elements.stmt.LabeledStmt
import codeql.swift.elements.pattern.Pattern
import codeql.swift.elements.decl.PatternBindingDecl
import codeql.swift.elements.decl.VarDecl

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.swift.elements.stmt.BraceStmt
.
import codeql.swift.elements.stmt.internal.LabeledStmtImpl::Impl as LabeledStmtImpl
import codeql.swift.elements.pattern.Pattern
import codeql.swift.elements.decl.PatternBindingDecl
import codeql.swift.elements.decl.VarDecl

Check warning

Code scanning / CodeQL

Redundant import Warning generated

Redundant import, the module is already imported inside
codeql.swift.elements.stmt.BraceStmt
.
@redsun82 redsun82 marked this pull request as ready for review December 9, 2024 08:48
@redsun82 redsun82 requested review from a team as code owners December 9, 2024 08:48
Copy link
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.

I've attempted to review this - the bazel stuff and most of the other wiring doesn't mean a lot to me, so I've tried to understand what I can and ask questions.

The tests, CI and DCA do give me a bit of confidence. I notice though:

  • new extraction errors in the DCA run (though lower dataset check failures)
  • slower extraction in the DCA run
  • and possibly less data extracted for SideStore

I'd like to hear your thoughts on how serious those issues are and what if anything can be done about them?

Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

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

The bazel changes LGTM. Thanks for putting those files into LFS now instead of pulling them from a third location.

I'll defer final approval of the PR to your team.

@redsun82
Copy link
Contributor Author

The tests, CI and DCA do give me a bit of confidence. I notice though:

  • new extraction errors in the DCA run (though lower dataset check failures)
  • slower extraction in the DCA run
  • and possibly less data extracted for SideStore

I'd like to hear your thoughts on how serious those issues are and what if anything can be done about them?

In a normal situation, we would probably need to look at those more in detail. Right now we don't really have capacity for that, and I'm quite happy with the result as is (I honestly thought it would look worse).

Copy link
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.

In a normal situation, we would probably need to look at those more in detail. Right now we don't really have
capacity for that, and I'm quite happy with the result as is (I honestly thought it would look worse).

OK. I think we should follow up at least to the point of diagnosing the majority of new extractor errors next year. It's not severe enough it has to block this work.

There's a CI (test) failure but it looks like it only has to do with packs being out of sync.

@redsun82 redsun82 merged commit 2cbb072 into main Dec 13, 2024
34 checks passed
@redsun82 redsun82 deleted the redsun82/swift-6 branch December 13, 2024 11:27
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.

4 participants