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

correctly parse enum values #104

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ install:
- . $HOME/.nvm/nvm.sh
- nvm install 8
- nvm use 8
- npm install typescript@2.6.2
- npm install typescript@3.5.2

cache:
directories:
Expand Down
12 changes: 12 additions & 0 deletions samples/valueExpression.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
declare module valueExpression {
export enum StringEnum {
A = "test",
B = A,
C = "test" + "test"
}
export enum NumberEnum {
A = 3 + 3,
B = 1 << 3,
C = 10**2+1*8- - -3/3>>2<<3>>>+19%~~~3+ + +9
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Please make sure there is a (single) new line at the end of the file, so that git and GitHub don't have that mark.

40 changes: 40 additions & 0 deletions samples/valueExpression.d.ts.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@

import scala.scalajs.js
import js.annotation._
import js.|

package valueExpression {

package valueExpression {

@js.native
sealed trait StringEnum extends js.Object {
}

@js.native
@JSGlobal("valueExpression.StringEnum")
object StringEnum extends js.Object {
var A: StringEnum = js.native
var B: StringEnum = js.native
var C: StringEnum = js.native
@JSBracketAccess
def apply(value: StringEnum): String = js.native
}

@js.native
sealed trait NumberEnum extends js.Object {
}

@js.native
@JSGlobal("valueExpression.NumberEnum")
object NumberEnum extends js.Object {
var A: NumberEnum = js.native
var B: NumberEnum = js.native
var C: NumberEnum = js.native
@JSBracketAccess
def apply(value: NumberEnum): String = js.native
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class TSDefLexical extends Lexical with StdTokens with ImplicitConversions {
digits => digits.foldLeft(0L)(_ * 8 + _).toString
}
)
| opt('-') ~ stringOf1(digit) ~ opt(stringOf1('.', digit)) ^^ {
case sign ~ part1 ~ part2 => sign.getOrElse("") + part1 + (part2.getOrElse(""))
| stringOf1(digit) ~ opt(stringOf1('.', digit)) ^^ {
case part1 ~ part2 => part1 + part2.getOrElse("")
}
) ^^ NumericLit

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,18 @@ class TSDefParser extends StdTokenParsers with ImplicitConversions {
"...", "=>"
)

// for value expressions
val binaryValueOperators = Set(
"+", "-", "*", "/", "**", "%",
"<<", ">>>", ">>", "&", "|", "^"
Copy link
Owner

Choose a reason for hiding this comment

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

These 2 lines should be indented with 4 spaces.

Copy link
Owner

Choose a reason for hiding this comment

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

And given my comments below, consider just including those directly inside lexical.delimiters ++= above.

)
lexical.delimiters ++= binaryValueOperators

val unaryValueOperators = Set(
"+", "-", "~"
Copy link
Owner

Choose a reason for hiding this comment

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

This line should be indented with 4 spaces.

)
lexical.delimiters ++= unaryValueOperators

def parseDefinitions(input: Reader[Char]) =
phrase(ambientDeclarations)(new lexical.Scanner(input))

Expand Down Expand Up @@ -105,7 +117,20 @@ class TSDefParser extends StdTokenParsers with ImplicitConversions {
"enum" ~> typeName ~ ("{" ~> ambientEnumBody <~ "}") ^^ EnumDecl

lazy val ambientEnumBody: Parser[List[Ident]] =
repsep(identifier <~ opt("=" ~ (numericLit | stringLit) ), ",") <~ opt(",")
repsep(identifier <~ opt("=" ~! valueExpression), ",") <~ opt(",")
Copy link
Owner

Choose a reason for hiding this comment

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

What are the ...! operators?

Copy link
Author

Choose a reason for hiding this comment

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

they are non backtracking operators. basically, after the = is parsed here the parser will not go back and try other alternatives prior to = when an error comes afterwards. this is very important to get better error locations when parsing fails, because we know that when we arrived after the = there is no other rule that could parse this. so there is no point in going back and try to parse a class declaration instead of the enum declaration, for example. Instead we just stop and show the much more accurate error location.

I was already about to make a pr which added many more of these operators but it's not always totally clear where to introduce these cuts while preserving the semantics and I want to make sure that I am correct. I will make the pr in a few days


lazy val valueExpression: Parser[_] =
rep1sep(success() ~>! rep(unaryValueOperator) ~
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the purpose of success() ~>! . Could you enlighten me?

(numericLit | stringLit | identifierName | parenthesizedValueExpression), binaryValueOperator)

lazy val parenthesizedValueExpression =
"(" ~! valueExpression ~ ")"

lazy val binaryValueOperator =
elem("binary operator", tok => binaryValueOperators.contains(tok.chars))
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather you use a more direct style, like

Suggested change
elem("binary operator", tok => binaryValueOperators.contains(tok.chars))
"+" | "-" | "*" | "/" | "**" | "%" | "<<" | ">>>" | ">>" | "&" | "|" | "^"

Copy link
Author

Choose a reason for hiding this comment

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

may I ask why? in the current version the operators occur only once instead of twice at different locations


lazy val unaryValueOperator =
elem("unary operator", tok => unaryValueOperators.contains(tok.chars))
Copy link
Owner

Choose a reason for hiding this comment

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

Same here:

Suggested change
elem("unary operator", tok => unaryValueOperators.contains(tok.chars))
"+" | "-" | "~"


lazy val ambientClassDecl: Parser[DeclTree] =
(abstractModifier <~ "class") ~ typeName ~ tparams ~ classParent ~ classImplements ~ memberBlock <~ opt(";") ^^ {
Expand Down Expand Up @@ -342,8 +367,8 @@ class TSDefParser extends StdTokenParsers with ImplicitConversions {
stringLit ^^ StringLiteral

lazy val numberLiteral: Parser[NumberLiteral] =
numericLit ^^ { s =>
val d = s.toDouble
opt("-") ~ numericLit ^^ { case minus ~ s =>
val d = (minus.getOrElse("") + s).toDouble
if (!s.contains(".") && d.isValidInt) {
IntLiteral(d.toInt)
} else {
Expand Down