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

Removes MismatchedCount argument swapping workaround #103

Merged
merged 3 commits into from
Nov 4, 2021

Conversation

cipharius
Copy link
Contributor

It is not clear why this workaround exists here, but it seems to be outdated as of now. Instead of correcting the error message at the return site, it produces an incorrect message:

image

Unfortunately I could not run the test suite due to build error that I could not resolve. I will create an issue for this error.

@andyfriesen
Copy link
Collaborator

Well spotted!

Do you think you could toss a unit test into TypeInfer.test.cpp? It should be pretty simple to write.

Thanks!

@cipharius
Copy link
Contributor Author

As I was trying to figure out what is expected/actual in each context, I found some other improvements to make to the MismatchedType error messages.

I will look into the CI errors, I think the MismatchedCount::Result case actually does need the swapping logic.

@cipharius cipharius force-pushed the bugfix/swapped-mismatched-count branch from 9498a00 to b18fd1c Compare November 3, 2021 21:14
@zeux zeux requested a review from andyfriesen November 3, 2021 21:16
@cipharius
Copy link
Contributor Author

cipharius commented Nov 3, 2021

The last error isn't related to this swapping issue anymore. It just became appearant because of the unit test improvements.

Edit:
Just realized that error originates from the newly added test case. I added it because of an error message branch in Error.cpp. Will add comment to the line in question.

@zeux
Copy link
Collaborator

zeux commented Nov 3, 2021

@cipharius I believe we currently intentionally type check the program with ignored values because in some cases it's intentional, and some functions esp. in standard library may return more arguments than you realize. So this test should probably check for no errors and have a comment that it's okay to have the right hand side of the assignment to produce more values than the left hand side does.

Analysis/src/Error.cpp Outdated Show resolved Hide resolved
@cipharius
Copy link
Contributor Author

This last commit implements the suggestion made by @zeux and leaves a note on why only required values > returned values case is considered.

After the review, should I squash the commits into one, or do you prefer them to be separate?

@zeux
Copy link
Collaborator

zeux commented Nov 3, 2021

There's no need to squash the commits manually - we have the repository set up so that squashes are enforced during PR merge.

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thanks!

@zeux zeux merged commit 344d37f into luau-lang:master Nov 4, 2021
@cipharius cipharius deleted the bugfix/swapped-mismatched-count branch November 8, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants