Improved error handling for composed property wrapper mismatches #36732

Strieker · 2021-04-02T21:45:20Z

The purpose of this pull request is to remedy a confusing error associated with wrapped value type mismatches to give more context for the wrapped value type in the error message. So that when running this block of code:

@propertyWrapper struct Wrapper<Value> {
    var wrappedValue: Value
}


struct User {
    @Wrapper<String> @Wrapper var value: Int
}

the new error message for the above example would be

composed wrapper type 'Wrapper<Int>' does not match the type of 'Wrapper<String>'.wrappedValue, which is 'String'

Should this change not have been made, the error would have looked like the following:

composed wrapper type 'Wrapper<Int>' does not match former 'wrappedValue' type 'String'

which is rather confusing.

NOTE: There is an existing diagnostic, called wrapped_value_mismatch that should be combine with this diagnostic, called composed_property_wrapper_mismatch. There are too many separate modifications to include in this PR in order to combine these two diagnostics, so that will be done in a separate PR.

Resolves:
https://bugs.swift.org/browse/SR-14130

Thank you @hborla for all of your help with this!

xedin

Overall looks great! I have left a couple comments about documentation and code but logic-wise this is on point.

include/swift/Sema/ConstraintLocator.h

lib/Sema/CSDiagnostics.cpp

lib/Sema/CSFix.cpp

include/swift/Sema/CSFix.h

include/swift/Sema/ConstraintLocator.h

include/swift/Sema/ConstraintLocatorPathElts.def

Strieker · 2021-04-05T21:01:57Z

Overall looks great! I have left a couple comments about documentation and code but logic-wise this is on point.

Thank you so much! I really appreciate your comments I will definitely go back and implement the changes you suggest :)

xedin

A few more small changes.

xedin · 2021-04-07T18:02:51Z

lib/Sema/CSDiagnostics.cpp

+  auto elt = locator->castLastElementTo<LocatorPathElt::WrappedValue>();
+
+  emitDiagnostic(diag::composed_property_wrapper_mismatch,
+                 resolveType(getFromType()), elt.getType(), getToType());


Nit: getFromType() would call resolve internally so there is no need to call it again. But I think we need to call that for elt.getType()?

I agree. I did some investigating, and it turns out it's not required to use resolveType for any of the three function calls, so I went on and got rid of it all together, I hope that was okay.

I think it is required on the element because element would get a type that might have type variables in it, so we'd end up printing _ instead of an actual type type variable was bound to.

Oh that makes sense, I will fix that!

lib/Sema/CSGen.cpp

xedin · 2021-04-07T18:09:10Z

lib/Sema/CSSimplify.cpp

+
+  case FixKind::AllowWrappedValueMismatch: {
+    if (recordFix(fix)) return SolutionKind::Error;
+      return SolutionKind::Solved;


Nit: formatting is incorrect here, I'd suggest to use a ternary - return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;

lib/Sema/ConstraintLocator.cpp

Strieker · 2021-04-07T23:13:35Z

A few more small changes.
Ill get to working on these, thank you for the feedback :)

xedin · 2021-04-07T23:16:05Z

Happy to help, thank you for making this work!

Strieker · 2021-04-08T02:26:35Z

Happy to help, thank you for making this work!

Of course, it's been awesome to have the chance to work on this project, I've been learning so much!

lib/Sema/CSSimplify.cpp

lib/Sema/CSDiagnostics.cpp

xedin

Perfect!

xedin · 2021-04-12T17:23:38Z

@swift-ci please smoke test

Strieker · 2021-04-12T17:54:53Z

Perfect!

Awesome, thank you!!!! In that case then, in regards to @LucianoPAlmeida's comment, @xedin, should we make the removal of CTP_ComposedPropertyWrapper a project for another PR, or attempt to delete it in this one?

xedin · 2021-04-12T17:57:30Z

in regards to @LucianoPAlmeida's comment, @xedin, should we make the removal of CTP_ComposedPropertyWrapper a project for another PR, or attempt to delete it in this one?

Apparently I didn't press "Comment" :/ I think it's okay to clean that up separately.

xedin · 2021-04-12T17:57:50Z

@swift-ci please smoke test Windows platform

xedin · 2021-04-12T18:09:06Z

@swift-ci please smoke test Windows platform

xedin · 2021-04-12T18:47:06Z

@Strieker Looks like these changes regressed validation-test/compiler_crashers_2_fixed/sr11599.swift, you'd have to take a look at the before we can merge.

hborla · 2021-04-12T18:50:05Z

Looks like these changes regressed validation-test/compiler_crashers_2_fixed/sr11599.swift, you'd have to take a look at the before we can merge.

@Strieker you can run the validation tests locally by passing --validation-test to build-script. Looks like the test just needs to be updated with the new error message!

xedin · 2021-04-12T18:51:37Z

you can run the validation tests locally by passing --validation-test to build-script

Or -T which would run both tests and validation-tests.

Strieker · 2021-04-12T20:19:01Z

in regards to @LucianoPAlmeida's comment, @xedin, should we make the removal of CTP_ComposedPropertyWrapper a project for another PR, or attempt to delete it in this one?

Apparently I didn't press "Comment" :/ I think it's okay to clean that up separately.

No problem, sounds good! Thank you :)

Strieker · 2021-04-12T20:21:21Z

you can run the validation tests locally by passing --validation-test to build-script

Or -T which would run both tests and validation-tests.

@xedin and @hborla I will make sure to get on that, thank you for telling me how to work on fixing these validation tests :)

Strieker · 2021-04-13T03:06:02Z

you can run the validation tests locally by passing --validation-test to build-script

Or -T which would run both tests and validation-tests.

@xedin and @hborla I will make sure to get on that, thank you for telling me how to work on fixing these validation tests :) Also as a heads up, my .zshrc file is currently corrupted/something is wrong with it. I can't run ls, git, or anything else from terminal right now at the moment. I have to talk to a tech support person or something to figure out what's wrong, since I've been debugging for a hot minute to try and figure out what went wrong, so it might be sometime tomorrow before I can run the commands you told me to run, sorry about that.

xedin · 2021-04-13T03:19:54Z

No rush! You can try running /bin/sh from your terminal to see whether it would allow you to run commands again, if so - it’s something with your PATH. Run env to see what bindings you currently have and unbind broken variables.

Strieker · 2021-04-13T17:38:01Z

No rush! You can try running /bin/sh from your terminal to see whether it would allow you to run commands again, if so - it’s something with your PATH. Run env to see what bindings you currently have and unbind broken variables.

Thank you so, so much for this advice! Yes, I was assuming it was something having to do with my PATH, I will try your suggestions and see what happens, because I've been trying to change my PATH and I can't seem to get it working.

… can recognize a composed property wrapper’s wrapped value type.

… wrapper mismatch in DiagnosticsSema.def and made changes to show the new composed property wrapper mismatch type diagnostic in both CSDiagnostics.cpp and CSDiagnostics.h.

…here the wrappedValue type of a property wrapper was determined and added necessary logic so that a constraint locator can recognize a composed property wrapper’s wrapped value type in CSFix.cpp, CSFix.h, and CSSimplify.cpp.

…est in property_wrappers.swift

…to improve error handling for composed wrapped value mismatches

… composed property werapper mismatches"

LucianoPAlmeida · 2021-04-23T12:53:22Z

@swift-ci Please smoke test

hborla requested review from hborla and xedin April 2, 2021 21:48

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch 3 times, most recently from 9d4b447 to ee228a4 Compare April 3, 2021 17:18

xedin approved these changes Apr 5, 2021

View reviewed changes

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from 015219c to 36ed399 Compare April 7, 2021 00:45

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from bae7b89 to 7e0ca2c Compare April 7, 2021 03:41

Strieker requested a review from xedin April 7, 2021 04:34

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from 417a589 to 349a5f5 Compare April 7, 2021 05:13

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from 23746db to f3c7592 Compare April 7, 2021 05:25

xedin approved these changes Apr 7, 2021

View reviewed changes

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from 910b739 to 45b786e Compare April 8, 2021 02:26

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from d9cf79d to 6034e35 Compare April 8, 2021 04:06

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from 231f175 to aed8c6c Compare April 8, 2021 05:38

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from 7920b64 to e11f58d Compare April 8, 2021 06:34

Strieker requested a review from xedin April 8, 2021 06:41

Strieker requested a review from xwu April 10, 2021 19:19

LucianoPAlmeida reviewed Apr 10, 2021

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

lib/Sema/CSDiagnostics.cpp Show resolved Hide resolved

Strieker requested a review from LucianoPAlmeida April 10, 2021 23:46

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from ee9ece4 to 572a0c9 Compare April 10, 2021 23:47

xedin approved these changes Apr 12, 2021

View reviewed changes

Strieker added 7 commits April 22, 2021 11:31

[ConstraintSystem] Added necessary logic so that a constraint locator…

3891dc7

… can recognize a composed property wrapper’s wrapped value type.

[Diagnostics] Added new diagnostic error string for composed property…

11efb7d

… wrapper mismatch in DiagnosticsSema.def and made changes to show the new composed property wrapper mismatch type diagnostic in both CSDiagnostics.cpp and CSDiagnostics.h.

[NFC] Fixed and added property wrapper tests.

b732319

[ConstraintSystem] Modified CSGen.cpp in order to resolve a failing t…

b3f0442

…est in property_wrappers.swift

[NFC] Resolved merge conflict in CSSimplify.cpp after making changes …

4e2b67c

…to improve error handling for composed wrapped value mismatches

"modified existing validation test after improving error handling for…

72fc506

… composed property werapper mismatches"

Strieker force-pushed the composed_property_wrapper_improved_error_handling branch from 076be0c to 72fc506 Compare April 23, 2021 08:13

xedin approved these changes Apr 23, 2021

View reviewed changes

xedin merged commit 5a7fd68 into swiftlang:main Apr 23, 2021

Improved error handling for composed property wrapper mismatches #36732

Improved error handling for composed property wrapper mismatches #36732

Uh oh!

Conversation

Strieker commented Apr 2, 2021 • edited by xedin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Strieker commented Apr 5, 2021

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Strieker Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

xedin Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

Strieker Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xedin Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Strieker commented Apr 7, 2021

Uh oh!

xedin commented Apr 7, 2021

Uh oh!

Strieker commented Apr 8, 2021

Uh oh!

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Apr 12, 2021

Uh oh!

Strieker commented Apr 12, 2021

Uh oh!

xedin commented Apr 12, 2021

Uh oh!

xedin commented Apr 12, 2021

Uh oh!

xedin commented Apr 12, 2021

Uh oh!

xedin commented Apr 12, 2021

Uh oh!

hborla commented Apr 12, 2021

Uh oh!

xedin commented Apr 12, 2021

Uh oh!

Strieker commented Apr 12, 2021

Uh oh!

Strieker commented Apr 12, 2021

Uh oh!

Strieker commented Apr 13, 2021

Uh oh!

xedin commented Apr 13, 2021

Uh oh!

Strieker commented Apr 13, 2021

Uh oh!

LucianoPAlmeida commented Apr 23, 2021

Uh oh!

Strieker commented Apr 2, 2021 •

edited by xedin

Loading