Skip to content

Commit

Permalink
[MERGE #5761 @rhuanjl] Improve error messages for syntax errors
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@dilijev
dilijev committed Oct 9, 2018
2 parents edfceaf + 1f18f53 commit 42ba7b9
Show file tree
Hide file tree
Showing 18 changed files with 307 additions and 69 deletions.
201 changes: 190 additions & 11 deletions lib/Parser/Parse.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,168 @@ void Parser::OutOfMemory()
throw ParseExceptionObject(ERRnoMemory);
}

void Parser::Error(HRESULT hr)
LPCWSTR Parser::GetTokenString(tokens token)
{
switch (token)
{
case tkNone : return _u("");
case tkEOF : return _u("end of script");
case tkIntCon : return _u("integer literal");
case tkFltCon : return _u("float literal");
case tkStrCon : return _u("string literal");
case tkRegExp : return _u("regular expression literal");

// keywords
case tkABSTRACT : return _u("abstract");
case tkASSERT : return _u("assert");
case tkAWAIT : return _u("await");
case tkBOOLEAN : return _u("boolean");
case tkBREAK : return _u("break");
case tkBYTE : return _u("byte");
case tkCASE : return _u("case");
case tkCATCH : return _u("catch");
case tkCHAR : return _u("char");
case tkCONTINUE : return _u("continue");
case tkDEBUGGER : return _u("debugger");
case tkDECIMAL : return _u("decimal");
case tkDEFAULT : return _u("default");
case tkDELETE : return _u("delete");
case tkDO : return _u("do");
case tkDOUBLE : return _u("double");
case tkELSE : return _u("else");
case tkENSURE : return _u("ensure");
case tkEVENT : return _u("event");
case tkFALSE : return _u("false");
case tkFINAL : return _u("final");
case tkFINALLY : return _u("finally");
case tkFLOAT : return _u("float");
case tkFOR : return _u("for");
case tkFUNCTION : return _u("function");
case tkGET : return _u("get");
case tkGOTO : return _u("goto");
case tkIF : return _u("if");
case tkIN : return _u("in");
case tkINSTANCEOF : return _u("instanceof");
case tkINT : return _u("int");
case tkINTERNAL : return _u("internal");
case tkINVARIANT : return _u("invariant");
case tkLONG : return _u("long");
case tkNAMESPACE : return _u("namespace");
case tkNATIVE : return _u("native");
case tkNEW : return _u("new");
case tkNULL : return _u("null");
case tkREQUIRE : return _u("require");
case tkRETURN : return _u("return");
case tkSBYTE : return _u("sbyte");
case tkSET : return _u("set");
case tkSHORT : return _u("short");
case tkSWITCH : return _u("switch");
case tkSYNCHRONIZED : return _u("synchronized");
case tkTHIS : return _u("this");
case tkTHROW : return _u("throw");
case tkTHROWS : return _u("throws");
case tkTRANSIENT : return _u("transient");
case tkTRUE : return _u("true");
case tkTRY : return _u("try");
case tkTYPEOF : return _u("typeof");
case tkUINT : return _u("uint");
case tkULONG : return _u("ulong");
case tkUSE : return _u("use");
case tkUSHORT : return _u("ushort");
case tkVAR : return _u("var");
case tkVOID : return _u("void");
case tkVOLATILE : return _u("volatile");
case tkWHILE : return _u("while");
case tkWITH : return _u("with");

// Future reserved words that become keywords in ES6
case tkCLASS : return _u("class");
case tkCONST : return _u("const");
case tkEXPORT : return _u("export");
case tkEXTENDS : return _u("extends");
case tkIMPORT : return _u("import");
case tkLET : return _u("let");
case tkSUPER : return _u("super");
case tkYIELD : return _u("yield");

// Future reserved words in strict and non-strict modes
case tkENUM : return _u("enum");

// Additional future reserved words in strict mode
case tkIMPLEMENTS : return _u("implements");
case tkINTERFACE : return _u("interface");
case tkPACKAGE : return _u("package");
case tkPRIVATE : return _u("private");
case tkPROTECTED : return _u("protected");
case tkPUBLIC : return _u("public");
case tkSTATIC : return _u("static");

case tkID: return _u("identifier");

// Non-operator non-identifier tokens
case tkSColon: return _u(";");
case tkRParen: return _u(")");
case tkRBrack: return _u("]");
case tkLCurly: return _u("{");
case tkRCurly: return _u("}");

// Operator non-identifier tokens
case tkComma: return _u(",");
case tkDArrow: return _u("=>");
case tkAsg: return _u("=");
case tkAsgAdd: return _u("+=");
case tkAsgSub: return _u("-=");
case tkAsgMul: return _u("*=");
case tkAsgDiv: return _u("/=");
case tkAsgExpo: return _u("**=");
case tkAsgMod: return _u("%=");
case tkAsgAnd: return _u("&=");
case tkAsgXor: return _u("^=");
case tkAsgOr: return _u("|=");
case tkAsgLsh: return _u("<<=");
case tkAsgRsh: return _u(">>=");
case tkAsgRs2: return _u(">>>=");
case tkQMark: return _u("?");
case tkColon: return _u(":");
case tkLogOr: return _u("||");
case tkLogAnd: return _u("&&");
case tkOr: return _u("|");
case tkXor: return _u("^");
case tkAnd: return _u("&");
case tkEQ: return _u("==");
case tkNE: return _u("!=");
case tkEqv: return _u("===");
case tkNEqv: return _u("!==");
case tkLT: return _u("<");
case tkLE: return _u("<=");
case tkGT: return _u(">");
case tkGE: return _u(">=");
case tkLsh: return _u("<<");
case tkRsh: return _u(">>");
case tkRs2: return _u(">>>");
case tkAdd: return _u("+");
case tkSub: return _u("-");
case tkExpo: return _u("**");
case tkStar: return _u("*");
case tkDiv: return _u("/");
case tkPct: return _u("%");
case tkTilde: return _u("~");
case tkBang: return _u("!");
case tkInc: return _u("++");
case tkDec: return _u("--");
case tkEllipsis: return _u("...");
case tkLParen: return _u("(");
case tkLBrack: return _u("[");
case tkDot: return _u(".");

default:
return _u("unknown token");
}
}

void Parser::Error(HRESULT hr, LPCWSTR stringOne, LPCWSTR stringTwo)
{
throw ParseExceptionObject(hr);
throw ParseExceptionObject(hr, stringOne, stringTwo);
}

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

HRESULT hr;
SmartFPUControl smartFpuControl;
bool handled = false;

BOOL fDeferSave = m_deferringAST;
try
Expand Down Expand Up @@ -287,9 +447,11 @@ HRESULT Parser::ValidateSyntax(LPCUTF8 pszSrc, size_t encodedCharCount, bool isG
{
m_deferringAST = fDeferSave;
hr = e.GetError();
hr = pse->ProcessError(this->GetScanner(), hr, /* pnodeBase */ NULL, e.GetStringOne(), e.GetStringTwo());
handled = true;
}

if (nullptr != pse && FAILED(hr))
if (handled == false && nullptr != pse && FAILED(hr))
{
hr = pse->ProcessError(this->GetScanner(), hr, /* pnodeBase */ NULL);
}
Expand Down Expand Up @@ -326,6 +488,7 @@ HRESULT Parser::ParseSourceInternal(
ParseNodeProg * pnodeBase = NULL;
HRESULT hr;
SmartFPUControl smartFpuControl;
bool handled = false;

try
{
Expand Down Expand Up @@ -364,13 +527,15 @@ HRESULT Parser::ParseSourceInternal(
catch (ParseExceptionObject& e)
{
hr = e.GetError();
hr = pse->ProcessError(this->GetScanner(), hr, pnodeBase, e.GetStringOne(), e.GetStringTwo());
handled = true;
}
catch (Js::AsmJsParseException&)
{
hr = JSERR_AsmJsCompileError;
}

if (FAILED(hr))
if (handled == false && FAILED(hr))
{
hr = pse->ProcessError(this->GetScanner(), hr, pnodeBase);
}
Expand Down Expand Up @@ -2153,7 +2318,7 @@ IdentPtr Parser::ParseMetaProperty(tokens metaParentKeyword, charcount_t ichMin,
}
else
{
Error(ERRsyntax);
Error(ERRValidIfFollowedBy, _u("'new.'"), _u("'target'"));
}
}

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

this->GetScanner()->Scan();
Expand Down Expand Up @@ -2498,7 +2663,7 @@ ParseNodePtr Parser::ParseImport()
{
if (!m_scriptContext->GetConfig()->IsESDynamicImportEnabled())
{
Error(ERRsyntax);
Error(ERRExperimental);
}

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

if (buildAST)
Expand Down Expand Up @@ -3423,7 +3588,18 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,

default:
LUnknown:
Error(ERRsyntax);
if (m_token.tk == tkNone)
{
Error(ERRInvalidIdentifier, m_token.GetIdentifier(this->GetHashTbl())->Psz(), GetTokenString(GetScanner()->GetPrevious()));
}
else if (m_token.IsKeyword())
{
Error(ERRKeywordAfter, GetTokenString(m_token.tk), GetTokenString(GetScanner()->GetPrevious()));
}
else
{
Error(ERRTokenAfter, GetTokenString(m_token.tk), GetTokenString(GetScanner()->GetPrevious()));
}
break;
}

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

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

uint uDeferSave = m_grfscr & (fscrCanDeferFncParse | fscrWillDeferFncParse);

Expand Down Expand Up @@ -11943,9 +12120,11 @@ HRESULT Parser::ParseFunctionInBackground(ParseNodeFnc * pnodeFnc, ParseContext
catch (ParseExceptionObject& e)
{
hr = e.GetError();
hr = pse->ProcessError(this->GetScanner(), hr, nullptr, e.GetStringOne(), e.GetStringTwo());
handled = true;
}

if (FAILED(hr))
if (handled == false && FAILED(hr))
{
hr = pse->ProcessError(this->GetScanner(), hr, nullptr);
}
Expand Down
3 changes: 2 additions & 1 deletion lib/Parser/Parse.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,8 @@ class Parser
Js::ScriptContext* m_scriptContext;
HashTbl * GetHashTbl() { return this->GetScanner()->GetHashTbl(); }

__declspec(noreturn) void Error(HRESULT hr);
LPCWSTR GetTokenString(tokens token);
__declspec(noreturn) void Error(HRESULT hr, LPCWSTR stringOne = _u(""), LPCWSTR stringTwo = _u(""));
private:
__declspec(noreturn) void Error(HRESULT hr, ParseNodePtr pnode);
__declspec(noreturn) void Error(HRESULT hr, charcount_t ichMin, charcount_t ichLim);
Expand Down
1 change: 1 addition & 0 deletions lib/Parser/Scan.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ class Scanner : public IScanner, public EncodingPolicy
}
};

tokens GetPrevious() { return m_tkPrevious; }
void Capture(_Out_ RestorePoint* restorePoint);
void SeekTo(const RestorePoint& restorePoint);
void SeekToForcingPid(const RestorePoint& restorePoint);
Expand Down
10 changes: 9 additions & 1 deletion lib/Parser/cmperr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@
//-------------------------------------------------------------------------------------------------------
#include "ParserPch.h"

ParseExceptionObject::ParseExceptionObject(HRESULT hr) : m_hr(hr)
ParseExceptionObject::ParseExceptionObject(HRESULT hr, LPCWSTR stringOneIn, LPCWSTR stringTwoIn)
{
m_hr = hr;
stringOne = SysAllocString(stringOneIn);
stringTwo = SysAllocString(stringTwoIn);
Assert(FAILED(hr));
}

ParseExceptionObject::~ParseExceptionObject()
{
SysFreeString(stringOne);
SysFreeString(stringTwo);
}
7 changes: 6 additions & 1 deletion lib/Parser/cmperr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ enum
class ParseExceptionObject
{
public:
ParseExceptionObject(HRESULT hr);
ParseExceptionObject(HRESULT hr, LPCWSTR stringOneIn = _u(""), LPCWSTR stringTwoIn = _u(""));
~ParseExceptionObject();
HRESULT GetError() { return m_hr; }
LPCWSTR GetStringOne() { return stringOne; }
LPCWSTR GetStringTwo() { return stringTwo; }
private:
HRESULT m_hr;
BSTR stringOne;
BSTR stringTwo;
};
9 changes: 9 additions & 0 deletions lib/Parser/perrors.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,12 @@ LSC_ERROR_MSG(1093, ERRLabelBeforeClassDeclaration, "Labels not allowed before c
LSC_ERROR_MSG(1094, ERRLabelFollowedByEOF, "Unexpected end of script after a label.")
LSC_ERROR_MSG(1095, ERRFunctionAfterLabelInStrict, "Function declarations not allowed after a label in strict mode.")
LSC_ERROR_MSG(1096, ERRAwaitAsLabelInAsync, "Use of 'await' as label in async function is not allowed.")
LSC_ERROR_MSG(1097, ERRExperimental, "Use of disabled experimental feature")
//1098-1199 available for future use

// Generic errors intended to be re-usable
LSC_ERROR_MSG(1200, ERRKeywordAfter, "Unexpected keyword '%s' after '%s'")
LSC_ERROR_MSG(1201, ERRTokenAfter, "Unexpected token '%s' after '%s'")
LSC_ERROR_MSG(1202, ERRIdentifierAfter, "Unexpected identifier '%s' after '%s'")
LSC_ERROR_MSG(1203, ERRInvalidIdentifier, "Unexpected invalid identifier '%s' after '%s'")
LSC_ERROR_MSG(1205, ERRValidIfFollowedBy, "%s is only valid if followed by %s")
9 changes: 8 additions & 1 deletion lib/Parser/screrror.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ void CompileScriptException::CopyInto(CompileScriptException* pse)
}
}

HRESULT CompileScriptException::ProcessError(IScanner * pScan, HRESULT hr, ParseNode * pnodeBase)
HRESULT CompileScriptException::ProcessError(IScanner * pScan, HRESULT hr, ParseNode * pnodeBase, LPCWSTR stringOne, LPCWSTR stringTwo)
{
// fill in the ScriptException structure
Free();
Expand All @@ -267,6 +267,13 @@ HRESULT CompileScriptException::ProcessError(IScanner * pScan, HRESULT hr, Pars
if (nullptr == (ei.bstrDescription = SysAllocString(szT)))
ei.scode = E_OUTOFMEMORY;
}
else if (wcslen(stringOne) > 0)
{
OLECHAR szT[128];
_snwprintf_s(szT, ARRAYSIZE(szT), ARRAYSIZE(szT)-1, ei.bstrDescription, stringOne, stringTwo);
SysFreeString(ei.bstrDescription);
ei.bstrDescription = SysAllocString(szT);
}

ei.bstrSource = BstrGetResourceString(IDS_COMPILATION_ERROR_SOURCE);
if (nullptr == pnodeBase && nullptr != pScan)
Expand Down
2 changes: 1 addition & 1 deletion lib/Parser/screrror.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class CompileScriptException : public ScriptException

void CopyInto(CompileScriptException* cse);

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

friend class ActiveScriptError;
};
Loading

0 comments on commit 42ba7b9

Please sign in to comment.