Skip to content

[Property Wrappers] Improve diagnostics for property wrappers initialized out-of-line #32672

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
merged 6 commits into from
Jul 8, 2020

Conversation

hborla
Copy link
Member

@hborla hborla commented Jul 2, 2020

  • Type check out-of-line initialized property wrappers via SolutionApplicationTarget with fixes enabled in the constraint system to produce targeted diagnostics for specific failures, rather than the vague property type does not match that of the 'wrappedValue' property of its wrapper type error message. This error message is now a fallback diagnostic for property wrappers.
  • Remove code duplication for generating constraints between the different forms of attached property wrappers.
  • Add tailored diagnostics for when the wrappedValue type doesn't match the property or composed property wrapper type.

Resolves: rdar://problem/54491433

initialized wrapped property via SolutionApplicationTarget.
@hborla hborla requested a review from DougGregor July 2, 2020 03:04
@hborla
Copy link
Member Author

hborla commented Jul 2, 2020

@swift-ci please smoke test

1 similar comment
@hborla
Copy link
Member Author

hborla commented Jul 2, 2020

@swift-ci please smoke test

hborla added 5 commits July 2, 2020 14:47
anchored at a TypeRepr rather than an Expr.
via SolutionApplicationTarget. This allows fixes to be applied and diagnosed
for better error messages in the case of failures, and removes code
duplication for generating property wrapper constraints.
…ppers

to produce better diagnostics when there's a 'wrappedValue' type mismatch.
handle all errors with a property wrapper backing type.
@hborla hborla force-pushed the property-wrapper-diagnostics branch from b7cdce0 to 9b4f1f0 Compare July 2, 2020 21:48
@hborla
Copy link
Member Author

hborla commented Jul 2, 2020

@swift-ci please smoke test

1 similar comment
@hborla
Copy link
Member Author

hborla commented Jul 7, 2020

@swift-ci please smoke test

@hborla
Copy link
Member Author

hborla commented Jul 8, 2020

@swift-ci please test Windows Platform

@hborla hborla merged commit 063d420 into swiftlang:master Jul 8, 2020
@hborla hborla deleted the property-wrapper-diagnostics branch July 8, 2020 02:08
@@ -11,5 +11,5 @@ struct B {
}

struct C {
@A @B var foo: Int // expected-error{{extraneous argument label 'wrappedValue:' in call}}
@A @B var foo: Int // expected-error{{composed wrapper type 'B' does not match former 'wrappedValue' type 'Int'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this diagnostic... what are the two types facing a mismatch? Which wrappedValue is former 'wrappedValue' referring to here?

Copy link
Member Author

@hborla hborla Jul 8, 2020

Choose a reason for hiding this comment

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

For property wrapper composition @A @B the type of A.wrappedValue (which in this case is Int) must be equal to B. The generalization is that a composed wrapper type must match the wrappedValue type of the previous property wrapper. Does that make sense?

I would love to workshop this error message, and an educational note would be great too!

Copy link
Member Author

@hborla hborla Jul 8, 2020

Choose a reason for hiding this comment

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

I also want to add a note for this error message to have a note pointing to the wrappedValue declaration, but I need to add a new locator to support that so I figured it'd be better to do that in a separate PR. A new locator would also allow us to write the former property wrapper type in the error message itself

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that makes sense. I agree we should have some mention of A here; since wrappedValue itself is the standard name, it doesn't provide much context. For example (straw-man suggestion)

composed wrapper type 'B' does not match the type of 'A.wrappedValue' which is 'Int'
note: in a property wrapper composition, the type of an outer property wrapper's wrappedValue must match the type of the inner property wrapper

Addressing this in a subsequent PR SGTM. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that suggestion, thank you!

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.

3 participants