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

Add error message - Parsers.scala:1180 #1613

Merged
merged 1 commit into from
Oct 23, 2016
Merged

Add error message - Parsers.scala:1180 #1613

merged 1 commit into from
Oct 23, 2016

Conversation

jyotman
Copy link

@jyotman jyotman commented Oct 19, 2016

Part of #1589.
Explanation for the error on line Parsers.scala:1180.
Adds the sematic object for the error illiegal start of simple expression.
@felixmulder kindly review.

@@ -318,4 +318,13 @@ object messages {
|$code2
|""".stripMargin
}

case class IllegalExpressionStart()(implicit ctx: Context) extends Message(14) {
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 call this something like: IllegalStartSimpleExpr since it is simple expressions and not all valid expressions. I'm assuming you chose 14 to coordinate with Shane's PR? :)

val msg = "illegal start of simple expression"
val explanation = {
hl"""|The expression you've provided is not valid because
|an expression in Scala always returns a value of some type""".stripMargin
Copy link
Contributor

Choose a reason for hiding this comment

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

If the explanation is printed, the user is most likely a beginner. In this case we need to educate them by providing a hint as to how they can make their program valid. If I don't know that expressions are things which return values then I, the user, need to be given some examples.

This error also occurs frequently when a user has forgotten a parenthesis somewhere which makes it sort of tricky to give a 100% concrete solution in this case. Perhaps in the future we could have some machinery to discover this.

I would try to write something similar to:

An expression yields a value. In the case of the simple expression, this error
commonly occurs when there's a missing parenthesis or brace. The reason being
that a simple expression is one of the following:

- Block
- Expression in parenthesis
- Identifier
- Object creation
- Literal

Which cannot start with a `}` or a `)`.

Copy link
Author

@jyotman jyotman Oct 19, 2016

Choose a reason for hiding this comment

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

Hey so I'm trying to get the illegal string so that the above explanation's last line would read like -
Which cannot start with a //the illegal string comes here.

Now to get this string in Parsers.scala I'm gonna have to use the tokenString() method -
syntaxErrorOrIncomplete(IllegalStartSimpleExpr(tokenString(in.token)))

What do you think of this? Will tokenString() method always return a value or could it produce an exception?

if my usage is fine then currently it looks like this -

scala> val a = ;
-- [E014] Syntax Error: <console> --------------------------------------------------------------------------------------
1 |val a = ;
  |        ^
  |        illegal start of simple expression

Explanation
===========
An expression yields a value. In the case of the simple expression, this error
commonly occurs when there's a missing parenthesis or brace. The reason being
that a simple expression is one of the following:

- Block
- Expression in parenthesis
- Identifier
- Object creation
- Literal

which cannot start with ';'.

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't produce an exception if the token is defined no - so it shouldn't be a problem. Good thinking on including user code 👍

@felixmulder
Copy link
Contributor

Also a quick note on commit messages, we prefer the form:

Add explanation for "illegal start of simple expression"

instead of:

Added explanation for "illegal start of simple expression"

:)

@felixmulder
Copy link
Contributor

Otherwise things are looking great 👍

@jyotman
Copy link
Author

jyotman commented Oct 19, 2016

Thanks @felixmulder all points noted :) I'll try to give a better explanation using the one you've suggested. Do you want me to include code samples also to explain both - probable wrong ways and the correct ways? But there could be many examples for that.

Also yes I kept the message number 14 to coordinate with Shane's PR.

@felixmulder
Copy link
Contributor

@jyotman94 - hmm, yeah I don't know, that's really subjective right? IMHO this explanation probably doesn't need any code examples - but you know, have fun with it and experiment :)

@jyotman jyotman changed the title Added error message - Parsers.scala:1180 Add error message - Parsers.scala:1180 Oct 19, 2016
@jyotman
Copy link
Author

jyotman commented Oct 20, 2016

@felixmulder I've made the changes. Kindly review. Also I've resolved conflicts with Shane's #1611 .

@@ -400,4 +400,22 @@ object messages {
| ((${nestedRepresentation}))""".stripMargin
}
}

case class IllegalStartSimpleExpr(illegalString: String)(implicit ctx: Context) extends Message(14) {
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 rename illegalString to illegalToken :)

@felixmulder
Copy link
Contributor

So when I said the the git commit should be in "imperative form" I meant do a rebase and reword the commit. If you didn't know how, it can be done like this:

$ git remote add upstream [email protected]:lampepfl/dotty.git
$ git fetch upstream
$ git rebase -i upstream/master

Then choose to reword your commit message and call it something like: Add explanation for "illegal start of simple expression". Makes sense? After that everything looks good to me, great job!

@jyotman
Copy link
Author

jyotman commented Oct 20, 2016

Sorry, actually I got confused whether you referred to my commit message or the pull request title earlier so I had just renamed the pull request title. I've made the new changes. Thank you for being so patient :)

@felixmulder
Copy link
Contributor

No problem - thank you for putting in all the hard work :)

Once this passes CI, I'll merge it - cheers!

@felixmulder
Copy link
Contributor

Hey @jyotman94 - could you rebase so I can merge? :) (Don't forget to check that your errorId is correct)

@jyotman
Copy link
Author

jyotman commented Oct 23, 2016

@felixmulder done!

@felixmulder felixmulder merged commit cdb83f9 into scala:master Oct 23, 2016
@felixmulder
Copy link
Contributor

Great! Thanks a lot for the PR :)

@jyotman jyotman deleted the error_message_Parsers.scala-1180 branch October 23, 2016 12:37
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