[CodeCompletion] Fix a crash when completing the call pattern of a function that takes a parameter pack #72570
Conversation
ahoppen
requested review from
bnbarham,
hamishknight,
rintaro,
hborla and
xedin
as code owners
ahoppen
force-pushed
the
ahoppen/parameter-pack-pattern-crash
branch
from
4e5dd2e to
c4247bc
Compare
lib/Sema/CSSimplify.cpp
Outdated
| if (!isSynthesizedArg && isForCodeCompletion() && | ||
| containsIDEInspectionTarget(arg) && !lhs->isVoid() && | ||
| !lhs->isUninhabited()) | ||
| fixBehavior = FixBehavior::Suppress; |
There was a problem hiding this comment.
What happens if this returns true like before instead of trying to suppress the diagnostic which wouldn't be emitted anyway in the code completion mode?
There was a problem hiding this comment.
The reason I want to record the fix is so I can inspect it while producing code completion results here: https://github.com/apple/swift/pull/72568/files#diff-1e522eaf116c0d9062c2ba193e9a4cd86687f87ebd6d3afbd705917a2f5746dbR178-R192.
That’s basically the content of #72568 which this PR depends on.
There was a problem hiding this comment.
Hm, containsIDEInspectionTarget is overly broad then... If there is a mismatch between result type of a call where code completion token is used and some contextual type, I think presence of completion token shouldn't affect that.
Is the following example covered by this check as well:
func other(_: Int) {}
func test(_: String) {}
func compute(_: Int) -> String {}
other(test(compute(#^COMPLETE^#)))
lib/IDE/ArgumentCompletion.cpp
Outdated
| for (auto Fix : S.Fixes) { | ||
| if (auto CM = dyn_cast<AllowArgumentMismatch>(Fix)) { | ||
| if (simplifyLocatorToAnchor(CM->getLocator()) == ASTNode(ParentCall)) { | ||
| ExpectedCallType = CM->getToType(); |
There was a problem hiding this comment.
nit: Can we merge two if conditions into one using && operator. This might improve readability
There was a problem hiding this comment.
I don’t think so because one of them is binding a variable (auto CM) and the other one is a boolean check.
There was a problem hiding this comment.
Technically we could now do (as of C++17) if (auto CM = dyn_cast<AllowArgumentMismatch>(Fix); simplifyLocatorToAnchor(CM->getLocator()) == ASTNode(ParentCall)). But we don't use this pattern anywhere else in the compiler and IMO we probably want to agree in general that it's a pattern we want to use first.
There was a problem hiding this comment.
The part of the condition after ; still gets evaluated even if CM is nullptr. Which means that it would need to be if (auto CM = dyn_cast<AllowArgumentMismatch>(Fix); CM && simplifyLocatorToAnchor(CM->getLocator()) == ASTNode(ParentCall)) which I find sufficiently ugly to not use.
lib/IDE/ArgumentCompletion.cpp
Outdated
| // func bar() -> String {} | ||
| // foo(bar(#^COMPLETE^#)) | ||
| for (auto Fix : S.Fixes) { | ||
| if (auto CM = dyn_cast<AllowArgumentMismatch>(Fix)) { |
There was a problem hiding this comment.
I think regardless of presence of fix we should dig up the "contextual" type from the solution instead of taking result of the call because conversions are allowed here i.e. bar() could produce String but foo might expect String? as a parameter.
ahoppen
force-pushed
the
ahoppen/parameter-pack-pattern-crash
branch
from
c4247bc to
992587a
Compare
…nction that takes a parameter pack
The issue here was that we inferred the contextual type of the `Mixer` call to be the following, inferred from the result type of the partial call to the initializer of `Mixer`
```
(bound_generic_struct_type decl="swift_ide_test.(file).Mixer@/Users/alex/src/swift/test/IDE/complete_parameter_pack_as_call_argument.swift:3:8"
(pack_type num_elements=1
(pack_expansion_type
(pattern=unresolved_type)
(count=unresolved_type))))
```
Technically, the contextual type that we should have here should be `Any` (from `print`) but getting that information out of the constraint system turns out to be quite hard. swiftlang#72568 makes some improvements in this area but in general the constraint system does not contain enough information to figure out the contextual type of an arbitrary expression.
The important thing right now is that the unresolved type in here trips the constraint system over when comparing types of code completion results with the contextual type. To prevent the crash for now, reset the expected call type if the computed type contains an unresolved type.
rdar://124166587
Co-authored-by: Pavel Yaskevich <[email protected]>
ahoppen
force-pushed
the
ahoppen/parameter-pack-pattern-crash
branch
from
992587a to
078e1ea
Compare
ahoppen
changed the title
|
@swift-ci Please smoke test |
The issue here was that we inferred the contextual type of the
Mixercall to be the following, inferred from the result type of the partial call to the initializer ofMixerTechnically, the contextual type that we should have here should be
Any(fromprint) but getting that information out of the constraint system turns out to be quite hard. #72568 makes some improvements in this area but in general the constraint system does not contain enough information to figure out the contextual type of an arbitrary expression.The important thing right now is that the unresolved type in here trips the constraint system over when comparing types of code completion results with the contextual type. To prevent the crash for now, reset the expected call type if the computed type contains an unresolved type.
rdar://124166587