Skip to content

[Diagnostics] Remove FailureDiagnosis::diagnoseParameterErrors from CSDiag #29734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 11, 2020
Merged

[Diagnostics] Remove FailureDiagnosis::diagnoseParameterErrors from CSDiag #29734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5572832
[CSSimplify] Remove diagnostics obsolete diagnostics checks involving…
LucianoPAlmeida Feb 8, 2020
cc29dc5
[CSDiag] Removing diagnose parameter error from CSDiag
LucianoPAlmeida Feb 9, 2020
449ab73
[ConstraintSystem] Disambiguate cases of argument/param mismatches in…
LucianoPAlmeida Feb 9, 2020
46092af
[tests] Adjusting tests under type/opaque
LucianoPAlmeida Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 0 additions & 55 deletions lib/Sema/CSDiag.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,12 +182,6 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
return type;
}

/// Diagnose common failures due to applications of an argument list to an
/// ApplyExpr or SubscriptExpr.
bool diagnoseParameterErrors(CalleeCandidateInfo &CCI,
Expr *fnExpr, Expr *argExpr,
ArrayRef<Identifier> argLabels);

/// Attempt to diagnose a specific failure from the info we've collected from
/// the failed constraint system.
bool diagnoseExprFailure();
Expand Down Expand Up @@ -918,48 +912,6 @@ static Expr *getFailedArgumentExpr(CalleeCandidateInfo CCI, Expr *argExpr) {
}
}

/// If the candidate set has been narrowed down to a specific structural
/// problem, e.g. that there are too few parameters specified or that argument
/// labels don't match up, diagnose that error and return true.
bool FailureDiagnosis::diagnoseParameterErrors(CalleeCandidateInfo &CCI,
Expr *fnExpr, Expr *argExpr,
ArrayRef<Identifier> argLabels) {
// If we have a failure where the candidate set differs on exactly one
// argument, and where we have a consistent mismatch across the candidate set
// (often because there is only one candidate in the set), then diagnose this
// as a specific problem of passing something of the wrong type into a
// parameter.
//
// We don't generally want to use this path to diagnose calls to
// symmetrically-typed binary operators because it's likely that both
// operands contributed to the type.
if ((CCI.closeness == CC_OneArgumentMismatch ||
CCI.closeness == CC_OneArgumentNearMismatch ||
CCI.closeness == CC_OneGenericArgumentMismatch ||
CCI.closeness == CC_OneGenericArgumentNearMismatch ||
CCI.closeness == CC_GenericNonsubstitutableMismatch) &&
CCI.failedArgument.isValid() &&
!isSymmetricBinaryOperator(CCI)) {
// Map the argument number into an argument expression.
TCCOptions options = TCC_ForceRecheck;
if (CCI.failedArgument.parameterType->is<InOutType>())
options |= TCC_AllowLValue;

// It could be that the argument doesn't conform to an archetype.
Expr *badArgExpr = getFailedArgumentExpr(CCI, argExpr);

// Re-type-check the argument with the expected type of the candidate set.
// This should produce a specific and tailored diagnostic saying that the
// type mismatches with expectations.
Type paramType = CCI.failedArgument.parameterType;
if (!typeCheckChildIndependently(badArgExpr, paramType,
CTP_CallArgument, options))
return true;
}

return false;
}

bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
auto *fnExpr = callExpr->getFn();
auto fnType = CS.getType(fnExpr)->getRValueType();
Expand Down Expand Up @@ -997,9 +949,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
SmallVector<Identifier, 2> argLabelsScratch;
ArrayRef<Identifier> argLabels =
callExpr->getArgumentLabels(argLabelsScratch);
if (diagnoseParameterErrors(calleeInfo, callExpr->getFn(),
callExpr->getArg(), argLabels))
return true;

Type argType; // argument list, if known.
if (auto FTy = fnType->getAs<AnyFunctionType>()) {
Expand All @@ -1025,10 +974,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {

calleeInfo.filterListArgs(decomposeArgType(CS.getType(argExpr), argLabels));

if (diagnoseParameterErrors(calleeInfo, callExpr->getFn(), argExpr,
argLabels))
return true;

// Force recheck of the arg expression because we allowed unresolved types
// before, and that turned out not to help, and now we want any diagnoses
// from disallowing them.
Expand Down
34 changes: 10 additions & 24 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3072,23 +3072,16 @@ bool ConstraintSystem::repairFailures(
auto elt = path.back();
switch (elt.getKind()) {
case ConstraintLocator::LValueConversion: {
auto CTP = getContextualTypePurpose(anchor);
// Special case for `CTP_CallArgument` set by CSDiag
// while type-checking each argument because we yet
// to cover argument-to-parameter conversions in the
// new framework.
if (CTP != CTP_CallArgument) {
// Ignore l-value conversion element since it has already
// played its role.
path.pop_back();
// If this is a contextual mismatch between l-value types e.g.
// `@lvalue String vs. @lvalue Int`, let's pretend that it's okay.
if (!path.empty() && path.back().is<LocatorPathElt::ContextualType>()) {
auto *locator = getConstraintLocator(anchor, path.back());
conversionsOrFixes.push_back(
IgnoreContextualType::create(*this, lhs, rhs, locator));
break;
}
// Ignore l-value conversion element since it has already
// played its role.
path.pop_back();
// If this is a contextual mismatch between l-value types e.g.
// `@lvalue String vs. @lvalue Int`, let's pretend that it's okay.
if (!path.empty() && path.back().is<LocatorPathElt::ContextualType>()) {
auto *locator = getConstraintLocator(anchor, path.back());
conversionsOrFixes.push_back(
IgnoreContextualType::create(*this, lhs, rhs, locator));
break;
}

LLVM_FALLTHROUGH;
Expand Down Expand Up @@ -3521,13 +3514,6 @@ bool ConstraintSystem::repairFailures(
if (rhs->isExistentialType())
break;

// TODO(diagnostics): This is a problem related to `inout` mismatch,
// in argument position, and we got here from CSDiag. Once
// argument-to-pararameter conversion failures are implemented,
// this check could be removed.
if (lhs->is<InOutType>() || rhs->is<InOutType>())
break;

if (repairViaOptionalUnwrap(*this, lhs, rhs, matchKind, conversionsOrFixes,
locator))
break;
Expand Down
63 changes: 56 additions & 7 deletions lib/Sema/ConstraintSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2765,6 +2765,9 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
ParameterList,
/// General ambiguity failure.
General,
/// Argument mismatch ambiguity where each solution has the same
/// argument mismatch fixes for the same call.
ArgumentMismatch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that could be generalized to all fixes. That is exactly something @hborla is about to open a PR for, it would cover not only argument mismatches but any ambiguity which involves the same fix kind at a shared location :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the same callee location? because in those cases all the fixes are the same, but each one for an argument. e.g. In the case of

func f<T>(_: T, _: T) {}
f(Int(1), Float(1))

There is a fix located at arg #1 in the first solution which was attempted (Int, Int)->Void and another at #0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the two fixes here have different locators so it's not quite the same case as N common fixes across N solutions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, maybe the locator doesn't matter. With my changes here, I've implemented a special method on CSFix for diagnosing for ambiguity when the fix is "common" across all solutions, but I consider a fix "common" only when the locator is the same. Instead of assuming that the locator must be the same, we can let the particular fix kind decide what it means to be "common"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm ... so in for these case for AllowTupleTypeMismatch and AllowArgumentMismatch fixes will be special cases where we could ignore locator for common to be able to disambiguate.

};
auto ambiguityKind = AmbiguityKind::CloseMatch;

Expand All @@ -2775,16 +2778,52 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
return false;

if (fixes.size() > 1) {
ambiguityKind = (ambiguityKind == AmbiguityKind::CloseMatch ||
ambiguityKind == AmbiguityKind::ParameterList) &&
// Attempt to disambiguite in cases where the argument matches
// involves tuple mismatches. e.g.
// func t<T, U>(_: (T, U), _: (U, T)) {}
// func useTuples(_ x: Int, y: Float) {
// t((x, y), (x, y))
// }
// So fixes are ambiguous in all solutions.
if ((ambiguityKind == AmbiguityKind::CloseMatch ||
ambiguityKind == AmbiguityKind::ArgumentMismatch) &&
llvm::all_of(fixes, [](const ConstraintFix *fix) -> bool {
auto *locator = fix->getLocator();
return locator->findLast<LocatorPathElt::ApplyArgument>().hasValue();
}) ? AmbiguityKind::ParameterList
: AmbiguityKind::General;
return fix->getKind() == FixKind::AllowTupleTypeMismatch;
})) {
ambiguityKind = AmbiguityKind::ArgumentMismatch;
} else {
ambiguityKind =
(ambiguityKind == AmbiguityKind::CloseMatch ||
ambiguityKind == AmbiguityKind::ArgumentMismatch ||
ambiguityKind == AmbiguityKind::ParameterList) &&
llvm::all_of(
fixes,
[](const ConstraintFix *fix) -> bool {
auto *locator = fix->getLocator();
return locator
->findLast<LocatorPathElt::ApplyArgument>()
.hasValue();
})
? AmbiguityKind::ParameterList
: AmbiguityKind::General;
}
}

if (fixes.size() == 1) {
// Attempt to disambiguite in cases where all the solutions
// produces the same fixes for different generic arguments e.g.
// func f<T>(_: T, _: T) {}
// f(Int(1), Float(1))
//
ambiguityKind =
((ambiguityKind == AmbiguityKind::CloseMatch ||
ambiguityKind == AmbiguityKind::ArgumentMismatch) &&
fixes.front()->getKind() == FixKind::AllowArgumentTypeMismatch)
? AmbiguityKind::ArgumentMismatch
: AmbiguityKind::CloseMatch;
}

for (const auto *fix: fixes) {
for (const auto *fix : fixes) {
auto *locator = fix->getLocator();
// Assignment failures are all about the source expression,
// because they treat destination as a contextual type.
Expand Down Expand Up @@ -2816,6 +2855,15 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
return true;
});

if (ambiguityKind == AmbiguityKind::ArgumentMismatch &&
viableSolutions.size() == 1) {
// Let's apply the solution so the contextual generic types
// are available in the system for diagnostics.
applySolution(*viableSolutions[0]);
solutions.front().Fixes.front()->diagnose(/*asNote*/ false);
return true;
}

if (!diagnosable || viableSolutions.size() < 2)
return false;

Expand Down Expand Up @@ -2860,6 +2908,7 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
};

switch (ambiguityKind) {
case AmbiguityKind::ArgumentMismatch:
case AmbiguityKind::CloseMatch:
// Handled below
break;
Expand Down
4 changes: 2 additions & 2 deletions test/type/opaque.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,10 +270,10 @@ func associatedTypeIdentity() {

sameType(cr, c.r_out())
sameType(dr, d.r_out())
sameType(cr, dr) // expected-error{{}} expected-note {{}}
sameType(cr, dr) // expected-error {{cannot convert value of type '(some opaque.R).S' (associated type of protocol 'R') to expected argument type '(some opaque.R).S' (associated type of protocol 'R')}}
sameType(gary(candace()).r_out(), gary(candace()).r_out())
sameType(gary(doug()).r_out(), gary(doug()).r_out())
sameType(gary(doug()).r_out(), gary(candace()).r_out()) // expected-error{{}} expected-note {{}}
sameType(gary(doug()).r_out(), gary(candace()).r_out()) // expected-error {{cannot convert value of type 'some R' (result of 'candace()') to expected argument type 'some R' (result of 'doug()')}}
}

func redeclaration() -> some P { return 0 } // expected-note 2{{previously declared}}
Expand Down