[Sema] Clean up extension binding a little #83777
Conversation
hamishknight
requested review from
CodaFi,
DougGregor,
bnbarham,
hborla,
rintaro,
slavapestov and
xedin
as code owners
|
@swift-ci please SourceKit stress test https://ci.swift.org/job/swift-PR-macos-with-sourcekit-stress-tester/431/ |
hamishknight
force-pushed
the
ext-cleanup
branch
from
b5609b4 to
6c649cb
Compare
|
@swift-ci please test |
include/swift/AST/Decl.h
Outdated
There was a problem hiding this comment.
Can lldb evaluate the request too?
There was a problem hiding this comment.
Yeah I'm hoping so, the current call to computeExtendedNominal is buried within some code synthesis they're doing after parsing, I'm not yet sure where the best place would be to stick a call to the main request. In any case, I'd like to tackle that in a follow-up since I don't want it to block this PR
lib/Parse/Parser.cpp
Outdated
There was a problem hiding this comment.
I know it was like this before your PR, but do you mind moving IDEInspectionSecondPassRequest::evaluate() and Parser::performIDEInspectionSecondPassImpl() to lib/IDE? Feels like a layering violation for this to live here.
There was a problem hiding this comment.
Heh yeah, I wasn't too bothered about the layering violation here since this code will all change once we switch over to ASTGen. I think the rest of this logic being in libParse is sort of fine (at the very least, defining a Parser method outside of libParse seems like it would just be a different kind of layering violation), I think the better fix here would be sinking the call to bindExtensions into the callback code that's in libIDE, I can make that change
There was a problem hiding this comment.
Sunk the call to bindExtensions into libIDE. Hopefully we can also delete this when we requestify extension binding (last I checked there was a horrible request cycle though the clang importer)
I missed this in my previous patch that moved extension binding until after we've mutated the AST for IDE inspection, this ad-hoc extension binding logic is no longer necessary.
This will mainly be useful once extension binding is fully requestified, but even now it's a good idea to ensure module loading isn't kicking name lookup.
- Turn `BindExtensionsForIDEInspectionRequest` into the main extension binding request. - Change `ExtendedNominalRequest` such that it's no longer what extension binding calls into to do the name lookup, instead it calls directly into `computeExtendedNominal`. `getExtendedNominal` can then be the entrypoint for `ExtendedNominalRequest` and assumes that extension binding has already run. This avoids needing to fake the dependency relationship in the DeclChecker.
This avoids the layering violation of calling `bindExtensions` from parser code.
hamishknight
force-pushed
the
ext-cleanup
branch
from
6c649cb to
d3be024
Compare
|
@swift-ci please smoke test |
2 participants
BindExtensionsForIDEInspectionRequestinto the main extension binding request.ExtendedNominalRequestsuch that it's no longer what extension binding calls into to do the name lookup, instead it calls directly intocomputeExtendedNominal.getExtendedNominalcan then be the entrypoint forExtendedNominalRequestand assumes that extension binding has already run. This avoids needing to fake the dependency relationship in the DeclChecker.