Swift: dataflow for `for-in` loops by rdmarsh2 · Pull Request #13909 · github/codeql

rdmarsh2 · 2023-08-07T15:11:11Z

No description provided.

rdmarsh2 · 2023-08-07T15:11:58Z

This is built on top of #13838 - only the last two commits are new

geoffw0

This is pleasingly straightforward!

I'd like to see iteration through CollectionElement supported + tested in addition to ArrayContent.

rdmarsh2 · 2023-08-08T20:46:02Z

Actually after starting on dictionaries I'm no longer sure this is the right way to do this - I think there's an implicit call that generates an iterator or something, and this PR won't capture the behavior of dictionaries properly.

rdmarsh2 · 2023-08-11T16:52:12Z

After rethinking how dictionary flow is implemented I think this will work properly for dictionaries. There's still some weirder options (self-iterators and such) that won't be handled.

geoffw0

I'm happy with this, I think you just need to finish the discussion with @MathiasVP and probably do a DCA run.

geoffw0 · 2023-09-13T15:29:15Z

Is there anything I can do to help get this mergeable / merged?

rdmarsh2 · 2023-09-13T18:53:22Z

#14119 broke the original version of the PR, and I decided to go ahead and handle the iterator semantics directly rather than doing a short-term workaround. The QL needs a re-review and there's some extractor changes in a3e250a that I'd like @AlexDenisov's review on. There's likely to be a small test conflict with #14205, so it's probably best to get that out of the way.

AlexDenisov · 2023-09-14T09:26:23Z

#14119 broke the original version of the PR, and I decided to go ahead and handle the iterator semantics directly rather than doing a short-term workaround. The QL needs a re-review and there's some extractor changes in a3e250a that I'd like @AlexDenisov's review on. There's likely to be a small test conflict with #14205, so it's probably best to get that out of the way.

C++ change looks good!

+      [
+        ";IteratorProtocol;true;next();;;Argument[-1].CollectionElement;ReturnValue.OptionalSome;value"
+      ]


geoffw0 · 2023-09-27T13:49:03Z

I'm unable to reproduce the new swift/string-length-conflation result from DCA locally - @rdmarsh2 do you have any idea what's going on with it?

rdmarsh2 · 2023-09-28T20:44:57Z

I haven't been able to reproduce it either... Not sure what's going on there.

geoffw0 · 2023-10-02T16:54:05Z

... and indeed the new result does not appear on the second DCA run. I'm inclined not to worry about it any further, at least with regards to this PR.

geoffw0

I've just done a quick re-review, including a very brief test of the upgrade script for myself (focussing on AST intactness). I'm happy.

Might be good to wait for a 👍 from @MathiasVP as well.

MathiasVP

A couple of comments. Mostly for my own understanding.

MathiasVP · 2023-10-04T08:03:47Z

+  )
+}
+
+// TODO: do we need a new_apply_expr_arguments


What's the status of this TODO?

We don't need it - the this parameter is part of new_self_apply_exprs.

MathiasVP · 2023-10-04T08:06:27Z

  fillLabeledStmt(stmt, entry);
  entry.body = dispatcher.fetchLabel(stmt.getBody());
-  entry.sequence = dispatcher.fetchLabel(stmt.getTypeCheckedSequence());
+ // entry.sequence = dispatcher.fetchLabel(stmt.getTypeCheckedSequence());


Hmm... Isn't the sequence the xs in:

for i in xs { /* ... */ }

do we no longer extract this expression then?

Oh, looking at the PrintAST test I now see what's going on. Instead of being able to do myLoop.getSequence() you should now do myLoop.getIteratorVar().getInit(). Is that right?

If so, we should probably (in a future PR) add a convenience predicate on ForEachStmt.

That actually makes the change note easier to write (and no need for a deprecation), so I'll go ahead and do it as part of this.

Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>

Fix conflict in AST test output

MathiasVP

LGTM!

MathiasVP

LGTM once the code is formatted!

geoffw0

LGTM too (when the checks pass)!

geoffw0

👍

rdmarsh2 requested a review from a team as a code owner August 7, 2023 15:11

github-actions Bot added documentation Swift labels Aug 7, 2023

geoffw0 reviewed Aug 7, 2023

View reviewed changes

Comment thread swift/ql/test/library-tests/dataflow/dataflow/test.swift Outdated

rdmarsh2 closed this Aug 8, 2023

rdmarsh2 reopened this Aug 11, 2023

github-actions Bot removed the documentation label Aug 11, 2023

rdmarsh2 force-pushed the rdmarsh2/swift/for-in branch from e942019 to 1e0f410 Compare August 11, 2023 17:02

github-actions Bot added the documentation label Aug 15, 2023

MathiasVP reviewed Aug 16, 2023

View reviewed changes

Comment thread swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll Outdated

geoffw0 reviewed Aug 16, 2023

View reviewed changes

AlexDenisov and others added 5 commits August 22, 2023 17:36

Swift: extract nextCall from ForEachStmt

e9fdbfa

Swift: update test expectations

b657301

merge main into for-in getnextcall branch

50d23f1

Swift: extract iterator variable for for-in loops

a3e250a

Swift: add flow through for-in loops

988a871

rdmarsh2 force-pushed the rdmarsh2/swift/for-in branch from 7026eb2 to 988a871 Compare September 8, 2023 20:07

github-actions Bot removed the documentation label Sep 8, 2023

Robert Marsh added 2 commits September 12, 2023 19:04

Swift: fix for-try-await CFG

53ad559

Swift: add dataflow test for async for

7ca6b6f

Swift: move IteratorProtocol model to its own file

e0fae76

rdmarsh2 force-pushed the rdmarsh2/swift/for-in branch from 2f0f3f6 to e0fae76 Compare September 13, 2023 20:09

Merge branch 'main' into rdmarsh2/swift/for-in

56b646a

github-advanced-security AI found potential problems Sep 14, 2023

View reviewed changes

Robert Marsh added 2 commits September 26, 2023 18:03

Swift: Fix db types in downgrade script for for-in

81ee932

Swift: fix ql reference in downgrade script

50521f7

Robert Marsh added 7 commits September 27, 2023 18:01

Swift: more downgrade script fixes

a7a50cf

Swift: Hopefully fix codegen test failure

9975a18

Swift: another attempt at fixing downgrade script

188ad6a

Swift: yet another downgrade script fix

4799ba0

Swift: remove table from downgrade properties

d71d6b2

Swift: remove another table from downgrade script

64e4c1e

Swift: Fix extra param in downgrade script

77fd917

geoffw0 previously approved these changes Oct 2, 2023

View reviewed changes

Comment thread swift/ql/lib/change-notes/2023-09-14-for-in-data-flow.md

MathiasVP reviewed Oct 4, 2023

View reviewed changes

Swift: Use getFullyConverted in CFG generation

b5f1296

Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>

rdmarsh2 dismissed geoffw0’s stale review via b5f1296 October 4, 2023 13:44

Robert Marsh added 2 commits October 4, 2023 15:58

Merge branch 'main' into rdmarsh2/swift/for-in

00068d6

Fix conflict in AST test output

Swift: remove TODO comment

9c3b72c

MathiasVP previously approved these changes Oct 4, 2023

View reviewed changes

Robert Marsh added 2 commits October 4, 2023 20:05

Swift: add getSequence back to ForEachStmt

5dd7c14

Swift: update for-in change note

a402bfc

rdmarsh2 dismissed MathiasVP’s stale review via a402bfc October 4, 2023 20:07

MathiasVP reviewed Oct 5, 2023

View reviewed changes

geoffw0 previously approved these changes Oct 5, 2023

View reviewed changes

Robert Marsh added 2 commits October 6, 2023 15:21

Swift: Split for-each change note into two lines

ec292ca

Swift: improve QLDoc for getSequence

c281db6

rdmarsh2 dismissed geoffw0’s stale review via c281db6 October 6, 2023 15:24

Swift: fix QLDoc formatting for getSequence

8558741

geoffw0 approved these changes Oct 6, 2023

View reviewed changes

rdmarsh2 merged commit 8af7277 into github:main Oct 9, 2023

Conversation

rdmarsh2 commented Aug 7, 2023

Uh oh!

rdmarsh2 commented Aug 7, 2023

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rdmarsh2 commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdmarsh2 commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

geoffw0 commented Sep 13, 2023

Uh oh!

rdmarsh2 commented Sep 13, 2023

Uh oh!

AlexDenisov commented Sep 14, 2023

Uh oh!

Check warning

geoffw0 commented Sep 27, 2023

Uh oh!

rdmarsh2 commented Sep 28, 2023

Uh oh!

geoffw0 commented Oct 2, 2023

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MathiasVP Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

MathiasVP Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

MathiasVP Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rdmarsh2 commented Aug 8, 2023 •

edited

Loading

rdmarsh2 commented Aug 11, 2023 •

edited

Loading

MathiasVP Oct 4, 2023 •

edited

Loading