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.

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

[Diagnostics] Improve error when type parameters aren't equal #17210

Merged
merged 1 commit into from Jun 15, 2018

Conversation

Projects
None yet
5 participants
@mdiep
Contributor

mdiep commented Jun 14, 2018

This improves the error for code like the following:

var a = [1,2,3,4]
var b = a.popFirst()

Before it would give this error:

error: '[Int]' requires the types '[Int]' and 'ArraySlice<Int>' be equivalent to use 'popFirst'
var b = a.popFirst()
          ^

I found that to be very unclear about why that was a requirement.

Now it gives this error, with note about where the method was declared:

error: 'Collection' requires the types '[Int]' and 'Array<Int>.SubSequence' ('ArraySlice<Int>') be equivalent to use 'popFirst'
var b = a.popFirst()
          ^
Swift.Collection:2:37: note: 'popFirst()' declared here
    @inlinable public mutating func popFirst() -> Self.Element?
                                    ^

That seems much clearer to me.

I'm not sure if it's possible to use a locally-built copy of Swift in Xcode, so I'm not sure if that location is enough for tools like Xcode to properly link to the declaration of popFirst.

Resolves SR-7177.

include/swift/AST/DiagnosticsSema.def Outdated
@@ -1432,6 +1432,9 @@ ERROR(types_not_equal,none,
ERROR(types_not_equal_in_call,none,
"%0 requires the types %1 and %2 be equivalent to use %3",
(Type, Type, Type, DeclName))
ERROR(types_not_equal_in_call_dependent,none,

This comment has been minimized.

@mdiep

mdiep Jun 14, 2018

Contributor

This ID is not my best naming, but nothing better jumped out at me.

lib/Sema/CSDiag.cpp Outdated
} else {
TC.diagnose(UDE->getLoc(), diag::types_not_equal_in_call, ownerType, lhs,
rhs, UDE->getName());
}

This comment has been minimized.

@mdiep

mdiep Jun 14, 2018

Contributor

This is… not as clean as I'd like. I'm very open to suggestions about how it might be improved.

This comment has been minimized.

@slavapestov

slavapestov Jun 14, 2018

Member

@DougGregor might want to take a look.

This comment has been minimized.

@xedin

xedin Jun 14, 2018

Member

At least to me new messages don't seem to be much clearer about what is going on especially for.popFirst(), maybe a better solution here would be to print the actual requirement which got broken like we do in other cases e.g. https://github.com/apple/swift/blob/master/test/Constraints/generics.swift#L251, WDYT?

This comment has been minimized.

@mdiep

mdiep Jun 14, 2018

Contributor

Hmm… it does seem like it be nice to have parity there 🤔

Unfortunately, I'm not sure it's possible without a somewhat major change to this diagnostic. (I'd love to be wrong on this.)

That information is available on the Requirement. I can get to that through member's GenericContext. It looks like the Constraint is generated from the Requirement, but I don't see a way to connect that Requirement back to the current Constraint to know which of the Requirements failed.

I tried moving the diagnoseArgumentGenericRequirements earlier—to be before this check—but it didn't hit.

It sure seems like there ought to be a way to unify these two things. But there's quite a bit of code between them, and I don't understand enough of the big picture to know how to connect them.

This PR seems like an improvement to me, so I'd probably take this improvement for now. But I'm happy to spend some more time looking—especially if you have any suggestions.

This comment has been minimized.

@xedin

xedin Jun 14, 2018

Member

Constraint locator should tell you exactly what requirement is this constraint related to and you can get it by index from generic signature of the context, take a look at how open{Unbound}Generic/openGenericRequirements generates such constraints.

This comment has been minimized.

@mdiep

mdiep Jun 15, 2018

Contributor

How does this look?

error: value of type '[Int]' has no member 'popFirst'
var b = a.popFirst()
          ^
Swift.Collection:2:37: note: candidate requires that the types '[Int]' and 'ArraySlice<Int>' be equivalent (requirement specified as 'Self' == 'Self.SubSequence')
    @inlinable public mutating func popFirst() -> Self.Element?
                                    ^

This comment has been minimized.

@xedin

xedin Jun 15, 2018

Member

@mdiep I'm not sure what is going on but for some reason I can't see any new changes... But the new node looks great though!

@xedin xedin self-requested a review Jun 14, 2018

@mdiep

This comment has been minimized.

Contributor

mdiep commented Jun 15, 2018

Okay, just pushed the updated code and tests.

Thanks for the help on this one, @xedin!

@xedin

xedin approved these changes Jun 15, 2018

LGTM! One last nit request - could you please use clang-format on the commit before we merge this?

@mdiep

This comment has been minimized.

Contributor

mdiep commented Jun 15, 2018

could you please use clang-format on the commit before we merge this?

Done!

@xedin

This comment has been minimized.

Member

xedin commented Jun 15, 2018

@swift-ci please test and merge

1 similar comment
@xedin

This comment has been minimized.

Member

xedin commented Jun 15, 2018

@swift-ci please test and merge

@DougGregor

This comment has been minimized.

Member

DougGregor commented Jun 15, 2018

This looks fantastic, thank you!

@swift-ci swift-ci merged commit 7937406 into apple:master Jun 15, 2018

4 of 5 checks passed

Test and Merge Build started.
Details
Swift Test Linux Platform 11152 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 55945 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

@mdiep mdiep deleted the mdiep:SR-7177 branch Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment