[Index] Walk implicit DeclRefExpr with an explicit ValueDecl.

[ileitch]

All references to explicit declarations should be included in the index store, even when those references are themselves implicit. For example, a custom appendInterpolation(custom:) method in DefaultStringInterpolation is referenced implicitly via string interpolation: "\(custom: 1)".

Resolves: #56189

@benlangmuir

[ileitch]

Another option is to just add !isa<DeclRefExpr>(E) here, but I'm not sure of the implications of that, so opted for the safer option.

[benlangmuir]

I agree that checking for implicit references to explicit decls is the safe choice here. There may be fully implicit cases that we want to handle, but my feeling is we're better off handling those on a case-by-case basis.

[benlangmuir]

@swift-ci please smoke test

[ileitch]

I didn't realize there were more index related tests outside of test/Index, so naturally there's a bunch of failures relating to SourceKit.

I wonder then, if this change should only apply to IndexSwiftASTWalker, to avoid any change to SourceKit behavior? Also one issue I just noticed is that because the ValueDecl is explicit, the created reference doesn't get the Implicit role. I could achieve that by adding a new visit method to SourceEntityWalker (visitImplicitlyReferencedExplicitDecl) similar to the other visit methods we have that handle special cases (e.g visitCallAsFunctionReference).

Also looks like I should be able to remove some/all of handleCustomAttrInitRefs once the Implicit role can be applied.

[bnbarham]

I didn't realize there were more index related tests outside of test/Index, so naturally there's a bunch of failures relating to SourceKit.

The SourceEntityWalker is used for a bunch of different features, eg. cursor info.

I wonder then, if this change should only apply to IndexSwiftASTWalker, to avoid any change to SourceKit behavior?

I think it's worth adding to SemaAnnotator. It would fix cursor info not having results for test in your added test (it should give the custom appendInterpolation method). Would need to add a few more tests for eg. cursor info and rename.

EDIT: There may be some more work to fix it for the cursor info case, but in principle I think it's fine for SemaAnnotator to call passReference in this case.

Also one issue I just noticed is that because the ValueDecl is explicit, the created reference doesn't get the Implicit role. I could achieve that by adding a new visit method to SourceEntityWalker (visitImplicitlyReferencedExplicitDecl) similar to the other visit methods we have that handle special cases (e.g visitCallAsFunctionReference).

passReference takes a ReferenceMetaData where the third parameter is isImplicit (defaulting to false). You could change that to be DRE->isImplicit() where passReference is called in the DRE branch.

Also looks like I should be able to remove some/all of handleCustomAttrInitRefs once the Implicit role can be applied.

I'm not sure I follow this part - what's the change that would make this possible?

[ileitch]

@benlangmuir @bnbarham @akyrtzi

I'd like to get your thoughts on this.

class ClassA {
    init(){}
}
class ClassB {}
struct StructC {
    var x = 0
}
func someFunc() {
    _ = ClassA()
    _ = ClassB()
    _ = StructC()
    _ = StructC(x: 0)
}

Current main Swift will create explicit references for init() for all calls within someFunc(). However I think that behavior is wrong, as only ClassA has an explicit constructor declaration. The outcome for ClassB and StructC is misleading cursor info. Xcode's Jump To Definition lists init() as a destination, however since there's no explicit declaration the class/struct declaration is used as the location. If you look closely, you can see that the first 4 characters of the name are highlighted when performing the jump, which would be correct if it were highlighting a constructor.

I propose that we make these explicit references implicit, to match their declarations.

[ileitch]

passReference takes a ReferenceMetaData where the third parameter is isImplicit (defaulting to false). You could change that to be DRE->isImplicit() where passReference is called in the DRE branch.

I gave that a try, and came to the conclusion that these references should remain explicit. If we make them implicit, they're excluded from cursor info. So while they're technically implicit, I think it make sense to treat them as explicit. From the developer's perspective they look explicit, since they're references to explicit declarations. Thoughts?

[benlangmuir]

Yeah, I think the reference should be explicit here, even if the underlying decl is not. Just like a reference to a synthesized function (e.g. from Hashable or Codable synthesis) ought to be an explicit reference.

[bnbarham]

Yeah, I think the reference should be explicit here, even if the underlying decl is not. Just like a reference to a synthesized function (e.g. from Hashable or Codable synthesis) ought to be an explicit reference.

So I definitely agree that any actual reference (whether to an explicit or implicit decl) should be explicit. I'm not so sure about the case in the PR though - it's fairly borderline as to whether it's an implicit reference or not. The label is explicit but the call really does seem implicit to me.

