[SourceKit] Fix a crash in SwiftDocumentStructureWalker::getObjCSelectorName() #22075
Conversation
|
@swift-ci Please smoke test |
lib/AST/Decl.cpp
Outdated
| if (params->get(0)->hasInterfaceType()) { | ||
| return params->get(0)->getInterfaceType()->isVoid(); | ||
| } else if (auto tyR = params->get(0)->getTypeLoc().getTypeRepr()) { | ||
| // Fallback to textual check. This can happen when getting ObjC selector |
There was a problem hiding this comment.
I would prefer it if instead you did
if (auto *resolver = getASTContext().getLazyResolver())
resolver->resolveDeclSignature(this);
There was a problem hiding this comment.
I understand. But we want to avoid to resolve anything because "document structure" request should be purely syntactic.
There was a problem hiding this comment.
Is there really a resolver ? If there is a resolver it seems like a mistake to me because we don't need any kind of typechecking for syntactic requests so we shouldn't create a resolver.
There was a problem hiding this comment.
Confirmed there's no resolver at this point.
| // RUN: %sourcekitd-test -req=structure %s -- %s | %FileCheck %s | ||
|
|
||
| class C { | ||
| @IBAction init(foo: Void) {} |
There was a problem hiding this comment.
I think the bug also happens with @objc; you don't need @IBAction
There was a problem hiding this comment.
It doesn't happen with @objc because ObjC selector name is only resolved for @IBAction. https://github.com/apple/swift/blob/ff0d764ec922a5d69c2466499120fea0afe1c854/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp#L1275-L1277
|
Another idea is not to vend ObjC selector name for |
|
I think you'll find a lot of AST operations don't work if there's no resolver, and we haven't really tested that possibility much. Maybe SourceKit should always ensure there's a type checker present? |
As Rintaro mentioned earlier this is for syntactic-only functionality, so we explicitly don't want to invoke typechecking. |
|
@akyrtzi what benefit to you derive from there not being any type checking? As work on the request evaluator progresses the distinction between 'type checking' and not will blur. Eventually we hope to get rid of the global TypeChecker object altogether. If you go down the road of not having a TypeChecker, you will get this long tail of crashes to fix for no apparent benefit. What I'm really saying is that issuing AST queries without a TypeChecker configured is no longer a supported use-case for the compiler infrastructure and we would like it to be impossible in the future. |
|
The benefits of syntactic-only functionality are almost instantaneous performance and robust functionality that does not depend on needing to get correct build settings, needing swift module files to have been generated, needing clang modules to build correctly with the right search paths, etc. There is a concrete benefit that I'm not sure when and if we can approximate with the evaluator but hopefully we can come close. In any case, I discussed with Rintaro and since this is a specific code-path only for @IBActions, we'll go with what Rintaro suggested earlier which is to avoid calling this for constructor decls. |
|
In this case you're fixing a crash, so I'm not really buying the robustness argument. The compiler has much better test coverage in 'type checker is present' mode than this special SourceKit behavior. |
rintaro
force-pushed
the
sourcekit-rdar47426948
branch
from
106d19e to
5646499
Compare
…torName() SourceKit document structure request used to crash if if there's inititalizer with single named parameter with `@IBAction` attribute. This used to happen because ConstructorDecl::isObjCZeroParameterWithLongSelector() requires interface type of the parameter but the constructor doesn't have it at this stage. This change fixes that by not vending ObjC name for constructor or destructors. rdar://problem/47426948
|
With 'robustness' I'm not referring to crashers, I'm referring to the fragility of multiple systems interacting with each other and things breaking down easily if one piece goes wrong (like "swift module needs to be present, which needs the clang module to get built, which needs correct search paths from the build system to find the headers, which provided a headermap that ended up finding the wrong header", etc..) Compared to that, the syntactic functionality only depends on handling the contents of a single swift file and nothing else. |
rintaro
force-pushed
the
sourcekit-rdar47426948
branch
from
5646499 to
606b551
Compare
rintaro
changed the title
|
@swift-ci Please smoke test |
SourceKit document structure request used to crash if if there's initializer with single named parameter with
@IBActionattribute.This used to happen because
ConstructorDecl::isObjCZeroParameterWithLongSelector()requiresinterface type of the parameter but the constructor doesn't have it at this stage.
This PR fixes that by not vending ObjC name for constructor or destructors.
rdar://problem/47426948