Why isn't PatternBindingDecl scoped within VarDecl?

Development Compiler
1

Hey all,

I've been looking into fixing SR-13766, which is an issue I discovered while working on Periphery. In a nutshell, the type of a property is not associated with the property itself. Specifically, the type reference does not have 'container' relationship with the property declaration. For the purpose of identifying unused code, this is problematic, as it's not possible to determine if the type of an non-referenced property is itself also not referenced.

Initially I assumed this was an issue in lib/index/Index.cpp, however I now know that it's actually due to the AST structure. PatternBindingDecl is not scoped within VarDecl, but is instead inserted into the parent scope of the VarDecl, e.g a ClassDecl, StructDecl. This leads me to believe that this is in fact by design - why is that?

Conversely, the return type, and parameter types of functions are scoped within the function declaration, yet one could argue (from a syntactic perspective at least) that their scoping is the same as a property type's is to a property.

I'm sure I must be missing a crucial piece of the bigger picture here...

2

Consider the following PatternBindingDecl:

let (x, y) = computeIntersection()

The initializer expression here isn't something that can be attached to x or y individually, so it has to go on the PBD.

Okay, what if we wanted to add types?

let (x, y): (Int, Int) = computeIntersection()

The type here is (Int, Int). Again, that's not something that can be attached to x or y. But wait, why isn't it this?

// wrong
let (x: Int, y: Int) = computeIntersection()

…because that's the syntax for binding from a tuple with named elements:

// func computeIntersection() -> (x: Int, y: Int)
let (x: intersectionX, y: intersectionY) = computeIntersection()

So the AST design here matches the actual syntax of the language, even though it absolutely makes it less convenient to work with VarDecls on their own.

7 Likes
3

That totally make sense from an AST perspective, thanks for clearing that up. I wonder what your thoughts are on if/how far the index structure could diverge from the AST? Using your examples, the VarDecls could in theory still have container references to the explicit types and assignment values. I'm thinking along the lines of the index structure being a more semantically abstract form of the AST. The indexer is implemented using an AST walker, but I guess VarDecls and their bindings could be handled differently.

4

Someone else more involved in Index will have to answer that, sorry! Certainly the information should be there in some form, though, even if it ends up being indirected somehow.

1 Like
5

@bnbarham I think you were the last person to touch indexing of PatternBindingDecl. Any thoughts?

6

IIRC while the VarDecl is a sibling to the PatternBindingDecl, we actually walk the VarDecl as part of the NamedPattern instead (at least for source, see Traversal::shouldSkip). So at the moment we'll walk over a TypedPattern, walk any NamedPattern (which will index the decls), and then walk the TypeReprs (indexing the references).

I think the best bet is to do a custom walk in walkToPatternPre for TypedPattern in either the SemaAnnotator or the IndexSwiftASTWalker.

The general idea would be to walk over the subpattern as per normal, but add some state for the "containing" decl before walking the type(s). For the index walker that could just be in the EntitiesStack. SemaAnnotator would need to keep some extra state and then pass it out in walkToTypeReprPre.

7

Awesome, thanks, Ben. I'll have a go at implementing this.

8

No problem, let us know how you go :slight_smile:

9

So far I've setup the container as you described by implementing IndexSwiftASTWalker::walkToPatternPre. It's working great for containing the VarDecl type references, but I've also noticed some cases where it doesn't. References to property wrapper decls (the ComponentIdentTypeRepr bound decl), and any references from the assignment expression aren't getting container relationship with the VarDecl, because those references are created outside of the scope of a TypedPattern.

NOTE: My current implementation doesn't push the VarDecl onto the EntitiesStack, as that caused too much interference with other uses (e.g getParentDecl). Instead, I've added a new ContainerStack for both VarDecl and AbstractFunctionDecl, which I think is a bit cleaner.

After much poking around, it looks like the PatternBindingDecl itself might be the ideal visitation point to "hoist" up the VarDecl and push it onto the ContainerStack. However I can't implement this in IndexSwiftASTWalker::walkToDeclPre as the CustomAttr bound decl reference is created in SemaAnnotator::walkToDeclPre before the call to IndexSwiftASTWalker::walkToDeclPre. I guess I could add a new method to IndexSwiftASTWalker (e.g setupPatternBindingContainer) and call it near the top of SemaAnnotator::walkToDeclPre, however that'd muddy the walkTo*Pre/Post abstraction. I'm also not sure if it's safe to simply move the IndexSwiftASTWalker::walkToDeclPre call closer to the top in SemaAnnotator::walkToDeclPre, there's quite a lot going on in that method.

How do you suggest I proceed?

10

I wonder if perhaps a new pair of methods on IndexSwiftASTWalker might not be such a bad idea... It could be a single location for all current (PatternBidningDecl & AbstractFunctionDecl) and any future decls to be placed onto the new ContainerStack. I'm thinking something like this:

bool SemaAnnotator::walkToDeclPre(Decl *D) {
  // ...
  if (IsContainingDecl(D)) {
    SEWalker.walkToContainingDeclPre(D);
  }
  // ...
  SEWalker.walkToDeclPre(D);
}

bool SemaAnnotator::walkToDeclPost(Decl *D) {
    // ...
  if (IsContainingDecl(D)) {
    SEWalker.walkToContainingDeclPost(D);
  }
  // ...
  SEWalker.walkToDeclPost(D);
}

bool IsContainingDecl(Decl *D) {
  return isa<PatternBindingDecl>(D) || isa<AbstractFunctionDecl>(D);
}
11

OK, I went ahead and implemented what I described above, and it's now working for property wrapper and assignment expression references. It's also simpler than my previous implementation traversing TypedPattern, so that's an encouraging sign.

very WIP code is here, just so you can get a better idea: https://github.com/apple/swift/compare/main...ileitch:SR-13766?expand=1

Let me know if you think this is good path to continue down.

Next I will try implement expanding tuples, such that type references and assignment expression references are contained by their associated VarDecl. For example:

let (a, b): (Int, String) = (x, y)

// variable | a | Def
// struct | Int |  Ref,RelCont
//    RelCont | variable | a
// variable | x | Ref,RelCont
//    RelCont | variable | a

// variable | b | Def
// struct | String |  Ref,RelCont
//    RelCont | variable | b
// variable | y | Ref,RelCont
//    RelCont | variable | b

For assignment expressions that can't be expanded, i.e let (a, b) = someFunc() then both a and b with have a containing reference to someFunc().

12