Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fix unified expression language single quote escape #125

Closed

Conversation

sadikovi
Copy link
Contributor

@sadikovi sadikovi commented Jan 21, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR fixes issue when escaping single quote in single quoted string affects subsequent highlighting, because current pattern matches the first single quote as the end of the literal, even though it is escaped. It was realised that we only need to match ending of literal when it is not escaped, any single quote with preceding '\' should be marked as escaped.

Before patch:
before-patch

After patch:
after-patch

Also correctly highlights situation '\\ ', though I am not sure if this is a correct syntax.

Alternate Designs

None were considered apart from the PR solution.

Benefits

Fixes issue of highlighting when escaping single quote in single quote string literal.

Possible Drawbacks

Might break some other code, because I am not very familiar with expression language and could have overlooked some escape situations, such as using the \\' as a value.

Applicable Issues

Fixes #83

@sadikovi
Copy link
Contributor Author

@50Wliu could you review this pull request? Thanks!

@winstliu
Copy link
Contributor

@MoritzKn May I ask for your input here? Do you need to double-escape backslashes in EL?

@MoritzKn
Copy link
Contributor

MoritzKn commented Jan 27, 2018

@50Wliu I haven't used it for quite some time and don't have a setup at hand to test it right now. The specification says:

1.3 Literals
There are literals for boolean, integer, floating point, string, and null in an evalexpression.

  • Boolean - true and false
  • Integer - As defined by the IntegerLiteral construct in Section 1.19
  • Floating point - As defined by the FloatingPointLiteral construct in
    Section 1.19
  • String - With single and double quotes - " is escaped as \", ' is escaped as \',
    and \ is escaped as \\. Quotes only need to be escaped in a string value enclosed
    in the same type of quote
  • Null - null

-- http://download.oracle.com/otn-pub/jcp/jsp-2.1-fr-spec-oth-JSpec/jsp-2_1-fr-spec-el.pdf

However this may not apply for this case since it's embedded in the JSP code.

@winstliu
Copy link
Contributor

My hunch is that since this is embedded in JSP, we should be adding escape character support to JSP instead of EL.

@sadikovi
Copy link
Contributor Author

The reason I fixed in EL was due to broken punctuation.definition.string.end.java.el being defined in unified expression language, so I thought that it must be the place to fix it, rather than JSP, which does not define it. Another argument to support this was that bug originates from a function being part of the attribute value.

My understanding is that JSP file is for tags and attributes (key = value), and values are parsed as EL (scope source.java.el) - though I might be wrong here, and would appreciate any corrections. Thanks!

@winstliu
Copy link
Contributor

winstliu commented Feb 3, 2018

From the "Quoting and Escape Conventions" section of http://download.oracle.com/otn-pub/jcp/jsp-2_3-mrel2-eval-spec/JSP2.3MR.pdf?AuthParam=1517623843_d412f1e03b76e1d552dbf301b0948086 (not sure how up-to-date that is), backslash escaping is done in attribute values (in this case EL). So I think what's happening here is the same thing as what's happening when writing grammar files - the backslash needs to be double-escaped once for EL and then the second time for JSP.

@sadikovi
Copy link
Contributor Author

sadikovi commented Feb 6, 2018

@50Wliu I think this is what we do - escape only \\', right? Should we proceed with this fix or do you have any concerns? Thanks.

@winstliu
Copy link
Contributor

@sadikovi No, I believe that we should add proper escape tokenization to the JSP language.

@sadikovi
Copy link
Contributor Author

@50Wliu thanks for the review and comment! I used previous escape scope as a guidance and thought that this fox should be there as well. I am going to close this PR - currently there are other more critical issues to fix:)

@sadikovi sadikovi closed this Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntax highlighting confused by JSTL replace pattern
4 participants