Skip to content

Commit 42ba7b9

Browse files
dilijevdilijev
committed
[MERGE #5761 @rhuanjl] Improve error messages for syntax errors
Merge pull request #5761 from rhuanjl:parsingErrors Picking up on discussion in #5178 and also issue #767 This PR: 1. Adds support for %s in syntax error messages (though only up to 2 uses of it per message) 1. Adds facility for getting a string representation of the token being parsed, and the previous token 1. Updates some commonly encountered error messages that currently only say "Syntax error" I'd like to follow up and improve some other error messages but this seemed big enough for one PR. I'm slightly nervous that I may have done something wrong in my implementation of %s support - it works in my tests but I don't 100% understand all the relevant logic. fixes #5178
2 parents edfceaf + 1f18f53 commit 42ba7b9

18 files changed

+307
-69
lines changed

‎lib/Parser/Parse.cpp

Lines changed: 190 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,168 @@ void Parser::OutOfMemory()
159159
throw ParseExceptionObject(ERRnoMemory);
160160
}
161161

162-
void Parser::Error(HRESULT hr)
162+
LPCWSTR Parser::GetTokenString(tokens token)
163+
{
164+
switch (token)
165+
{
166+
case tkNone : return _u("");
167+
case tkEOF : return _u("end of script");
168+
case tkIntCon : return _u("integer literal");
169+
case tkFltCon : return _u("float literal");
170+
case tkStrCon : return _u("string literal");
171+
case tkRegExp : return _u("regular expression literal");
172+
173+
// keywords
174+
case tkABSTRACT : return _u("abstract");
175+
case tkASSERT : return _u("assert");
176+
case tkAWAIT : return _u("await");
177+
case tkBOOLEAN : return _u("boolean");
178+
case tkBREAK : return _u("break");
179+
case tkBYTE : return _u("byte");
180+
case tkCASE : return _u("case");
181+
case tkCATCH : return _u("catch");
182+
case tkCHAR : return _u("char");
183+
case tkCONTINUE : return _u("continue");
184+
case tkDEBUGGER : return _u("debugger");
185+
case tkDECIMAL : return _u("decimal");
186+
case tkDEFAULT : return _u("default");
187+
case tkDELETE : return _u("delete");
188+
case tkDO : return _u("do");
189+
case tkDOUBLE : return _u("double");
190+
case tkELSE : return _u("else");
191+
case tkENSURE : return _u("ensure");
192+
case tkEVENT : return _u("event");
193+
case tkFALSE : return _u("false");
194+
case tkFINAL : return _u("final");
195+
case tkFINALLY : return _u("finally");
196+
case tkFLOAT : return _u("float");
197+
case tkFOR : return _u("for");
198+
case tkFUNCTION : return _u("function");
199+
case tkGET : return _u("get");
200+
case tkGOTO : return _u("goto");
201+
case tkIF : return _u("if");
202+
case tkIN : return _u("in");
203+
case tkINSTANCEOF : return _u("instanceof");
204+
case tkINT : return _u("int");
205+
case tkINTERNAL : return _u("internal");
206+
case tkINVARIANT : return _u("invariant");
207+
case tkLONG : return _u("long");
208+
case tkNAMESPACE : return _u("namespace");
209+
case tkNATIVE : return _u("native");
210+
case tkNEW : return _u("new");
211+
case tkNULL : return _u("null");
212+
case tkREQUIRE : return _u("require");
213+
case tkRETURN : return _u("return");
214+
case tkSBYTE : return _u("sbyte");
215+
case tkSET : return _u("set");
216+
case tkSHORT : return _u("short");
217+
case tkSWITCH : return _u("switch");
218+
case tkSYNCHRONIZED : return _u("synchronized");
219+
case tkTHIS : return _u("this");
220+
case tkTHROW : return _u("throw");
221+
case tkTHROWS : return _u("throws");
222+
case tkTRANSIENT : return _u("transient");
223+
case tkTRUE : return _u("true");
224+
case tkTRY : return _u("try");
225+
case tkTYPEOF : return _u("typeof");
226+
case tkUINT : return _u("uint");
227+
case tkULONG : return _u("ulong");
228+
case tkUSE : return _u("use");
229+
case tkUSHORT : return _u("ushort");
230+
case tkVAR : return _u("var");
231+
case tkVOID : return _u("void");
232+
case tkVOLATILE : return _u("volatile");
233+
case tkWHILE : return _u("while");
234+
case tkWITH : return _u("with");
235+
236+
// Future reserved words that become keywords in ES6
237+
case tkCLASS : return _u("class");
238+
case tkCONST : return _u("const");
239+
case tkEXPORT : return _u("export");
240+
case tkEXTENDS : return _u("extends");
241+
case tkIMPORT : return _u("import");
242+
case tkLET : return _u("let");
243+
case tkSUPER : return _u("super");
244+
case tkYIELD : return _u("yield");
245+
246+
// Future reserved words in strict and non-strict modes
247+
case tkENUM : return _u("enum");
248+
249+
// Additional future reserved words in strict mode
250+
case tkIMPLEMENTS : return _u("implements");
251+
case tkINTERFACE : return _u("interface");
252+
case tkPACKAGE : return _u("package");
253+
case tkPRIVATE : return _u("private");
254+
case tkPROTECTED : return _u("protected");
255+
case tkPUBLIC : return _u("public");
256+
case tkSTATIC : return _u("static");
257+
258+
case tkID: return _u("identifier");
259+
260+
// Non-operator non-identifier tokens
261+
case tkSColon: return _u(";");
262+
case tkRParen: return _u(")");
263+
case tkRBrack: return _u("]");
264+
case tkLCurly: return _u("{");
265+
case tkRCurly: return _u("}");
266+
267+
// Operator non-identifier tokens
268+
case tkComma: return _u(",");
269+
case tkDArrow: return _u("=>");
270+
case tkAsg: return _u("=");
271+
case tkAsgAdd: return _u("+=");
272+
case tkAsgSub: return _u("-=");
273+
case tkAsgMul: return _u("*=");
274+
case tkAsgDiv: return _u("/=");
275+
case tkAsgExpo: return _u("**=");
276+
case tkAsgMod: return _u("%=");
277+
case tkAsgAnd: return _u("&=");
278+
case tkAsgXor: return _u("^=");
279+
case tkAsgOr: return _u("|=");
280+
case tkAsgLsh: return _u("<<=");
281+
case tkAsgRsh: return _u(">>=");
282+
case tkAsgRs2: return _u(">>>=");
283+
case tkQMark: return _u("?");
284+
case tkColon: return _u(":");
285+
case tkLogOr: return _u("||");
286+
case tkLogAnd: return _u("&&");
287+
case tkOr: return _u("|");
288+
case tkXor: return _u("^");
289+
case tkAnd: return _u("&");
290+
case tkEQ: return _u("==");
291+
case tkNE: return _u("!=");
292+
case tkEqv: return _u("===");
293+
case tkNEqv: return _u("!==");
294+
case tkLT: return _u("<");
295+
case tkLE: return _u("<=");
296+
case tkGT: return _u(">");
297+
case tkGE: return _u(">=");
298+
case tkLsh: return _u("<<");
299+
case tkRsh: return _u(">>");
300+
case tkRs2: return _u(">>>");
301+
case tkAdd: return _u("+");
302+
case tkSub: return _u("-");
303+
case tkExpo: return _u("**");
304+
case tkStar: return _u("*");
305+
case tkDiv: return _u("/");
306+
case tkPct: return _u("%");
307+
case tkTilde: return _u("~");
308+
case tkBang: return _u("!");
309+
case tkInc: return _u("++");
310+
case tkDec: return _u("--");
311+
case tkEllipsis: return _u("...");
312+
case tkLParen: return _u("(");
313+
case tkLBrack: return _u("[");
314+
case tkDot: return _u(".");
315+
316+
default:
317+
return _u("unknown token");
318+
}
319+
}
320+
321+
void Parser::Error(HRESULT hr, LPCWSTR stringOne, LPCWSTR stringTwo)
163322
{
164-
throw ParseExceptionObject(hr);
323+
throw ParseExceptionObject(hr, stringOne, stringTwo);
165324
}
166325

167326
void Parser::Error(HRESULT hr, ParseNodePtr pnode)
@@ -226,6 +385,7 @@ HRESULT Parser::ValidateSyntax(LPCUTF8 pszSrc, size_t encodedCharCount, bool isG
226385

227386
HRESULT hr;
228387
SmartFPUControl smartFpuControl;
388+
bool handled = false;
229389

230390
BOOL fDeferSave = m_deferringAST;
231391
try
@@ -287,9 +447,11 @@ HRESULT Parser::ValidateSyntax(LPCUTF8 pszSrc, size_t encodedCharCount, bool isG
287447
{
288448
m_deferringAST = fDeferSave;
289449
hr = e.GetError();
450+
hr = pse->ProcessError(this->GetScanner(), hr, /* pnodeBase */ NULL, e.GetStringOne(), e.GetStringTwo());
451+
handled = true;
290452
}
291453

292-
if (nullptr != pse && FAILED(hr))
454+
if (handled == false && nullptr != pse && FAILED(hr))
293455
{
294456
hr = pse->ProcessError(this->GetScanner(), hr, /* pnodeBase */ NULL);
295457
}
@@ -326,6 +488,7 @@ HRESULT Parser::ParseSourceInternal(
326488
ParseNodeProg * pnodeBase = NULL;
327489
HRESULT hr;
328490
SmartFPUControl smartFpuControl;
491+
bool handled = false;
329492

330493
try
331494
{
@@ -364,13 +527,15 @@ HRESULT Parser::ParseSourceInternal(
364527
catch (ParseExceptionObject& e)
365528
{
366529
hr = e.GetError();
530+
hr = pse->ProcessError(this->GetScanner(), hr, pnodeBase, e.GetStringOne(), e.GetStringTwo());
531+
handled = true;
367532
}
368533
catch (Js::AsmJsParseException&)
369534
{
370535
hr = JSERR_AsmJsCompileError;
371536
}
372537

373-
if (FAILED(hr))
538+
if (handled == false && FAILED(hr))
374539
{
375540
hr = pse->ProcessError(this->GetScanner(), hr, pnodeBase);
376541
}
@@ -2153,7 +2318,7 @@ IdentPtr Parser::ParseMetaProperty(tokens metaParentKeyword, charcount_t ichMin,
21532318
}
21542319
else
21552320
{
2156-
Error(ERRsyntax);
2321+
Error(ERRValidIfFollowedBy, _u("'new.'"), _u("'target'"));
21572322
}
21582323
}
21592324

@@ -2442,7 +2607,7 @@ void Parser::ParseImportClause(ModuleImportOrExportEntryList* importEntryList, b
24422607
// There cannot be a namespace import or named imports list on the left of the comma in a module import clause.
24432608
if (parsingAfterComma || parsedNamespaceOrNamedImport)
24442609
{
2445-
Error(ERRsyntax);
2610+
Error(ERRTokenAfter, _u(","), GetTokenString(this->GetScanner()->GetPrevious()));
24462611
}
24472612

24482613
this->GetScanner()->Scan();
@@ -2498,7 +2663,7 @@ ParseNodePtr Parser::ParseImport()
24982663
{
24992664
if (!m_scriptContext->GetConfig()->IsESDynamicImportEnabled())
25002665
{
2501-
Error(ERRsyntax);
2666+
Error(ERRExperimental);
25022667
}
25032668

25042669
ParseNodePtr pnode = ParseImportCall<buildAST>();
@@ -3109,7 +3274,7 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
31093274
// If the token after the right paren is not => or if there was a newline between () and => this is a syntax error
31103275
if (!IsDoingFastScan() && (m_token.tk != tkDArrow || this->GetScanner()->FHadNewLine()))
31113276
{
3112-
Error(ERRsyntax);
3277+
Error(ERRValidIfFollowedBy, _u("Lambda parameter list"), _u("'=>' on the same line"));
31133278
}
31143279

31153280
if (buildAST)
@@ -3423,7 +3588,18 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
34233588

34243589
default:
34253590
LUnknown:
3426-
Error(ERRsyntax);
3591+
if (m_token.tk == tkNone)
3592+
{
3593+
Error(ERRInvalidIdentifier, m_token.GetIdentifier(this->GetHashTbl())->Psz(), GetTokenString(GetScanner()->GetPrevious()));
3594+
}
3595+
else if (m_token.IsKeyword())
3596+
{
3597+
Error(ERRKeywordAfter, GetTokenString(m_token.tk), GetTokenString(GetScanner()->GetPrevious()));
3598+
}
3599+
else
3600+
{
3601+
Error(ERRTokenAfter, GetTokenString(m_token.tk), GetTokenString(GetScanner()->GetPrevious()));
3602+
}
34273603
break;
34283604
}
34293605

@@ -5607,7 +5783,7 @@ void Parser::ParseFncDeclHelper(ParseNodeFnc * pnodeFnc, LPCOLESTR pNameHint, us
56075783
// this after verifying there was a => token. Otherwise we would throw the wrong error.
56085784
if (hadNewLine)
56095785
{
5610-
Error(ERRsyntax);
5786+
Error(ERRValidIfFollowedBy, _u("Lambda parameter list"), _u("'=>' on the same line"));
56115787
}
56125788
}
56135789

@@ -11893,6 +12069,7 @@ HRESULT Parser::ParseFunctionInBackground(ParseNodeFnc * pnodeFnc, ParseContext
1189312069
ParseNodeBlock * pnodeBlock = StartParseBlock<true>(PnodeBlockType::Function, ScopeType_FunctionBody);
1189412070
pnodeFnc->pnodeScopes = pnodeBlock;
1189512071
m_ppnodeScope = &pnodeBlock->pnodeScopes;
12072+
bool handled = false;
1189612073

1189712074
uint uDeferSave = m_grfscr & (fscrCanDeferFncParse | fscrWillDeferFncParse);
1189812075

@@ -11943,9 +12120,11 @@ HRESULT Parser::ParseFunctionInBackground(ParseNodeFnc * pnodeFnc, ParseContext
1194312120
catch (ParseExceptionObject& e)
1194412121
{
1194512122
hr = e.GetError();
12123+
hr = pse->ProcessError(this->GetScanner(), hr, nullptr, e.GetStringOne(), e.GetStringTwo());
12124+
handled = true;
1194612125
}
1194712126

11948-
if (FAILED(hr))
12127+
if (handled == false && FAILED(hr))
1194912128
{
1195012129
hr = pse->ProcessError(this->GetScanner(), hr, nullptr);
1195112130
}

‎lib/Parser/Parse.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,8 @@ class Parser
313313
Js::ScriptContext* m_scriptContext;
314314
HashTbl * GetHashTbl() { return this->GetScanner()->GetHashTbl(); }
315315

316-
__declspec(noreturn) void Error(HRESULT hr);
316+
LPCWSTR GetTokenString(tokens token);
317+
__declspec(noreturn) void Error(HRESULT hr, LPCWSTR stringOne = _u(""), LPCWSTR stringTwo = _u(""));
317318
private:
318319
__declspec(noreturn) void Error(HRESULT hr, ParseNodePtr pnode);
319320
__declspec(noreturn) void Error(HRESULT hr, charcount_t ichMin, charcount_t ichLim);

‎lib/Parser/Scan.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,7 @@ class Scanner : public IScanner, public EncodingPolicy
688688
}
689689
};
690690

691+
tokens GetPrevious() { return m_tkPrevious; }
691692
void Capture(_Out_ RestorePoint* restorePoint);
692693
void SeekTo(const RestorePoint& restorePoint);
693694
void SeekToForcingPid(const RestorePoint& restorePoint);

‎lib/Parser/cmperr.cpp

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,16 @@
44
//-------------------------------------------------------------------------------------------------------
55
#include "ParserPch.h"
66

7-
ParseExceptionObject::ParseExceptionObject(HRESULT hr) : m_hr(hr)
7+
ParseExceptionObject::ParseExceptionObject(HRESULT hr, LPCWSTR stringOneIn, LPCWSTR stringTwoIn)
88
{
9+
m_hr = hr;
10+
stringOne = SysAllocString(stringOneIn);
11+
stringTwo = SysAllocString(stringTwoIn);
912
Assert(FAILED(hr));
1013
}
1114

15+
ParseExceptionObject::~ParseExceptionObject()
16+
{
17+
SysFreeString(stringOne);
18+
SysFreeString(stringTwo);
19+
}

‎lib/Parser/cmperr.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,13 @@ enum
1717
class ParseExceptionObject
1818
{
1919
public:
20-
ParseExceptionObject(HRESULT hr);
20+
ParseExceptionObject(HRESULT hr, LPCWSTR stringOneIn = _u(""), LPCWSTR stringTwoIn = _u(""));
21+
~ParseExceptionObject();
2122
HRESULT GetError() { return m_hr; }
23+
LPCWSTR GetStringOne() { return stringOne; }
24+
LPCWSTR GetStringTwo() { return stringTwo; }
2225
private:
2326
HRESULT m_hr;
27+
BSTR stringOne;
28+
BSTR stringTwo;
2429
};

‎lib/Parser/perrors.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,3 +109,12 @@ LSC_ERROR_MSG(1093, ERRLabelBeforeClassDeclaration, "Labels not allowed before c
109109
LSC_ERROR_MSG(1094, ERRLabelFollowedByEOF, "Unexpected end of script after a label.")
110110
LSC_ERROR_MSG(1095, ERRFunctionAfterLabelInStrict, "Function declarations not allowed after a label in strict mode.")
111111
LSC_ERROR_MSG(1096, ERRAwaitAsLabelInAsync, "Use of 'await' as label in async function is not allowed.")
112+
LSC_ERROR_MSG(1097, ERRExperimental, "Use of disabled experimental feature")
113+
//1098-1199 available for future use
114+
115+
// Generic errors intended to be re-usable
116+
LSC_ERROR_MSG(1200, ERRKeywordAfter, "Unexpected keyword '%s' after '%s'")
117+
LSC_ERROR_MSG(1201, ERRTokenAfter, "Unexpected token '%s' after '%s'")
118+
LSC_ERROR_MSG(1202, ERRIdentifierAfter, "Unexpected identifier '%s' after '%s'")
119+
LSC_ERROR_MSG(1203, ERRInvalidIdentifier, "Unexpected invalid identifier '%s' after '%s'")
120+
LSC_ERROR_MSG(1205, ERRValidIfFollowedBy, "%s is only valid if followed by %s")

‎lib/Parser/screrror.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ void CompileScriptException::CopyInto(CompileScriptException* pse)
251251
}
252252
}
253253

254-
HRESULT CompileScriptException::ProcessError(IScanner * pScan, HRESULT hr, ParseNode * pnodeBase)
254+
HRESULT CompileScriptException::ProcessError(IScanner * pScan, HRESULT hr, ParseNode * pnodeBase, LPCWSTR stringOne, LPCWSTR stringTwo)
255255
{
256256
// fill in the ScriptException structure
257257
Free();
@@ -267,6 +267,13 @@ HRESULT CompileScriptException::ProcessError(IScanner * pScan, HRESULT hr, Pars
267267
if (nullptr == (ei.bstrDescription = SysAllocString(szT)))
268268
ei.scode = E_OUTOFMEMORY;
269269
}
270+
else if (wcslen(stringOne) > 0)
271+
{
272+
OLECHAR szT[128];
273+
_snwprintf_s(szT, ARRAYSIZE(szT), ARRAYSIZE(szT)-1, ei.bstrDescription, stringOne, stringTwo);
274+
SysFreeString(ei.bstrDescription);
275+
ei.bstrDescription = SysAllocString(szT);
276+
}
270277

271278
ei.bstrSource = BstrGetResourceString(IDS_COMPILATION_ERROR_SOURCE);
272279
if (nullptr == pnodeBase && nullptr != pScan)

‎lib/Parser/screrror.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class CompileScriptException : public ScriptException
7272

7373
void CopyInto(CompileScriptException* cse);
7474

75-
HRESULT ProcessError(IScanner * pScan, HRESULT hr, ParseNode * pnodeBase);
75+
HRESULT ProcessError(IScanner * pScan, HRESULT hr, ParseNode * pnodeBase, LPCWSTR stringOne = _u(""), LPCWSTR stringTwo = _u(""));
7676

7777
friend class ActiveScriptError;
7878
};

0 commit comments

Comments
 (0)