If the only issue is that cursor info ignores implicit references but we want it do provide results for this case, I'd argue that we should just allow cursor info to visit them. I could be wrong, but my guess is that they were initially ignored because they sometimes don't have a source location, which would break CursorInfoResolver::visitDeclReference since it relies on having a valid range. I recently changed that to actually check for a valid range anyway.

Happy to take counter examples, but for the couple I can think of it would actually be useful to provide implicit references in the results - that applies for the appendInterpolation case as well as dynamic member lookups (eg. test/SourceKit/CursorInfo/cursor_info_keypath_member_lookup.swift:25:9 could give subscript as a result as well as topLeft). Note that I haven't actually checked whether we give a location in that case, we could always add that though 🤷

[ileitch]

Perhaps a way forward is to remove the explicit/implicit flag from references, and instead have cursor info look at the referenced decl when deciding if it should be included or not. I could be wrong, but I think in that case we'd only ever want to include explicit decls? What about others feature, e.g refactoring?

In terms of detecting unused code, Periphery doesn't use the explicitness of the reference at all, only the declaration's.

It looks like the work needed here it much greater than I'd originally anticipated. I wish I had more time to work on Swift 😅.

[benlangmuir]

I'd argue that we should just allow cursor info to visit them. I could be wrong, but my guess is that they were initially ignored because they sometimes don't have a source location, which would break CursorInfoResolver::visitDeclReference since it relies on having a valid range. I recently changed that to actually check for a valid range anyway.

SGTM

[bnbarham]

Perhaps a way forward is to remove the explicit/implicit flag from references, and instead have cursor info look at the referenced decl when deciding if it should be included or not. I could be wrong, but I think in that case we'd only ever want to include explicit decls? What about others feature, e.g refactoring?

I don't think we want to remove that flag from the index. I'd go ahead with what I said above, ie. just remove the implicit check on the reference in the cursor info resolver.

[ileitch]

I don't think we want to remove that flag from the index. I'd go ahead with what I said above, ie. just remove the implicit check on the reference in the cursor info resolver.

Might that then include things like implicit property accessor decls in cursor info? I believe they have the same source location as the property themselves, so it sounds like they might slip past your change to CursorInfoResolver::visitDeclReference?

Using the explicitness of the referenced decl would avoid cases like this I think. Is there a scenario in which we'd actually want a reference to an implicit decl included in cursor info?

[ileitch]

Also I'm still confused as to why we'd want explicit references to implicit constructors: #38900 (comment). This appears to have been an intentional change, added in 40c8904.

[bnbarham]

Might that then include things like implicit property accessor decls in cursor info? I believe they have the same source location as the property themselves, so it sounds like they might slip past your change to CursorInfoResolver::visitDeclReference?

Using the explicitness of the referenced decl would avoid cases like this I think. Is there a scenario in which we'd actually want a reference to an implicit decl included in cursor info?

I think it's reasonable for cursor info to return all involved symbols, whether implicit or not. Clients can then decide themselves whether to show them, ie. they could ignore symbols flagged with "synthesized" if there are non-synthesized symbols as well. An example there could be == and hashValue where IMO it would make sense to go to the location that caused them to be synthesized (ie. wherever Equatable/Hashable were added), though right now we don't currently add locations to them.

[akyrtzi]

Also I'm still confused as to why we'd want explicit references to implicit constructors

The discussion here focuses on what cursor-info should do, but AFAIK the way we walk the AST affects what symbol occurrences we get for indexing purposes. For the indexing data we need all the symbol references whether the declarations are implicit or not.

For example, for the case of implicit constructors we want the capability to do an index query to find all the places where that particular type is constructed.

[ileitch]

For example, for the case of implicit constructors we want the capability to do an index query to find all the places where that particular type is constructed.

Thanks, that makes sense. I'd rephrase my question a little then. Why do the references to the implicit constructors not have their implicit flag set? It has the unfortunate side effect of those references being included in cursor info, which causes Xcode to offer up the constructor as a jump to destination.

If it's OK for those references to marked as implicit, it'd simplify the changes I need to make in this PR.

[bnbarham]

Thanks, that makes sense. I'd rephrase my question a little then. Why do the references to the implicit constructors not have their implicit flag set? It has the unfortunate side effect of those references being included in cursor info, which causes Xcode to offer up the constructor as a jump to destination.

If it's OK for those references to marked as implicit, it'd simplify the changes I need to make in this PR.

The reference is explicit, the symbol is implicit/synthesized - which is now marked with synthesized in cursor info. The references should be included in cursor info. It's up to clients of cursor info as to what they do with that information.

[ileitch]

Sorry you're right, I should have said that the reference being explicit was causing Xcode to display it, not merely it's presence in cursor info.

[bnbarham]

New-ish but not brand new: #40062