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

[vf] Multiple parser bug fixes #340

Merged
merged 7 commits into from Apr 14, 2017
Merged

Conversation

sgorbaty
Copy link
Contributor

Fixing several VF parser bugs:

  1. Comments in EL scripts were not parsed -> fixed
  2. Style script parsing was buggy -> removed style support
  3. Escaping quotes were not parsed -> fixed

Added additional parser unit tests.

@jsotuyod jsotuyod changed the title [VF] Multiple parser bug fixes [vf] Multiple parser bug fixes Apr 14, 2017
@@ -70,7 +70,7 @@ PARSER_END(VfParser)
| <#XMLNAME: (<ALPHA_CHAR> | "_" | ":") (<IDENTIFIER_CHAR>)* >
| <#QUOTED_STRING_NO_BREAKS: ( "'" ( ~["'", "\r", "\n"] )* "'" )
| ( "\"" ( ~["\"", "\r", "\n"] )* "\"" ) >
| <#QUOTED_STRING: ( "'" ( ~["'"] )* "'" ) | ( "\"" ( ~["\""] )* "\"" ) >
| <#QUOTED_STRING: ( "'" ( ~["'"] )* "'" ) | ( "\"" ( ~["\""] | "\\\"" )* "\"" ) >
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't you change ( "'" ( ~["'"] )* "'" ) to ( "'" ( ~["'"] | "\\'" )* "'" ) to allow strings with single quotes including escaped quotes?

Copy link
Contributor Author

@sgorbaty sgorbaty Apr 14, 2017

Choose a reason for hiding this comment

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

@jsotuyod This would be superfluous, VisualForce rewrites all double quotes in the source code into single quotes for the resulting page. Putting something like <abc ab=' \' '>abc</abc> causes a compilation error and <abc ab=' \' >abc</abc> does not.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, not what I expected, but if vf works that way...

@jsotuyod
Copy link
Member

All comments (both XML / HTML and EL) are currently part of the generated AST... this is usually not the case on grammars, moving them to special tokens which are ignored. Completely outside of this PR's scope, but is there any reason to not want it that way?

@jsotuyod jsotuyod self-assigned this Apr 14, 2017
@sgorbaty
Copy link
Contributor Author

You are absolutely right, I'm going to pull the new AST node and the other AST node.

@jsotuyod
Copy link
Member

@sgorbaty awesome, make it on a different PR, I'll be merging this soon enough

@jsotuyod jsotuyod added the a:bug PMD crashes or fails to analyse a file. label Apr 14, 2017
@jsotuyod jsotuyod added this to the 5.6.0 milestone Apr 14, 2017
@sgorbaty
Copy link
Contributor Author

sgorbaty commented Apr 14, 2017

Oops. @jsotuyod, I just pushed it in.

@jsotuyod
Copy link
Member

@sgorbaty lol! Don't worry, that was way faster than expected.

@jsotuyod jsotuyod merged commit 5c8d324 into pmd:master Apr 14, 2017
jsotuyod added a commit that referenced this pull request Apr 14, 2017
@jsotuyod
Copy link
Member

Merged! Will be included in PMD 5.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:bug PMD crashes or fails to analyse a file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants