Skip to content
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

Improve warning for inferring an undesirable type #27797

Merged
merged 2 commits into from Nov 10, 2019

Conversation

jder
Copy link
Contributor

@jder jder commented Oct 19, 2019

Adds a fixit for the warning about inferring a probably-unexpected type, and adds another case for when inferring a type of [()]. It would be easy to also warn for arrays of any of the existing cases but this seemed like overkill and would require some additional messages.

Taken together, this:

let y = x.map { print($0) }

Now produces:

warning: variable 'y' inferred to have type '[()]'
add an explicit type annotation to silence this warning

With a fixit to add : [()] after y.

Resolves SR-11511.

Copy link
Collaborator

@theblixguy theblixguy left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@theblixguy
Copy link
Collaborator

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator

You’ve got one failing SourceKit test which needs to be updated

@jder jder force-pushed the improve-undesirable-warning branch from df239ce to 6aa0634 Compare October 20, 2019 01:30
@theblixguy
Copy link
Collaborator

@swift-ci please smoke test

@jder
Copy link
Contributor Author

jder commented Oct 20, 2019

Thanks, sorry about that. I believe it should be fixed now, but I was just trying to verify locally before pinging you again. Is check-swift the full set that CI runs?

@theblixguy
Copy link
Collaborator

No worries... it runs primary tests and validation tests, so check-swift-validation.

@jrose-apple
Copy link
Contributor

cc @hborla @xedin too

@jder jder force-pushed the improve-undesirable-warning branch 2 times, most recently from 94ab6bf to bf0b84b Compare October 24, 2019 01:41
@jder
Copy link
Contributor Author

jder commented Oct 25, 2019

OK, I just used the version in ConstraintSystem since they're both in sema. If that seems ok, I think this is ready to merge. (cc @theblixguy)

@theblixguy
Copy link
Collaborator

theblixguy commented Oct 25, 2019

Hmm feels a little strange to import the ConstraintSystem just for a small check, but fine, we can change it when #27195 lands...or we can just duplicate the check here. What do others think?

@jder
Copy link
Contributor Author

jder commented Oct 25, 2019

Yeah, it's true. I'm happy to change it when the other PR lands, or just copy-paste the code for now if that's preferable.

@jrose-apple
Copy link
Contributor

I was going to say it's simple enough to just expand inline, but this is fine too.

@jder
Copy link
Contributor Author

jder commented Oct 26, 2019

OK, then I think this is ready to merge after another smoke test run.

@theblixguy
Copy link
Collaborator

@swift-ci please smoke test

@theblixguy theblixguy requested a review from xedin November 6, 2019 02:03
lib/Sema/TypeCheckPattern.cpp Outdated Show resolved Hide resolved
@jder jder force-pushed the improve-undesirable-warning branch from bf0b84b to 502691f Compare November 9, 2019 20:43
@jder
Copy link
Contributor Author

jder commented Nov 9, 2019

Thanks, all. I just made the requested change, merged with master and ran check-swift-validation locally if someone (maybe @theblixguy or @xedin) could do a last check & merge.

@theblixguy
Copy link
Collaborator

@swift-ci please smoke test

@jder
Copy link
Contributor Author

jder commented Nov 10, 2019

Seems like it passed! Would you be able to merge it @theblixguy?

@theblixguy theblixguy merged commit f5f214d into apple:master Nov 10, 2019
@theblixguy
Copy link
Collaborator

Thank you for your contribution! Please don’t forget to mark the JIRA ticket as resolved :)

@jder
Copy link
Contributor Author

jder commented Nov 10, 2019

Thank you as well!

@BasThomas
Copy link
Contributor

Awesome!

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.

None yet

5 participants