Skip to content

Conversation

@ennru
Copy link
Contributor

@ennru ennru commented Sep 25, 2017

@felixmulder Long time, no commit.

@nicolasstucki
Copy link
Contributor

@enru Will need to be rebased. Conflicts with the merge of your other PR

case class ReturnOutsideMethodDefinition()(implicit ctx: Context)
extends Message(ReturnOutsideMethodDefinitionID) {
val kind = "Syntax"
val msg = s"return outside method definition"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would turn the phrasing around and say that retrun must be inside a method definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the wording from the original, which is the same in scalac.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Though we can improve

val kind = "Syntax"
val msg = s"return outside method definition"
val explanation =
hl"${"return"} is a keyword and may only be used within methods"
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would explain where the return vas found (i.e. in ctx.owner if it is not NoContext)

extends Message(ReturnOutsideMethodDefinitionID) {
val kind = "Syntax"
val msg = hl"${"return"} outside method definition"
private val contextInfo =
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be a def

}
def enclMethInfo(cx: Context): (Tree, Type) = {
val owner = cx.owner
if (cx == NoContext || owner.isType) {
Copy link
Contributor

@allanrenucci allanrenucci Sep 26, 2017

Choose a reason for hiding this comment

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

@nicolasstucki I'm curious. Does it ever happen to have NoContext here?

@allanrenucci
Copy link
Contributor

Can your rebase on top of master. Also you can now define the message as follow:

  case class ReturnOutsideMethodDefinition(owner: Symbol)(implicit ctx: Context)
    extends Message(ReturnOutsideMethodDefinitionID) {
    val kind = "Syntax"
    val msg = hl"${"return"} outside method definition"
    val explanation =
      hl"""You used ${"return"} in ${owner.show}".
          |${"return"} is a keyword and may only be used within method declarations.
          |"""
  }

@ennru
Copy link
Contributor Author

ennru commented Sep 27, 2017

So I understand, the NoContext case does not exist in practice?

@allanrenucci
Copy link
Contributor

So I understand, the NoContext case does not exist in practice?

No, cx should never be NoContext. It was fixed in #3188

@ennru
Copy link
Contributor Author

ennru commented Sep 27, 2017

Sorry, looks like git and I didn't come along that well.
I'll make a new clean PR.

@ennru ennru closed this Sep 27, 2017
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.

5 participants