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

Detail UnapplyInvalidReturnType error message #17167

Merged
merged 1 commit into from
May 11, 2023

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Mar 28, 2023

Investigated during today's issue spree with @natsukagami and @TheElectronWill. Fixes #17077.

The reference documentation on “Product Match” states:

  • U <: Product
  • N > 0 is the maximum number of consecutive (val or parameterless def) _1: P1 ... _N: PN members in U
  • Pattern-matching on exactly N patterns with types P1, P2, ..., PN

From the second item:

  • empty products as return types for unapply are not accepted (N > 0)
  • a Product return type must have _1.._n members

Therefore, it seems that the current behavior is correct.

Side note: Product0 has been removed: #10398.

This PR adds 2 tests that confirm the current behavior, and adds details in the error message.

@mbovel mbovel requested a review from Sporarum March 28, 2023 21:09
@mbovel mbovel force-pushed the spree/better-unapply-message branch from 5dba7bb to 67314a6 Compare March 28, 2023 21:11
@mbovel mbovel requested a review from nicolasstucki March 28, 2023 21:27
@mbovel mbovel force-pushed the spree/better-unapply-message branch from 67314a6 to 8f044bf Compare March 28, 2023 23:01
Copy link
Contributor

@Sporarum Sporarum left a comment

Choose a reason for hiding this comment

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

Looks good to me ^^

The only thing is the link, we should probably have a broader discussion in how to integrate general/spec information into error messages that are necessarily smaller than them

(I am not saying we should remove the link, just that we should make sure at a later time that links are really what we want to use)

@ckipp01
Copy link
Member

ckipp01 commented May 9, 2023

The only thing is the link, we should probably have a broader discussion in how to integrate general/spec information into error messages that are necessarily smaller than them

(I am not saying we should remove the link, just that we should make sure at a later time that links are really what we want to use)

We're not utilizing them yet, but inside of DiagnosticCode there is an explanation field:

https://github.com/lampepfl/dotty/blob/274babf8cd6e1dcd5a14fed735570d20a6e2af3b/sbt-bridge/src/dotty/tools/xsbt/DiagnosticCode.java#L12

At least in LSP terms, this is typically a URI that leads to an error index where these types of things could also be explained. If we are going to start using links, it would be amazing to ensure they are structured in the places that they can be to start enabling using these types of features. However, if we were to do that, we'd need an error index first 😆

@mbovel
Copy link
Member Author

mbovel commented May 11, 2023

If that sounds okay to you, I will merge this PR as-is for now as it has a much narrower scope.

We will hopefully tackle more structured diagnosis in the future as a separate issue.

@mbovel mbovel merged commit 9642fb2 into scala:main May 11, 2023
@mbovel mbovel deleted the spree/better-unapply-message branch May 11, 2023 17:56
@Kordyjan Kordyjan added this to the 3.3.1 milestone Aug 1, 2023
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.

Non-specific message: "not a valid result type of an unapply method of an extractor"
4 participants