Skip to content

Sema: Various TypeRefinementContext tree fixes #76621

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 3 commits into from
Sep 24, 2024

Conversation

tshortli
Copy link
Contributor

There were a number of ways that the tree produced by TypeRefinementContextBuilder could be malformed:

  • Overlapping sibling nodes for enum cases that contain more than one element.
  • Overlapping sibling nodes for var decls with pattern binding decl as parents.
  • In multi-file compilation jobs, entire duplicate subtrees of the root node for a source file.

Fortunately, because the duplicated tree content was always ordered after the canonical child, the malformed trees never resulted in incorrect query results. Building the extra nodes did waste CPU and memory, though.

…ases.

Nodes in the TypeRefinementContext tree should be introduced for enum cases,
rather than enum elements, since its the cases that carry availability
annotations. Previously, enum cases in source that contained more than one
element would result in a malformed TRC tree that had overlapping sibling nodes
for each of the elements in a case declaration.
The `TypeRefinementContextBuilder` could visit a given `VarDecl` in the AST
multiple times when the `VarDecl` has a parent `PatternBindingDecl`, resulting
in duplicate TRC nodes.
Previously, the root TypeRefinementContext could contain duplicate top level
nodes for any source file that had its TRC built on-demand to satisfy type
checking requests before that file was explicitly typechecked.
@tshortli tshortli force-pushed the type-refinement-context-fixes branch from 3e6e9a5 to 2f0a22c Compare September 20, 2024 23:29
@tshortli
Copy link
Contributor Author

@swift-ci please smoke test

@tshortli tshortli changed the title Sema: Various type refinement context tree fixes Sema: Various TypeRefinementContext tree fixes Sep 21, 2024
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

ASTScope has some logic that runs in asserts builds that ensures each child node is a subrange of its parents, and that children are disjoint, etc. Maybe we can try adding something similar here?

@tshortli
Copy link
Contributor Author

tshortli commented Sep 21, 2024

These fixes actually came out of writing a verifier for the TypeRefinementContext tree. I'm still working on cleaning up the verifier and a few other things about TRCs but I wanted to get these fixes in separately since they're an independent improvement.

@slavapestov
Copy link
Contributor

Awesome!

@tshortli
Copy link
Contributor Author

swiftlang/swift-syntax#2858

@swift-ci please smoke test Linux

@tshortli tshortli merged commit 37fb5dd into swiftlang:main Sep 24, 2024
3 checks passed
@tshortli tshortli deleted the type-refinement-context-fixes branch September 24, 2024 02:04
tshortli added a commit to tshortli/swift that referenced this pull request Oct 31, 2024
swiftlang#76621 caused a regression by skipping
the AST nodes nested under `defer` blocks. The node associated with a `defer`
block is implicit because it is a kind of closure context synthesized by the
compiler. However, the nodes it contains are not implicit and so they must be
visited by the `TypeRefinementContextBuilder`.
tshortli added a commit to tshortli/swift that referenced this pull request Oct 31, 2024
swiftlang#76621 caused a regression by skipping
the AST nodes nested under `defer` blocks. The node associated with a `defer`
block is implicit because it is a kind of closure context synthesized by the
compiler. However, the nodes it contains are not implicit and so they must be
visited by the `TypeRefinementContextBuilder`.

Resolves rdar://139012152
tshortli added a commit to tshortli/swift that referenced this pull request Nov 1, 2024
swiftlang#76621 caused a regression by skipping
the AST nodes nested under `defer` blocks. The node associated with a `defer`
block is implicit because it is a kind of closure context synthesized by the
compiler. However, the nodes it contains are not implicit and so they must be
visited by the `TypeRefinementContextBuilder`.

Resolves rdar://139012152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants