Rewrite expression parser to a state machine in optional mixin #4768

TimvdLippe · 2017-08-03T17:46:26Z

Implement a new binding expression parser as statement machine instead of a regex. This solves the problem of maintaining a whitelist of characters that are allowed in bindings, e.g. it allows Japanese unicode characters, slashes, etc... It can better deal with unbalanced brackets, e.g. {{asdf]] and [[asdf}}, since we can maintain state and escaping in strings is more intuitive.

Regarding performance: I ran the supplied test on current master and on this branch. The new binding expression parsers took 950 ms for 1000 templates while current master took 700 ms. Have to take a deep dive in what is taking a lot of time, but would like to get some initial feedback first.

TODO:

Documentation
Performance improvements
Check compliance with Closure Compiler
Add mechanism to check if parsing succeeded, to improve error handling of incorrect bindings

Reference Issue

Fixes #1734
Fixes #1978
Fixes #3870
Fixes #4239
Fixes #4723
Supersedes and closes #3871

TimvdLippe · 2017-08-08T22:14:43Z

Performance is now on-par with the old regex.

sorvell · 2017-09-15T00:19:09Z

It would be great to see if this PR could be done as an optional mixin. Hopefully there are enough hooks in the binding system to make this possible.

The concern is the size and speed of the additional code here. This plus the fact that these issues are mostly pretty corner case makes me think that perhaps an optional mixin might be a better fit.

sorvell

See: #4768 (comment)

dfreedm · 2017-09-15T00:21:27Z

Just a few eslint issues: https://travis-ci.org/Polymer/polymer/builds/265406074

TimvdLippe · 2017-09-19T03:10:51Z

This is now extracted to a mixin and tested separately. We might still want to take a closer look at the performance.

sorvell

Let's call this mixin something slightly more specific like perhaps StrictBindingParser?

TimvdLippe · 2017-09-22T23:07:27Z

@sorvell renamed!

sorvell · 2017-09-27T00:24:13Z

lib/utils/strict-binding-parser.html

@@ -0,0 +1,405 @@
+<!--


Let's move this file to lib/mixinssince it's.... a mixin.

sorvell · 2017-10-03T18:34:31Z

lib/mixins/strict-binding-parser.html

+                  storeMethodVariable(bindingData, text, i);
+                  storeMethod(bindingData, templateInfo);
+                  bindingData.startChar = i + 1;
+                  storeMethod(bindingData, templateInfo);


Is this a typo to call storeMethod a second time here? If so, let's fix and add a test to verify.

@TimvdLippe acknowledged issue and will fix.

I no longer have write access to this repository, so I submitted #4866 on this branch to fix the issue

sorvell · 2017-11-16T20:22:57Z

We need to fix the test issues before merging.

justinfagnani · 2017-11-16T22:18:39Z

Hey @TimvdLippe, we talked about this a bit already, but there's a lot of overlap here with the polymer-expressions 2.0 branch: https://github.com/Polymer/polymer-expressions/tree/2.0-preview

I think that recursive descent parsers are generally easier to reason about and maintain than hand-written state machines. It'd be interesting to see if polymer-expressions were suitable to use as the more strict parser either if 1) were stripped down to just the syntax and features that Polymer currently supports, or 2) we offered the full syntax as an opt-in for more powerful expressions.

@kevinpschaaf mentions that this PR handles finding and matching the expression delimiters too, and that would need to be done for polymer-expressions. He and I have both not been able to put together the time to finish the push to integrating it is a mixin. I wonder if you could investigate that and try to get it actually working to compare against this option.

TimvdLippe · 2017-11-17T10:22:16Z

@justinfagnani Yes I did consider using polymer-expressions instead. However, the original bindings were detected by a regex, which is quite performant, but also quite unmaintainable. I did multiple iterations of my overall idea (on how to remove the whitelist of accepted characters and take into account delimiters). Most solutions were simply not performant enough to be comparable with the regex.

The pure state machine was the solution that did get very close (not 100% same speed), while it is more maintainable than the original regex (imo). I fear that a full-blown generalized parser (like in polymer-expressions) is too big and slow for such a small syntax, that's why I did not go down that route.

Nonetheless, I will try to port the expressions in a similar manner to this PR and confirm/deny my suspicion.

TimvdLippe · 2018-02-25T15:44:31Z

@sorvell Build is passing now and docs + types are updated.

TimvdLippe · 2018-03-27T18:22:03Z

For me: update the docs to state performance vs correctness trade-off, fix conflicts and then merge. Verbal LGTM by @sorvell

TimvdLippe added 2.x enhancement performance labels Aug 3, 2017

TimvdLippe requested review from dfreedm, sorvell and kevinpschaaf August 3, 2017 17:46

googlebot added the cla: yes label Aug 3, 2017

TimvdLippe force-pushed the rewrite-expression-parser branch from 9cca465 to 864bf6e Compare August 8, 2017 22:10

TimvdLippe mentioned this pull request Sep 15, 2017

Fix binding to field in object with slash in key name #3871

Closed

sorvell suggested changes Sep 15, 2017

View reviewed changes

TimvdLippe force-pushed the rewrite-expression-parser branch from 3bece73 to b03bca9 Compare September 19, 2017 02:41

TimvdLippe mentioned this pull request Sep 19, 2017

Data binding doesn't parse Japanese unicode characters #4723

Closed

TimvdLippe changed the title ~~Rewrite expression parser to a state machine~~ Rewrite expression parser to a state machine in optional mixin Sep 22, 2017

sorvell suggested changes Sep 22, 2017

View reviewed changes

TimvdLippe force-pushed the rewrite-expression-parser branch from 39f8664 to 7a51a10 Compare September 22, 2017 23:06

sorvell reviewed Sep 27, 2017

View reviewed changes

sorvell reviewed Oct 3, 2017

View reviewed changes

TimvdLippe mentioned this pull request Oct 5, 2017

Only store method once for dynamic functions #4866

Merged

sorvell approved these changes Nov 16, 2017

View reviewed changes

sorvell requested a review from justinfagnani November 16, 2017 20:23

TimvdLippe and others added 18 commits February 25, 2018 16:01

Rewrite expression parser to state machine

13b834d

Fix linter warnings

79d05b8

Refactor to be functional and add more tests

7eb1a62

Fix linter warning

8a5525b

Remove escaping from bindings

8cd4947

Rewrite parser to use switch-case instead of functions

423074d

Add performance test for binding-expressions

42f7d78

Add documentation

df0ee35

Fix linter warning

14fac01

Add missing dependency to bower.json

333a466

Extract to a mixin

57a1423

Fix linter errors

d8cf449

Rename to StrictBindingParser

19d4b8c

Move strict-binding-parser to lib/mixins

a4d4eb9

Only store method once for dynamic functions

0f0ccda

Correct import paths

ab93ab0

Update perf test to use strict-binding-parser

f53e9e8

Update docs and types

211c223

TimvdLippe force-pushed the rewrite-expression-parser branch from 198c937 to 211c223 Compare February 25, 2018 15:13

Merge branch 'master' into rewrite-expression-parser

bb69819

TimvdLippe added 2 commits March 31, 2018 15:52

Merge branch 'master' into rewrite-expression-parser

8d97d13

Add note about performance vs correctness

89ab738

TimvdLippe mentioned this pull request Mar 31, 2018

Add note about the strict-binding-parser in data-binding section Polymer/old-docs-site#2508

Open

Update types

23ba7de

TimvdLippe merged commit 569ff37 into master Mar 31, 2018

TimvdLippe deleted the rewrite-expression-parser branch March 31, 2018 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite expression parser to a state machine in optional mixin #4768

Rewrite expression parser to a state machine in optional mixin #4768

TimvdLippe commented Aug 3, 2017 •

edited

Loading

TimvdLippe commented Aug 8, 2017

sorvell commented Sep 15, 2017

sorvell left a comment

dfreedm commented Sep 15, 2017

TimvdLippe commented Sep 19, 2017

sorvell left a comment

TimvdLippe commented Sep 22, 2017

sorvell Sep 27, 2017

TimvdLippe Sep 27, 2017

sorvell Oct 3, 2017

sorvell Oct 3, 2017

TimvdLippe Oct 5, 2017

sorvell commented Nov 16, 2017

justinfagnani commented Nov 16, 2017

TimvdLippe commented Nov 17, 2017

TimvdLippe commented Feb 25, 2018

TimvdLippe commented Mar 27, 2018

Rewrite expression parser to a state machine in optional mixin #4768

Rewrite expression parser to a state machine in optional mixin #4768

Conversation

TimvdLippe commented Aug 3, 2017 • edited Loading

Reference Issue

TimvdLippe commented Aug 8, 2017

sorvell commented Sep 15, 2017

sorvell left a comment

Choose a reason for hiding this comment

dfreedm commented Sep 15, 2017

TimvdLippe commented Sep 19, 2017

sorvell left a comment

Choose a reason for hiding this comment

TimvdLippe commented Sep 22, 2017

sorvell Sep 27, 2017

Choose a reason for hiding this comment

TimvdLippe Sep 27, 2017

Choose a reason for hiding this comment

sorvell Oct 3, 2017

Choose a reason for hiding this comment

sorvell Oct 3, 2017

Choose a reason for hiding this comment

TimvdLippe Oct 5, 2017

Choose a reason for hiding this comment

sorvell commented Nov 16, 2017

justinfagnani commented Nov 16, 2017

TimvdLippe commented Nov 17, 2017

TimvdLippe commented Feb 25, 2018

TimvdLippe commented Mar 27, 2018

TimvdLippe commented Aug 3, 2017 •

edited

Loading