Skip to content

Swift: deal with incomplete ASTs #11131

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

Merged
merged 7 commits into from
Nov 8, 2022
Merged

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Nov 4, 2022

This PR makes the Swift extractor deal gracefully with incomplete ASTs, which may come from compilation errors, wrong assumptions on our side or plain extractor bugs. Any label which is left unspecified in the extractor will instead point to an instance of UnspecifiedElement.

Commit by commit review is strongly encouraged.

The implementation of this can be roughly divided into:

  • changing code generation so that in the DB all properties can also hold an instance of @unspecified_element. What element is used is marked with the @use_for_null decorator in schema.py. Each property type @bar is then replaced with @bar_or_none. cppgen must also account for this (using type TrapLabel<BarOrNoneTag>). A special treatment must be given to the case where the property type is the root class Element, as the null type is already also of root type, so @element_or_none is unneeded (and would introduce some ambiguities in the tag hierarchy on C++).
  • making a validation pass on each emitted entity in SwiftDispatcher. In order to do so, trapgen and cppgen generate the member function forEachLabel allowing to iterate through and possibly modify all labels in the trap or class entry. SwiftDispatcher is then able to locate the undefined labels and emit UnspecifiedElement instances in their place. If this is not possible, which can happen if the class id is unspecified or for some direct trap emissions (which is now rare), emission is skipped altogether.

On the QL side this is for now mostly transparent: missing elements will result in the relevant predicates not having results, but can be queried directly through the UnspecifiedElement class. Whether we want those predicates to actually return UnspecifiedElement, or whether we want UnspecifiedElement to appear in the AST printer is left for future discussion and implementation.

For the moment the extractor will print on standard error information about missing labels, but this shall be moved to proper diagnostics once we have that.

Notice that @unspecified_element can be used in upgrade/downgrade scripts to "discard" an element.

@github-actions github-actions bot added the Swift label Nov 4, 2022
@redsun82 redsun82 force-pushed the redsun82/swift-incomplete-ast branch from fb186a1 to eb2b5d1 Compare November 7, 2022 16:25
The test was inspired by locally running the query against files in
https://github.com/apple/swift/tree/main/test/Parse

A query for missing elements was also added to the AST tests, expecting
nothing to be found.
An interesting byproduct was finding a problematic `assert` in the
Swift headers. An incomplete `FallthroughStmt` was asserting on having
a destination. I did not find any other sensible way of getting rid of
the crash when running in debug mode than to patch the header.
@redsun82 redsun82 force-pushed the redsun82/swift-incomplete-ast branch from eb2b5d1 to 9731048 Compare November 8, 2022 10:47
@redsun82 redsun82 marked this pull request as ready for review November 8, 2022 11:27
@redsun82 redsun82 requested review from a team as code owners November 8, 2022 11:27
Copy link
Contributor

@AlexDenisov AlexDenisov left a comment

Choose a reason for hiding this comment

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

All in all, looks great.
The only tiny concern is that the extractor is becoming way too template-heavy as to my taste 😅

@redsun82 redsun82 merged commit 552c524 into main Nov 8, 2022
@redsun82 redsun82 deleted the redsun82/swift-incomplete-ast branch November 8, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants