Community
Participate
Working Groups
Build ID: I20080617-2000 JSDT doesn’t understand multi-line strings. In the following example, you will get an error ‘String literal is not properly closed by a double-quote’, even though it should just work. Also, the error description seems incorrect as in JS strings can use both single and double-quotes. Example: var xslt = new DOMParser().parseFromString('\ <xsl:stylesheet version="1.0"\ xmlns="http://www.w3.org/1999/xhtml"\ xmlns:xsl="http://www.w3.org/1999/XSL/Transform">\ <xsl:output method="html" indent="no"/>\ <xsl:template match="/">\ <div>\ <xsl:apply-templates select="*" />\ </div>\ </xsl:template>\ </xsl:stylesheet>\ '); ~Grauw
The multi-line string example shown is a violation of the spec. An escape character is not allowed before a line break. I do, however, realize that most browsers support this practice. I plan to do two things to address this bug: 1. Fix the error message. 2. Validation will remain in place to catch this problem, but a preference can be added allowing you to switch this error to a warning or ignore.
For what it’s worth, the ECMAScript 3.1 draft does specify this currently (page 20 in my local version, see LineContinuation key in the syntax). It would already help if this was parsed correctly, even if it triggers an error message, so that it does not mess up further parsing :). Personally, by the way, I would consider this de facto Javascript behaviour, and not have it trigger an error at all (at least not by default).
Categorizing JSDT bugzillas for planning purposes.
ECMAScript 5, now a standard, describes this syntax in section 7.8.4, the LineContinuation production.
Created attachment 155691 [details] Testcase Somewhat extensive testcase.
Created attachment 155692 [details] Fix Part 1 of the fix, org.eclipse.wst.jsdt.core module. See also http://hg.grauw.nl/org.eclipse.wst.jsdt.core/rev/a2d8ce10d2da
Created attachment 155693 [details] Fix (part 2) Part 2 of the fix, org.eclipse.wst.jsdt.ui module. See also http://hg.grauw.nl/org.eclipse.wst.jsdt.ui/rev/700917937ad0
So I fixed this. Part 1 takes care of the AST parser, part 2 takes care of the syntax highlighting. There’s a lot of code duplication between Scanner and PublicScanner in core btw. Is the latter still used? When I added breakpoints they didn’t get triggered. Anyway, I copied the code there, too.
Comment on attachment 155693 [details] Fix (part 2) Setting review flag to get some movement here.
Thanks for the patch Laurens. 3.2m5 is already in shutdown mode, so I targeted this for the next milestone, m6. Chris
Nitin, this is flagged to be reviewed by you. Can you take a look at this, so we can get it in m6?
This patch only needs to be reviewed and checked in. What’s holding it up?
Partly it's the "part 1" part, where the original js.g file changes aren't present so we can't actually regenerate a version with your changes when we make other updates to it, but also because we don't have an answer to your question in comment 8, and I don't like changing the parser in later milestones.
(In reply to comment #13) > Partly it's the "part 1" part, where the original js.g file changes aren't > present so we can't actually regenerate a version with your changes when we > make other updates to it, Really, those are generated files? It doesn’t look like that, with all those in-line comments... > but also because we don't have an answer to your > question in comment 8, The code I mentioned in comment 8 is virtually identical to the other code, so even if it is still used, I don’t think there is reason to believe it would not work there. (Also, then you would have a big code duplication issue.) Nevertheless I understand that you would want to verify this. > and I don't like changing the parser in later milestones. Meh, I submitted the patch in January, 4 months ago… Anyway, I’m looking forward to seeing it in the next version then. Thanks for your quick response!
(In reply to comment #14) > (In reply to comment #13) > > Partly it's the "part 1" part, where the original js.g file changes aren't > > present so we can't actually regenerate a version with your changes when we > > make other updates to it, > > Really, those are generated files? It doesn’t look like that, with all those > in-line comments... It's possible. The directions for regenerating the JavaScript parser aren't as simple as they are for our other languages. We'll look at this in 3.2.1.
Rescheduled 3.2.1 is too short, moving to 3.2.2.
Laurens, In public scanner you updated the scanEscapeCharacter() to always return false. Shoulddn't one of those return statements be true? Chris
Created attachment 177640 [details] patch Regenerated the patch so that it applies correctly in the 3_2_maintenance branch. Made no changes to the content.
Hi There, I'm running an Eclipse Helios patched with the most recent WTP 3.2.2 maintenance release (as of August 28th, 2010) and I still see errors for multi-line strings. Is there a setting I have to change to not see this, or has this still not been integrated at the present time? Thanks, Dominic.
(In reply to comment #19) > Hi There, > > I'm running an Eclipse Helios patched with the most recent WTP 3.2.2 > maintenance release (as of August 28th, 2010) and I still see errors for > multi-line strings. Is there a setting I have to change to not see this, or has > this still not been integrated at the present time? > > Thanks, Dominic. This bug is not in resolved state, meaning no code has been checked in. The patch has been reviewed and there was a question posted for the patch submitter that we are awaiting a response on. Chris
Created attachment 179672 [details] new patch
(In reply to comment #17) > In public scanner you updated the scanEscapeCharacter() to always return false. > > Shoulddn't one of those return statements be true? Hey, sorry for the late reply, Yes, looks like you’re right, the latter one should return true. Thanks, Laurens
Do you want me to post an updated patch or can you just make the above mentioned change before checking in? (I don’t really have an up-to-date tree so it would be convenient :).)
That new patch you just posted doesn’t seem right. It seems like it only contains part of the changes from fix (part 1). (Also, I see I mentioned in comment 8 that I couldn’t get the PublicScanner code to run, so that explains why it wasn’t tested and the error snuck in :). Sharp eyes!)
Created attachment 179680 [details] Updated patch Changed false to true in PublicScanner.
Created attachment 179681 [details] JUnits
Checked into 3.2.3 and HEAD.
Wonderful!
