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

Give better error when matching too many fields #13062

Closed
wants to merge 1 commit into from

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Jul 13, 2021

Fixes #10757

else args
case (argType : AppliedType) :: Nil
if (defn.isTupleClass(argType.tycon.typeSymbol)) &&
(args.lengthCompare(1) > 0 && Feature.autoTuplingEnabled) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a lotta parens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Och yeah, we should remove those. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to move isTupleClass after args.lengthCompare since it is a more expensive test.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

It would be good to add a check file for i10757 that tests that the error message is correct.

The easiest way to do that is to generate a dummy check file i10757.check first and then follow instructions when running the test (it will suggest you a command to move the correct check file i10757.check.out that it generated to the actual check file i10757.check.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

It would be good to add a check file for i10757 that tests that the error message is correct.

The easiest way to do that is to generate a dummy check file i10757.check first and then follow instructions when running the test (it will suggest you a command to move the correct check file i10757.check.outthat it generated to the actual check filei10757.check`.

In fact #13063 seems to be one step closer to the finish line, so I think we can continue with this one.

else args
case (argType : AppliedType) :: Nil
if (defn.isTupleClass(argType.tycon.typeSymbol)) &&
(args.lengthCompare(1) > 0 && Feature.autoTuplingEnabled) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to move isTupleClass after args.lengthCompare since it is a more expensive test.

@tgodzik
Copy link
Contributor Author

tgodzik commented Jul 14, 2021

In fact #13063 seems to be one step closer to the finish line, so I think we can continue with this one.

I must say it's a bit discouraging that we worked on it during the spree for the effort to be for nothing. Would be nice to work on the communication aspect a bit, so that this doesn't happen again.

@tgodzik tgodzik closed this Jul 14, 2021
@tgodzik tgodzik deleted the fix-num-arg branch July 14, 2021 09:30
@odersky
Copy link
Contributor

odersky commented Jul 14, 2021

@tgodzik I was also surprised to see the double PR. Agreed we should avoid that in the future!

@dwijnand
Copy link
Member

I'm sorry that this happened, but communication was very literally the problem: it seemed Tomasz couldn't hear me in Spacial Chat even when my avatar was right on top of his! So I rage quit when I realised that all the comments I'd made were actually me speaking to myself for half an hour, like an idiot. It's a shame because I really like the concept of the chat, but it sucks that the implementation let's it down (including the CPU hogging...).

It is at least reassuring that we both reached the same conclusion on how to fix the issue though.

@tgodzik
Copy link
Contributor Author

tgodzik commented Jul 14, 2021

That explains what happened, I saw your avatar, but I thought that you might be muted on purpose and haven't realized you were talking to us. Glad to see the particular issue fixed, but next time we should make sure that we additionally communicate on the issue itself, I think that would help here and I haven't properly communicated there.

@som-snytt
Copy link
Contributor

That is a fraught story. Footnote, the other PR doesn't have "convert to pattern guard" with reduced parens. PR stands for "paren reduction". I was also curious whether the name-based symbol test or the type test is better in this context. Probably the type test in the other PR, but in such cases where there are 2 ways, it's not obvious if there is a downside (forcing info or inducing circularity).

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.

Give better error when matching too many fields
4 participants