Skip to content

Add error message for only functional erased or implicit types #7848

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

Conversation

hope-doe
Copy link

No description provided.

@hope-doe hope-doe mentioned this pull request Dec 25, 2019
@anatoliykmetyuk anatoliykmetyuk self-assigned this Jan 6, 2020
Copy link
Contributor

@anatoliykmetyuk anatoliykmetyuk left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, the CI failure doesn't seem to be related.

@@ -1423,9 +1423,9 @@ object Parsers {
case FORSOME => syntaxError(ExistentialTypesNoLongerSupported()); t
case _ =>
if (imods.isOneOf(GivenOrImplicit) && !t.isInstanceOf[FunctionWithMods])
syntaxError("Types with implicit keyword can only be function types `implicit (...) => ...`", implicitKwPos(start))
syntaxError(ImplicitTypesCanOnlyBeFunctionTypes())
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nicer when the error message highlights the keyword in question rather than just points to the location after the erroneous type.

Suggested change
syntaxError(ImplicitTypesCanOnlyBeFunctionTypes())
syntaxError(ImplicitTypesCanOnlyBeFunctionTypes(), implicitKwPos(start))

if (imods.is(Erased) && !t.isInstanceOf[FunctionWithMods])
syntaxError("Types with erased keyword can only be function types `erased (...) => ...`", implicitKwPos(start))
syntaxError(ErasedTypesCanOnlyBeFunctionTypes())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
syntaxError(ErasedTypesCanOnlyBeFunctionTypes())
syntaxError(ErasedTypesCanOnlyBeFunctionTypes(), implicitKwPos(start))

case class ImplicitTypesCanOnlyBeFunctionTypes()(implicit val ctx: Context)
extends Message(ImplicitTypesCanOnlyBeFunctionTypesID) {
val kind: String = "Syntax"
val msg: String = "Types with implicit keyword can only be function types `given (...) => ...`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val msg: String = "Types with implicit keyword can only be function types `given (...) => ...`"
val msg: String = "Types with given keyword can only be function types `given (...) => ...`"

implicit val ctx: Context = ictx
assertMessageCount(1, messages)
val ImplicitTypesCanOnlyBeFunctionTypes() :: Nil = messages
assertEquals("Types with implicit keyword can only be function types `given (...) => ...`", messages.head.msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertEquals("Types with implicit keyword can only be function types `given (...) => ...`", messages.head.msg)
assertEquals("Types with given keyword can only be function types `given (...) => ...`", messages.head.msg)

@anatoliykmetyuk
Copy link
Contributor

Thank you for making the effort to improve the codebase @hope-doe! For us to merge this, could you sign the CLA here?

@hope-doe hope-doe force-pushed the issue-1589-error-mes-implicit-erased-types-can-only-be-ft branch from f6345da to 37dcf9b Compare January 13, 2020 11:32
@hope-doe
Copy link
Author

hope-doe commented Jan 13, 2020

@anatoliykmetyuk I've signed the CLA, but still see here "CLA Expected - Waiting for status to be reported " is it okay? or something more needed from my side?
And 'erased' test now fails, since you already saw my test case (and it passed earlier) could you please clarify is it bug or mb my wrong understanding of how it should look

@anatoliykmetyuk
Copy link
Contributor

anatoliykmetyuk commented Jan 14, 2020

And 'erased' test now fails, since you already saw my test case (and it passed earlier) could you please clarify is it bug or mb my wrong understanding of how it should look

That's because we've recently made the erased an opt-in feature hidden under a flag. Now you need to call the compiler with -Yerased-terms to enable this keyword. Since one of your tests uses it, it now fails.

To run that test with that flag, locate the initializeCtx method here:

https://github.com/lampepfl/dotty/blob/4a514126d87528a6d03364e461d5f409fe714bfe/compiler/test/dotty/tools/DottyTest.scala#L42-L46

And add the following to it:

fc.setSetting(fc.settings.YerasedTerms, true)

To see if your tests pass before submitting them to CI, use from SBT:

testOnly dotty.tools.dotc.reporting.ErrorMessagesTests -- *TypesCanOnlyBeFunctionTypes*

@anatoliykmetyuk I've signed the CLA, but still see here "CLA Expected - Waiting for status to be reported " is it okay? or something more needed from my side?

The status won't update on this PR page. You can check your status here. However, it says false for some reason. Have you signed the Lightbend or Scala CLA? Have you signed it with your GitHub account?

@hope-doe hope-doe force-pushed the issue-1589-error-mes-implicit-erased-types-can-only-be-ft branch from 37dcf9b to c3a72f7 Compare January 15, 2020 13:23
@hope-doe
Copy link
Author

no idea what problem was with my first attempt, but now sign status is true
thank you for all the help!

@anatoliykmetyuk anatoliykmetyuk merged commit c79ffa1 into scala:master Jan 15, 2020
@anatoliykmetyuk
Copy link
Contributor

LGTM, thanks @hope-doe!

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.

2 participants