Skip to content

[Diagnostics] Strip off unrelated optionals from generic parameter di… #31982

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 1 commit into from
May 25, 2020
Merged

[Diagnostics] Strip off unrelated optionals from generic parameter di… #31982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 22 additions & 13 deletions lib/Sema/CSDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,11 +630,32 @@ void GenericArgumentsMismatchFailure::emitNoteForMismatch(int position) {

bool GenericArgumentsMismatchFailure::diagnoseAsError() {
auto anchor = getAnchor();
auto path = getLocator()->getPath();

auto fromType = getFromType();
auto toType = getToType();

// This is a situation where right-hand size type is wrapped
// into a number of optionals and argument isn't e.g.
//
// func test(_: UnsafePointer<Int>??) {}
//
// var value: Float = 0
// test(&value)
//
// `value` has to get implicitly wrapped into 2 optionals
// before pointer types could be compared.
auto path = getLocator()->getPath();
unsigned toDrop = 0;
for (const auto &elt : llvm::reverse(path)) {
if (!elt.is<LocatorPathElt::OptionalPayload>())
break;

// Disregard optional payload element to look at its source.
toDrop++;
}

path = path.drop_back(toDrop);

Optional<Diag<Type, Type>> diagnostic;
if (path.empty()) {
if (isExpr<AssignExpr>(anchor)) {
Expand Down Expand Up @@ -684,18 +705,6 @@ bool GenericArgumentsMismatchFailure::diagnoseAsError() {
break;
}

case ConstraintLocator::OptionalPayload: {
// If we have an inout expression, this comes from an
// InoutToPointer argument mismatch failure.
if (isExpr<InOutExpr>(anchor)) {
diagnostic = diag::cannot_convert_argument_value;
auto applyInfo = getFunctionArgApplyInfo(getLocator());
if (applyInfo)
toType = applyInfo->getParamType();
}
break;
}

case ConstraintLocator::TupleElement: {
auto rawAnchor = getRawAnchor();

Expand Down
4 changes: 2 additions & 2 deletions test/Constraints/diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1433,7 +1433,7 @@ func assignGenericMismatch() {
var a: [Int]?
var b: [String]

a = b // expected-error {{cannot assign value of type '[String]' to type '[Int]?'}}
a = b // expected-error {{cannot assign value of type '[String]' to type '[Int]'}}
// expected-note@-1 {{arguments to generic parameter 'Element' ('String' and 'Int') are expected to be equal}}

b = a // expected-error {{cannot assign value of type '[Int]' to type '[String]'}}
Expand All @@ -1447,7 +1447,7 @@ func assignGenericMismatch() {
let value: [Int] = []
func gericArgToParamOptional(_ param: [String]?) {}

gericArgToParamOptional(value) // expected-error {{convert value of type '[Int]' to expected argument type '[String]?'}}
gericArgToParamOptional(value) // expected-error {{convert value of type '[Int]' to expected argument type '[String]'}}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure how I feel about these diagnostics changes. I understand that Optional is not part of the mismatch, but it is important in identifying where that type is in the source code. This error message seems wrong because the argument type is [String]?, not [String].

Could we instead add a note similar to the one we emit for same-type generic requirements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the diagnostics here were wrong because they mentioned optional only on one side and produced a note which mentioned either Element or Pointee which is confusing. So this is at least an improvement on status quo.

Because it's allowed to implicitly load value into an optional, the problem is that either we produce a diagnostic where everything is optional - e.g. [Int]? and [String]? and a note which would have to say that Wrapped is a problem (which is incorrect), or we ignore optionals and produce diagnostic about actual mismatch which is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

This error message seems wrong because the argument type is [String]?, not [String].

I thought the same when added this in #30814, because that is the argument type as the user declared :)
So to me, it seemed like the type in the message was not right in some way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like before doing anything else we need to figure out diagnostic format that allows to mention complete types as well as their portions e.g. tuple mismatches which have to mention whole tuple as well as narrowed down message(s) about mismatched bits.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine that the diagnostic only has optional on one side - I think that's how users think about value-to-optional conversions anyway. I was suggesting adding a new note about the Element types expected to be the same (in this case), not Wrapped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hborla What do you mean by new note? There is a note about Element already associated with that error.

Copy link
Member

Choose a reason for hiding this comment

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

Oh whoops, you're right, I didn't even notice the note because it wasn't part of the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if we wanted to start mentioning types as written we'd end up with diagnostics like this:

func takesConstPointer(_ x: UnsafePointer<Int>??) -> Character { return "x" }

var ff: [Float] = [0, 1, 2]
takesConstPointer(ff)

error:  cannot convert value of type '[Float]' to expected argument type 'UnsafePointer<Int>??'

note: arguments to generic parameter 'Pointee' ('Float' and 'Int') are expected to be equal
takesConstPointer(ff)
                  ^

Which is not very informative, but maybe okay?

Copy link
Member

Choose a reason for hiding this comment

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

I think that case in particular might benefit from a tailored argument-to-parameter error message that somehow mentions the array-to-pointer conversion so that Pointee in the note makes more sense... I do think the note is very informative because it tells you exactly which part of the type that's wrong (Float vs Int)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently produce a diagnostic which mentions UnsafePointer<Float> instead of [Float] and doesn't have optional on the right-hand side, that's why it makes sense to have Pointee in the note.

If we wanted to start diagnosing chains like [Float] -> [Float]? -> [Float]?? -> UnsafePointer<Float>?? which then matched vs. UnsafePointer<Int>?? we'd probably need to get some kind of "nested" error representation first...

// expected-note@-1 {{arguments to generic parameter 'Element' ('Int' and 'String') are expected to be equal}}

// Inout Expr conversions
Expand Down
4 changes: 2 additions & 2 deletions test/Constraints/valid_pointer_conversions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ func givesPtr(_ str: String) {
takesDoubleOptionalPtr(i) // expected-error {{cannot convert value of type 'Int' to expected argument type 'UnsafeRawPointer??'}}
takesMutableDoubleOptionalPtr(arr) // expected-error {{cannot convert value of type '[Int]' to expected argument type 'UnsafeMutableRawPointer??'}}

takesMutableDoubleOptionalTypedPtr(&i) // expected-error {{cannot convert value of type 'UnsafeMutablePointer<Int>' to expected argument type 'UnsafeMutablePointer<Double>??'}}
takesMutableDoubleOptionalTypedPtr(&i) // expected-error {{cannot convert value of type 'UnsafeMutablePointer<Int>' to expected argument type 'UnsafeMutablePointer<Double>'}}
// expected-note@-1 {{arguments to generic parameter 'Pointee' ('Int' and 'Double') are expected to be equal}}
}

// SR12382
func SR12382(_ x: UnsafeMutablePointer<Double>??) {}

var i = 0
SR12382(&i) // expected-error {{cannot convert value of type 'UnsafeMutablePointer<Int>' to expected argument type 'UnsafeMutablePointer<Double>??'}}
SR12382(&i) // expected-error {{cannot convert value of type 'UnsafeMutablePointer<Int>' to expected argument type 'UnsafeMutablePointer<Double>'}}
// expected-note@-1 {{arguments to generic parameter 'Pointee' ('Int' and 'Double') are expected to be equal}}