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

Loose parser: fixes and ES6 #151

Closed
wants to merge 37 commits into from
Closed

Conversation

RReverser
Copy link
Member

Done:

  • Simplified some tests.
  • Made driver to run already existing tests for both loose and normal parser.
  • Fixed inconsistencies in loose parser for found errors (in regexes, arrays etc.).
  • Added support for ES6 for loose parser with basic tolerance (might need extension in future for more cases).

Not done:

  • There are still few ES5 tests that are failing and not sure what is the best way to fix.
  • Template literals and tagged template literals from ES6 (not sure what is the best way to share / inject special tokenizer case for them).
  • Loose parser will still need own tests in future, but for now we can at least ensure it works for correct simple code with no specific indentation.

…Token`.

Allows to avoid both custom `testAssert` and adding extra arguments in `test`.
Includes correction of mistype ".." vs "...".
@marijnh
Copy link
Member

marijnh commented Nov 3, 2014

Great work!

Please put additional testing logic in test/driver.js, so that we don't have to duplicate it in test/index.html (the web-based test runner).

My lint tool spotted a bunch of suspicious things:

./acorn_loose.js: Assignment to global variable (793:4)
./acorn_loose.js: Assignment to global variable (810:6)
./acorn_loose.js: Access to global variable inTemplate. (813:4)
./acorn_loose.js: Access to global variable tokVal. (797:28)
./acorn_loose.js: Access to global variable tokStart. (797:53)
./acorn_loose.js: Access to global variable tokEnd. (810:15)
./acorn_loose.js: Access to global variable tokType. (801:10)
./acorn_loose.js: Access to global variable _bquote. (801:22)
./acorn_loose.js: Access to global variable _dollarBraceL. (806:13)
./acorn_loose.js: Access to global variable tokPos.(810:6)
./acorn_loose.js: Access to global variable _braceR. (811:13)
./acorn_loose.js: Unused function parseTemplate (789:11)

I'm also not sure about the tokenizer magic, I expect you understand the problem more completely than I do.

@RReverser
Copy link
Member Author

@marijnh Oh, right, in the beginning I was going to update web-driver too, but forgot until the moment of submitting PR. Will do.

Will check possible problems, thanks.

@RReverser
Copy link
Member Author

@marijnh Ah - linter is right, I copy-pasted parseTemplate function and tried to enhance it, but as I said in PR message, no luck yet, so just committed as-is. Will remove now.

@RReverser
Copy link
Member Author

Pushed support for web-driver:

2014-11-04_2-02-36

@RReverser
Copy link
Member Author

Regarding tokenizer magic - it's not that much problem with functionality but rather with understanding what is the best way to share it between acorn and acorn_loose parsers (as you already did with getToken thing).

* Added support for acorn_loose and grouped log to web-driver.
* Removed unused copy-pasted `parseTemplate` from loose parser.
* Throw non-SyntaxError errors immediately (as those are generic).
@marijnh
Copy link
Member

marijnh commented Nov 12, 2014

I've done some more work to make all tests pass for the loose parser, excluding the ones that use template strings. See attached patch. I've merged this into master.

@marijnh marijnh closed this Nov 12, 2014
marijnh added a commit that referenced this pull request Nov 12, 2014
Define Program node extent to be the whole program, make
both parser and the tests conform to this.

Fix a bunch of bugs in the loose parser's handling of corner
cases.

Issue #151
@RReverser
Copy link
Member Author

@marijnh Thanks! I'll check what I can about template strings, it shouldn't be too hard to implement them.

basicer added a commit to differentmatt/filbert that referenced this pull request Oct 19, 2015
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.

None yet

2 participants