Skip to content

[Diagnostics] Port diagnostics from FailureDiagnosis::visitObjectLiteralExpr #29304

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

Merged

Conversation

LucianoPAlmeida
Copy link
Contributor

Ports the FailureDiagnosis::visitObjectLiteralExpr from CSDiag to the new framework.

cc @xedin @hborla :)

@LucianoPAlmeida LucianoPAlmeida requested a review from xedin January 18, 2020 20:29
@xedin
Copy link
Contributor

xedin commented Jan 19, 2020

Thank you, @LucianoPAlmeida! I'll take a look on Monday/Tuesday.

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, I have left some comments inline.

@@ -1036,6 +1036,34 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
if (cs.recordFix(fix, /*impact=*/extraArguments.size() * 5))
return cs.getTypeMatchFailure(locator);
}
} else if (args != params && cs.shouldAttemptFixes() &&
!cs.hasFixFor(loc)) {
if (auto *expr = dyn_cast<ObjectLiteralExpr>(loc->getAnchor())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this special case here is unnecessary because it should be possible to generate applicable fn constraint in visitObjectLiteralExpr and let to solver handle the rest (currently it calls into matchCallArguments directly which is not optimal). Could you give it a shot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give it a shot?

For sure 👍


// rdar://problem/49861813
#fileLiteral() // expected-error {{cannot convert value of type '()' to expected argument type '(resourceName: String)'}}
#fileLiteral() // expected-error {{missing argument label 'resourceName:' in call}}
Copy link
Contributor

Choose a reason for hiding this comment

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

To be precise the problem here is not the absence of label alone but the argument type as well...

// Attempt diagnose and maybe suggest import the correct module for
// ObjectLiteral expr.
if (cs.shouldAttemptFixes()) {
if (auto *fix = SpecifyObjectLiteralTypeImport::attempt(cs, loc)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The way failure manifests here is related to solver being unable to infer type of _ExpressibleByFileReferenceLiteral which is connected to result of the call by literal conforms constraint. I think the way we should approach it - mark type variable representing a result of ObjectLiteralExpr as "potential hole" while generating constraints for ObjectLiteralExpr expression and record a fix in TypeVariableBinding::attempt just like we do for generic parameters and closure result type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing it that way eliminates a need for attempt since we'll know exactly what happened.

if (!isa<ObjectLiteralExpr>(anchor))
return nullptr;

if (cs.getContextualType())
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition should be removed since we know exactly that the problem is related to solver's inability to infer result type of the ObjectLiteralExpr. Also it should have been getContextualType(anchor) otherwise it might be contextual to completely different expression...

@LucianoPAlmeida LucianoPAlmeida force-pushed the port-object-literal-diagnostics branch from 081a556 to 7a564e5 Compare January 21, 2020 01:28
@LucianoPAlmeida LucianoPAlmeida force-pushed the port-object-literal-diagnostics branch 5 times, most recently from e504b22 to 0bf3704 Compare January 21, 2020 20:00
… attempting literal result type variable binding and handle object literal in MissingArgumentsFailure
@LucianoPAlmeida LucianoPAlmeida force-pushed the port-object-literal-diagnostics branch from 43fbbab to f07e21b Compare January 21, 2020 23:41
@LucianoPAlmeida LucianoPAlmeida requested a review from xedin January 21, 2020 23:43
Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

LGTM! Once nit I left inline is addressed, we'll be ready to go.

@LucianoPAlmeida
Copy link
Contributor Author

@xedin Nit's fixed 👍

@xedin
Copy link
Contributor

xedin commented Jan 22, 2020

@swift-ci please smoke test

@slavapestov
Copy link
Contributor

Thank you for helping along with the new diagnostics effort!

@xedin
Copy link
Contributor

xedin commented Jan 22, 2020

+1! Thank you, @LucianoPAlmeida!

@xedin xedin merged commit 4b46043 into swiftlang:master Jan 22, 2020
@LucianoPAlmeida
Copy link
Contributor Author

Thank you for helping along with the new diagnostics effort!

No problem! Happy to help :)

Thank you for the reviews @xedin :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants