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] Provide contextual type when diagnosing invalid if-exp #11531

Merged
merged 1 commit into from Aug 29, 2017

Conversation

Projects
None yet
6 participants
@mkchoi212
Copy link
Contributor

mkchoi212 commented Aug 20, 2017

This pull request now provides better diagnostics (with fixits) when invalid type/member is provided as a return value of a ternary operator.

Resolves SR-910.

cc @xedin

lib/Sema/CSDiag.cpp Outdated
@@ -7200,18 +7200,19 @@ bool FailureDiagnosis::visitBindOptionalExpr(BindOptionalExpr *BOE) {
}

bool FailureDiagnosis::visitIfExpr(IfExpr *IE) {
auto typeCheckClauseExpr = [&](Expr *clause) -> Expr * {
return typeCheckChildIndependently(clause, Type(), CTP_Unused, TCCOptions(),
auto typeCheckClauseExpr = [&](Expr *clause, ContextualTypePurpose convertTypePurpose) -> Expr * {

This comment has been minimized.

@CodaFi

CodaFi Aug 20, 2017

Collaborator

You should ask the constraint system for the purpose of the contextual type

cs.getContextualTypePurpose()

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 20, 2017

Contributor

CS.getContextualTypePurpose() of a condExpr evaluates to CTP_ReturnStmt, causing the compiler to emit the old message and exiting
error: cannot convert return expression of type 'Bool' to return type 'E'.

Should I replace condExpr's CTP explicitly to CTP_Unused?

This comment has been minimized.

@CodaFi

CodaFi Aug 20, 2017

Collaborator

You should type check the condition as unused then and just use the purpose for the branches.

@CodaFi CodaFi requested a review from xedin Aug 20, 2017

test/Constraints/diagnostics.swift Outdated
@@ -188,6 +188,23 @@ func r17224804(_ monthNumber : Int) {
let monthString = (monthNumber <= 9) ? ("0" + monthNumber) : String(monthNumber)
}

// <rdar://problem/33768959> QoI: invalid diagnostics when input to ternary operator does not exist
func r33768959() {
enum E {

This comment has been minimized.

@CodaFi

CodaFi Aug 20, 2017

Collaborator

Indentation here is tabs, please use spaces to match the rest of this file

test/Constraints/diagnostics.swift Outdated
}

func compare(_ lhs: Int, _ rhs: Int) -> E {
guard lhs != rhs else { return .EqualTo }

This comment has been minimized.

@CodaFi

CodaFi Aug 20, 2017

Collaborator

Indent every syntactic level with two spaces.

This comment has been minimized.

@CodaFi

CodaFi Aug 20, 2017

Collaborator

Also, this part isn't really relevant to diagnostic emission and could be removed

This comment has been minimized.

@xedin

xedin Aug 20, 2017

Member

@mkchoi212 Just to clarify a little bit, we are trying to pick the snippets of code we use in the test suite as small as possible so it's worth to reduce the example from issue even further and delete all irrelevant parts.

lib/Sema/CSDiag.cpp Outdated
@@ -7200,18 +7200,19 @@ bool FailureDiagnosis::visitBindOptionalExpr(BindOptionalExpr *BOE) {
}

bool FailureDiagnosis::visitIfExpr(IfExpr *IE) {
auto typeCheckClauseExpr = [&](Expr *clause) -> Expr * {
return typeCheckChildIndependently(clause, Type(), CTP_Unused, TCCOptions(),
auto typeCheckClauseExpr = [&](Expr *clause, ContextualTypePurpose convertTypePurpose) -> Expr * {

This comment has been minimized.

@xedin

xedin Aug 20, 2017

Member

@mkchoi212 @CodaFi is right, if we want to provide contextual type to typeCheckClauseExpr we need to use both type and its purpose from the original constraint system. It's just so happens that in the snippet attached to the patch it is CTP_ResultStmt, but it might easily be e.g. CTP_AssignSource.

To properly fix the problem at hand (and you should also add additional test-case e.g. let _: E = lhs < rhs ? .LessThan : .GreaterTham) you'll have to make changes to diagnoseMemberFailures method which should be called when .GreaterTham is type-checked separately, since it doesn't matter what is the purpose of the contextual type when we use to to resolve UnresolvedMemberExpr it would work all different kinds of expressions.

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 21, 2017

Contributor

@xedin Nothing is breaking under diagnoseMemberFailures :(
Should we try to add more try____FixIts under a switch statement in diagnoseContextualConversionError for CTP_ReturnStmt?

This comment has been minimized.

@xedin

xedin Aug 21, 2017

Member

That is exactly the problem - error is not detected at the right place and it is not contextual, it's related to the member lookup, which should be diagnosed by visitUnresolvedMemberExpr and more specifically diagnoseMemberFailures. You need to try and figure out how to make that happen to properly fix diagnostic.

test/Constraints/diagnostics.swift Outdated
func compare(_ lhs: Int, _ rhs: Int) -> E {
guard lhs != rhs else { return .EqualTo }
// expected-error @+1 {{type 'E' has no member 'GreaterTham'}}
return lhs < rhs ? .LessThan : .GreaterTham

This comment has been minimized.

@CodaFi

CodaFi Aug 21, 2017

Collaborator

Yes. This test specifically requires this typo so lookup fails.

@slavapestov

This comment has been minimized.

@slavapestov slavapestov self-assigned this Aug 27, 2017

@mkchoi212

This comment has been minimized.

Copy link
Contributor

mkchoi212 commented Aug 27, 2017

@slavapestov @xedin Sorry for not having updated anything for awhile. I've been busy with moving! Anyways, I've been stuck with auto condExpr = typeCheckClauseExpr(IE->getCondExpr(), CS.getContextualTypePurpose()); not providing the correct diagnostics unless I explicitly provide the CTP as CTP_Unused.

@CodaFi

This comment has been minimized.

Copy link
Collaborator

CodaFi commented Aug 27, 2017

Yes. You should use unused for the Boolean condition and ask the constraint system for the purpose of the contextual type at the branches.

@mkchoi212

This comment has been minimized.

Copy link
Contributor

mkchoi212 commented Aug 27, 2017

@CodaFi Ok great! I guess I just overcomplicating things. I will check if there are any pre-existing tests for this, make some changes and commit them later today.

lib/Sema/CSDiag.cpp Outdated
auto typeCheckClauseExpr = [&](Expr *clause, ContextualTypePurpose convertTypePurpose) -> Expr * {
// Provide proper contextual type when type conversion is specified.
auto type = convertTypePurpose == CTP_Unused ? Type() : CS.getContextualType();
return typeCheckChildIndependently(clause, type, convertTypePurpose, TCCOptions(),

This comment has been minimized.

@xwu

xwu Aug 27, 2017

Collaborator

Drive-by nit: please adhere to 80-column lines and indent two spaces in this file.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:master branch Aug 27, 2017

@CodaFi

This comment has been minimized.

Copy link
Collaborator

CodaFi commented Aug 27, 2017

Cool! Looks great.

@swift-ci please smoke test

@xedin
Copy link
Member

xedin left a comment

Overall LGTM, only requires some minor changes now.

lib/Sema/CSDiag.cpp Outdated
};

This comment has been minimized.

@xedin

xedin Aug 27, 2017

Member

Please remove whitespace.

lib/Sema/CSDiag.cpp Outdated
auto typeCheckClauseExpr = [&](Expr *clause) -> Expr * {
return typeCheckChildIndependently(clause, Type(), CTP_Unused, TCCOptions(),
nullptr, false);
auto typeCheckClauseExpr = [&](Expr *clause, ContextualTypePurpose convertPurpose) -> Expr * {

This comment has been minimized.

@xedin

xedin Aug 27, 2017

Member

Please pass both type and purpose as parameters (you can have type default to Type() and purpose to CTP_Unused), I am trying to reduce usage of CS.getContextualType in places like this, it could be computed right before the call.

@xedin
Copy link
Member

xedin left a comment

Ok, one last thing - please squash everything into a single commit and mention Resolves: SR-xxxx in the commit message, also please use clang-format on changes to make sure that everything is formatted properly.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:master branch 2 times, most recently Aug 27, 2017

@xedin

This comment has been minimized.

Copy link
Member

xedin commented Aug 27, 2017

There is git-clang-format script in clang/utils you can use right before commit to format only staged changes. I just noticed that commit should also say [Diagnostics] instead of [Sema] because changes are related to diagnostics.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:master branch Aug 27, 2017

@mkchoi212 mkchoi212 changed the title [Sema] Provide contextual type when diagnosing invalid if-exp [Diagnostics] Provide contextual type when diagnosing invalid if-exp Aug 27, 2017

@mkchoi212 mkchoi212 force-pushed the mkchoi212:master branch to 3745d6b Aug 27, 2017

@mkchoi212

This comment has been minimized.

Copy link
Contributor

mkchoi212 commented Aug 29, 2017

@xedin Changes have been made. Please review :)

@xedin

This comment has been minimized.

Copy link
Member

xedin commented Aug 29, 2017

@mkchoi212 Perfect, thank you!

@xedin

xedin approved these changes Aug 29, 2017

Copy link
Member

xedin left a comment

LGTM!

@xedin

This comment has been minimized.

Copy link
Member

xedin commented Aug 29, 2017

@swift-ci please smoke test and merge

@mkchoi212

This comment has been minimized.

Copy link
Contributor

mkchoi212 commented Aug 29, 2017

@CodaFi @slavapestov @xedin Thank you guys for helping me complete my first PR to Swift! I learned a lot during the process. Moving on to the next bug!

@swift-ci swift-ci merged commit 45d971c into apple:master Aug 29, 2017

2 of 3 checks passed

Test and Merge (smoke test) Build started.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment