Skip to content

Add an error message for "invalid unapply return type" error. #3501

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

Merged

Conversation

markarasev
Copy link

@markarasev markarasev commented Nov 17, 2017

Part of #1589.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

  1. Separate subject from body with a blank line
  2. When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:
  3. Limit the subject line to 72 characters
  4. Capitalize the subject line
  5. Do not end the subject line with a period
  6. Use the imperative mood in the subject line ("Added" instead of "Add")
  7. Wrap the body at 80 characters
  8. Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

@maseev
Copy link
Contributor

maseev commented Nov 18, 2017

Use the imperative mood in the subject line ("Added" instead of "Add")

I thought I already fixed this. It looks like the dotty-bot hasn't been rebuilt since then 😟

@markarasev markarasev force-pushed the 1589-unvalid-unapply-result branch from b7cab0f to 0a1bb70 Compare November 18, 2017 08:39
@markarasev markarasev changed the title Created an error message for "invalid unapply return type" error. Add an error message for "invalid unapply return type" error. Nov 18, 2017
@markarasev
Copy link
Author

I fixed the commit message =)

@senia-psm
Copy link
Contributor

senia-psm commented Nov 18, 2017

@markarasev This new feature is called Name Based Pattern. Please note there there is also a Product Pattern.
You can find discussion here.

@markarasev markarasev force-pushed the 1589-unvalid-unapply-result branch 2 times, most recently from a9b0243 to 776f25b Compare November 19, 2017 13:27
@markarasev
Copy link
Author

@senia-psm I updated the explanation to cover these cases.

@@ -113,6 +113,7 @@
IllegalStartOfStatementID,
TraitIsExpectedID,
TraitRedefinedFinalMethodFromAnyRefID,
InvalidUnapplyReturnTypeID,
Copy link
Contributor

@gosubpl gosubpl Nov 19, 2017

Choose a reason for hiding this comment

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

Or UnapplyInvalidReturnTypeID ? I am now struggling with the name for the other one from Applications.scala, and I tend to think that UnapplyInvalidNumberOfArgumentsID sounds better than InvalidUnapplyNumberOfArgumentsID. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

OK for me, I updated the names.

@@ -92,7 +93,7 @@ object Applications {
def getTp = extractorMemberType(unapplyResult, nme.get, pos)

def fail = {
ctx.error(i"$unapplyResult is not a valid result type of an $unapplyName method of an extractor", pos)
ctx.error(InvalidUnapplyReturnType(unapplyResult, unapplyName))
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the pos being passed here? (sorry for the probably naive question, I am just starting on those error messages)

Copy link
Author

Choose a reason for hiding this comment

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

I actually misunderstood this value's usage, fixed this.

@markarasev markarasev force-pushed the 1589-unvalid-unapply-result branch 2 times, most recently from c09b33d to 0240bf7 Compare November 24, 2017 09:32
@odersky
Copy link
Contributor

odersky commented Nov 27, 2017

In Dotty, Product subclasses are also legal unapply result types. So this specific PR is invalid. It would be good to

  • update the "Changes" section in the references to reflect the new rules, if that is not already the case
  • check whether the error message for types that are still illegal is correct, and update it if necessary.

@markarasev
Copy link
Author

@odersky do you mean that this part of the explanation is not explicit enough about Product subclasses?

To be used as an extractor, an unapply method has to return a type that either:

  • [...]
  • is a Product (like a "Tuple2[T1, T2]")

By "changes section in the references" do you mean these docs http://dotty.epfl.ch/docs/reference/changed/pattern-matching.html ? They seem to be up to date.

For your last point, do you mean I should add more unit tests?

@odersky
Copy link
Contributor

odersky commented Jan 12, 2019

@OlivierBlanvillain This has been inactive for a year. Let's merge or close this now.

@OlivierBlanvillain OlivierBlanvillain force-pushed the 1589-unvalid-unapply-result branch from 0240bf7 to 2ea1d87 Compare January 28, 2019 10:31
@OlivierBlanvillain OlivierBlanvillain merged commit 1948a5a into scala:master Feb 6, 2019
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.

7 participants