Swift: Support AST elements new in Swift 6.2 #20732
Conversation
Note that it does not seem to be possible to write test that exercise this code. Passing `-enable-experimental-feature KeyPathWithMethodMembers` to the extractor results in: ``` error: experimental feature 'KeyPathWithMethodMembers' cannot be enabled in production compiler ```
Note that we cannot write tests for these at the moment. Passing ``` -enable-experimental-feature DefaultIsolationPerFile ``` to the extractor results in: ``` error: experimental feature 'DefaultIsolationPerFile' cannot be enabled in production compile ```
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for new AST (Abstract Syntax Tree) elements introduced in Swift 6.2, including new declaration types, expression types, type representations, and accessor kinds. The changes also include database schema updates, upgrade/downgrade scripts, and test file adjustments.
Key changes:
- Added support for
UsingDecl,UnsafeExpr, andInlineArrayTypeAST elements - Extended
Accessorwith new accessor kinds:distributed get,read2,modify2, andinit - Updated
KeyPathComponentto support a new "apply" kind for method/initializer applications - Implemented database schema upgrade/downgrade logic for compatibility
Reviewed Changes
Copilot reviewed 35 out of 54 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| swift/schema.py | Defines new AST elements (UsingDecl, UnsafeExpr, InlineArrayType) and accessor properties |
| swift/ql/lib/swift.dbscheme | Updates database schema with new tables and relations |
| swift/extractor/translators/*.{h,cpp} | Adds translation logic for new Swift AST elements |
| swift/ql/lib/upgrades/*/upgrade.properties | Defines upgrade metadata for schema migration |
| swift/downgrades/*/upgrade.properties | Defines downgrade strategy to remove new elements |
| swift/ql/test/library-tests/ast/declarations.swift | Corrects typo in comment ("dirctly" → "directly") |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
I've looked over the code, it LGTM, though I find myself wanting a fair bit of practical reassurance as well for this sort of change. I appreciate the new test cases, I will look at the DCA run when it's finished, and we have existing tests / CI checks to lean on as well.
I'd like the usual reassurance that the upgrade and downgrade scripts have been tested.
Note that it does not seem to be possible to write test that exercise this
code.
And that this has at least been one-off tested somehow...
Note that we cannot write tests for these at the moment.
... and this.
| else | ||
| if kind = 1 or kind = 2 | ||
| then new_kind = kind + 1 | ||
| else new_kind = kind + 2 |
There was a problem hiding this comment.
This appears to be an edit to an existing upgrade script (and likewise the corresponding downgrade script). Was this intended? Has this script been in a release yet? Is it safe to edit it?
There was a problem hiding this comment.
Yes, that's intended. I noticed that this was incorrect when working on this PR. The scripts were introduced in the Swift 6.2 upgrade PR.
| select x, "getKind:", x.getKind(), property, subscript, opt_forcing, opt_chaining, opt_wrapping, | ||
| self, tuple_indexing | ||
| select x, "getKind:", x.getKind(), apply, property, subscript, opt_forcing, opt_chaining, | ||
| opt_wrapping, self, tuple_indexing |
There was a problem hiding this comment.
[very optional] Tests like this can be made a bit cleaner with the concat(describe... pattern.
There was a problem hiding this comment.
I'll leave this as-is. Given the amount of development we do on Swift, there's no gain in this.
There was a problem hiding this comment.
Yep, just wanted to make sure you were aware of the pattern really.
The only way I would be able to test this, is by doing a one-off custom developer build of the Swift compiler, and the seeing if things work there. At this point I'm not willing to invest any time in this. If you want I can rip this out and leave the TODOs in place. Note that users will not be able to exercise the code introduced in these commits for the same reason as I'm not able to write tests. |
Ah I see, long day, I missed the context that these are experimental features ... in which case we definitely don't need to support them, and I suppose it doesn't much matter whether or not the code is there (from a support point of view), nor indeed whether its tested. So it can remain there for future convenience. As long as it (1) doesn't break anything we do support (this is what our existing tests are for) and (2) gets tests added when these features are no longer experimental, I'm happy. How about the upgrade and downgrade scripts - are those tested? |
Yes, as far as I was able to. |
|
DCA is a bit messy, with some failed runs for unrelated reasons, but I see no reasons for concern. The dataset check error changes on Signal seem wobble (one of the v1 runs has a similar number of check errors as one of the v0 runs). |
3 participants
Commit-by-commit review is recommended.