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

Conversation

SrTobi
Copy link

@SrTobi SrTobi commented Jun 16, 2019

this pr fixes #103

enum values can be expressions including

  • unary operators
  • binary operators
  • string and number literals
  • parenthesis
  • identifiers

See:
https://github.com/microsoft/TypeScript/blob/master/doc/spec.md#92-enum-members
https://github.com/microsoft/TypeScript/blob/master/doc/spec.md#419-binary-operators

Note:
this implementation performs a flat parse that neither
handles expression-tree-building nor operator precedences.

Note:
This commit removes the minus from the NumericLit-token and
parses it explicitly in TSDefParser.numberLiteral

enum values can be expressions including
- unary operators
- binary operators
- string and number literals
- parenthesis
- identifiers

See:
https://github.com/microsoft/TypeScript/blob/master/doc/spec.md#92-enum-members
https://github.com/microsoft/TypeScript/blob/master/doc/spec.md#419-binary-operators

Note:
this implementation performs a flat parse that neither
handles expression-tree-building nor operator precedences.

Note:
This commit removes the minus from the NumericLit-token and
parses it explicitly in TSDefParser.numberLiteral
@SrTobi SrTobi force-pushed the value-expressions-pr branch from 91adb75 to 1ed9dbd Compare June 16, 2019 12:50
@SrTobi
Copy link
Author

SrTobi commented Jun 16, 2019

The CI fails because it uses typescript 2.6.2 for checking the typedefinitions in samples. Sometime between that and the newest version ([email protected]) the allowed syntax seems to have been broadened, as it works for me locally.

@exoego
Copy link
Contributor

exoego commented Jun 16, 2019

TypeScript 2.6.2 was added just because it was latest when #56 was created.
I think there is no reason to stay older TS, so it can be updated to 3.x.

@SrTobi
Copy link
Author

SrTobi commented Jun 16, 2019

ah ok. I did that. thx

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.

@@ -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

// 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.

"(" ~! 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

elem("binary operator", tok => binaryValueOperators.contains(tok.chars))

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))
"+" | "-" | "~"

repsep(identifier <~ opt("=" ~! valueExpression), ",") <~ opt(",")

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?

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.

Enum values are not parsed correctly
3 participants