parser: Fix/improve error recovery for procedure calls

This fixes a compiler crash during error recovery for BYREF AS ANY
parameters and (in theory) improves error recovery for procedure calls.

http://www.freebasic.net/forum/viewtopic.php?f=3&t=24140

The parser calls astNewCONSTz() to build some argument expression for
error recovery, but for ANY parameters, that returned NULL, causing
crashes later in astNewARG() which doesn't expect NULL.

Besides BYREF AS ANY parameters, it looks like astNewCONSTz() can only be
called with ANY type in case of SUBs (which internally use FB_DATATYPE_VOID
as type).

This patch changes the parser to build "proper" CALL expressions for error
recovery in the call parser, and it also changes astNewCONSTz() to never
return NULL anymore. If an ANY expression is requested, it just assumes
to be able to return an expression with any type.

This also means that cProcArgList() doesn't ever return NULL or a CONST
node anymore, but always a CALL node, which allows some further clean-ups.

Author: dkl

changelog.txt

@@ -15,6 +15,7 @@ Version 1.05.0
 - #print typeof() output now differentiates between ZSTRING and ZSTRING * N (ZSTRING without size is produced by dereferencing a ZSTRING PTR, or BYREF AS ZSTRING)
 - Context-specific keywords, e.g. graphics PUT modes, must now be given as keywords (e.g. PSET), string literals (e.g. "PSET") are no longer accepted.
 - Wstring-to-Zstring conversions didn't use the system's Unicode <-> codepage conversion function, and only converted ASCII characters. Now it will try to convert the Unicode chars to codepage chars.
+- Compiler crash during error recovery when there was an error when parsing the argument expression for a BYREF AS ANY parameter
 
 
 Version 1.04.0

src/compiler/ast-node-call.bas

@@ -531,3 +531,17 @@ function astIgnoreCallResult( byval n as ASTNODE ptr ) as ASTNODE ptr
 	astSetType( n, FB_DATATYPE_VOID, NULL )
 	function = n
 end function
+
+'' Build a fake CALL to the given procedure, to be used for error recovery.
+function astBuildFakeCall( byval proc as FBSYMBOL ptr ) as ASTNODE ptr
+	var c = astNewCALL( proc )
+
+	'' Build fake arg for each param (including the THIS param for methods)
+	var param = symbGetProcHeadParam( proc )
+	while( param )
+		astNewARG( c, astNewCONSTz( symbGetType( param ), param->subtype ) )
+		param = param->next
+	wend
+
+	function = c
+end function

src/compiler/ast-node-const.bas

@@ -125,9 +125,9 @@ function astNewCONSTz _
 	select case as const( typeGetDtAndPtrOnly( dtype ) )
 	case FB_DATATYPE_VOID
 		'' A CONST expression with VOID type doesn't make sense,
-		'' but some callers of astNewCONSTz() can call it for SUBs,
-		'' which have a VOID result type.
-		function = NULL
+		'' but astNewCONSTz() can be called for BYREF AS ANY parameters.
+		'' Any expression type should be ok for error recovery.
+		function = astNewCONSTi( 0 )
 
 	case FB_DATATYPE_STRING, FB_DATATYPE_FIXSTR, FB_DATATYPE_CHAR
 		function = astNewCONSTstr( NULL )

src/compiler/ast.bi

@@ -1307,6 +1307,7 @@ declare function astBuildByrefResultDeref( byval expr as ASTNODE ptr ) as ASTNOD
 declare function astIsByrefResultDeref( byval expr as ASTNODE ptr ) as integer
 declare function astRemoveByrefResultDeref( byval expr as ASTNODE ptr ) as ASTNODE ptr
 declare function astIgnoreCallResult( byval n as ASTNODE ptr ) as ASTNODE ptr
+declare function astBuildFakeCall( byval proc as FBSYMBOL ptr ) as ASTNODE ptr
 
 declare sub astGosubAddInit( byval proc as FBSYMBOL ptr )
 

src/compiler/parser-expr-function.bas

@@ -56,9 +56,6 @@ function cFunctionCall _
 			lexSkipToken( )
 
 			funcexpr = cProcArgList( base_parent, sym, ptrexpr, @arg_list, options )
-			if( funcexpr = NULL ) then
-				exit function
-			end if
 
 			'' ')'
 			hParseRPNT( )
@@ -70,14 +67,8 @@ function cFunctionCall _
 			end if
 
 			'' no args
-			funcexpr = cProcArgList( base_parent, _
-									 sym, _
-									 ptrexpr, _
-									 @arg_list, _
-									 options or FB_PARSEROPT_OPTONLY )
-			if( funcexpr = NULL ) then
-				exit function
-			end if
+			funcexpr = cProcArgList( base_parent, sym, ptrexpr, @arg_list, _
+			                         options or FB_PARSEROPT_OPTONLY )
 		end if
 
 	else
@@ -87,23 +78,14 @@ function cFunctionCall _
 
 			'' ProcArgList
 			funcexpr = cProcArgList( base_parent, sym, ptrexpr, @arg_list, options )
-			if( funcexpr = NULL ) then
-				exit function
-			end if
 
 			'' ')'
 			hParseRPNT( )
 
 		else
 			'' ProcArgList (function could have optional params)
-			funcexpr = cProcArgList( base_parent, _
-									 sym, _
-									 ptrexpr, _
-									 @arg_list, _
-									 options or FB_PARSEROPT_OPTONLY )
-			if( funcexpr = NULL ) then
-				exit function
-			end if
+			funcexpr = cProcArgList( base_parent, sym, ptrexpr, @arg_list, _
+			                         options or FB_PARSEROPT_OPTONLY )
 		end if
 
 	end if
@@ -198,15 +180,9 @@ function cCtorCall _
 	arg->expr = astNewVAR( tmp )
 	arg->mode = INVALID
 
-	procexpr = cProcArgList( NULL, _
-							 symbGetCompCtorHead( sym ), _
-					  		 NULL, _
-					  		 @arg_list, _
-					  		 FB_PARSEROPT_ISFUNC or _
-					  		 FB_PARSEROPT_HASINSTPTR or _
-					  		 iif( isprnt = FALSE, _
-					  		 	  FB_PARSEROPT_OPTONLY, _
-					  		 	  FB_PARSEROPT_NONE ) )
+	procexpr = cProcArgList( NULL, symbGetCompCtorHead( sym ), NULL, @arg_list, _
+	                         FB_PARSEROPT_ISFUNC or FB_PARSEROPT_HASINSTPTR or _
+	                         iif( isprnt = FALSE, FB_PARSEROPT_OPTONLY, FB_PARSEROPT_NONE ) )
 
 	'' Try to parse the ')' even in case of error recovery; it belongs to
 	'' the type() construct afterall, and nothing else would handle it.
@@ -222,13 +198,5 @@ function cCtorCall _
 		end if
 	end if
 
-	'' cProcArgList() usually returns a CALL, but it can be NULL or a CONST
-	'' etc. in case of error recovery
-	if( procexpr = NULL ) then
-		function = NULL
-	elseif( astIsCALL( procexpr ) = FALSE ) then
-		function = procexpr
-	else
-		function = astNewCALLCTOR( procexpr, astNewVAR( tmp ) )
-	end if
+	function = astNewCALLCTOR( procexpr, astNewVAR( tmp ) )
 end function

src/compiler/parser-proccall-args.bas

@@ -362,7 +362,7 @@ private function hOvlProcArgList _
 
 		errReportParam( proc, 0, NULL, err_num )
 		'' error recovery: fake an expr
-		return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+		return astBuildFakeCall( proc )
 	end if
 
 	proc = ovlproc
@@ -374,7 +374,7 @@ private function hOvlProcArgList _
 		                  FB_ERRMSG_ILLEGALMEMBERACCESS ), _
 		             symbGetFullProcName( proc ) )
 		'' error recovery: fake an expr
-		return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+		return astBuildFakeCall( proc )
 	end if
 
     '' method?
@@ -386,7 +386,7 @@ private function hOvlProcArgList _
 			if( (base_parent <> NULL) or (symbIsMethod( parser.currproc ) = FALSE) ) then
 				errReport( FB_ERRMSG_MEMBERISNTSTATIC, TRUE )
 				'' error recovery: fake an expr
-				return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+				return astBuildFakeCall( proc )
 			end if
 
 			'' pass the instance ptr of the current method
@@ -422,7 +422,8 @@ private function hOvlProcArgList _
 			errReport( FB_ERRMSG_PARAMTYPEMISMATCH )
 			'' error recovery: fake an expr (don't try to fake an arg,
 			'' different modes and param types like "as any" would break AST)
-			return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+			astDelTree( procexpr )
+			return astBuildFakeCall( proc )
 		end if
 
 		symbFreeOvlCallArg( @parser.ovlarglist, arg )
@@ -448,6 +449,9 @@ end function
 '':::::
 ''ProcArgList     =    ProcArg (DECL_SEPARATOR ProcArg)* .
 ''
+'' This function always returns a CALL, even for error recovery - so callers
+'' don't have to check before passing it to astNewCALLCTOR() etc.
+''
 function cProcArgList _
 	( _
 		byval base_parent as FBSYMBOL ptr, _
@@ -480,7 +484,7 @@ function cProcArgList _
 		                  FB_ERRMSG_ILLEGALMEMBERACCESS ), _
 		             symbGetFullProcName( proc ) )
 		'' error recovery: fake an expr
-		return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+		return astBuildFakeCall( proc )
 	end if
 
     '' method?
@@ -492,7 +496,7 @@ function cProcArgList _
 			if( (base_parent <> NULL) or (symbIsMethod( parser.currproc ) = FALSE) ) then
 				errReport( FB_ERRMSG_MEMBERISNTSTATIC, TRUE )
 				'' error recovery: fake an expr
-				return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+				return astBuildFakeCall( proc )
 			end if
 
 			'' pass the instance ptr of the current method
@@ -529,7 +533,8 @@ function cProcArgList _
 		dim as FB_CALL_ARG ptr nxt = arg->next
 
 		if( astNewARG( procexpr, arg->expr, , arg->mode ) = NULL ) then
-			exit function
+			astDelTree( procexpr )
+			return astBuildFakeCall( proc )
 		end if
 
 		symbFreeOvlCallArg( @parser.ovlarglist, arg )
@@ -597,7 +602,7 @@ function cProcArgList _
 				'' don't try to fake an arg, different modes and param
 				'' types like "as any" would break AST
 				astDelTree( procexpr )
-				return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+				return astBuildFakeCall( proc )
 			end if
 
 			'' next
@@ -624,12 +629,13 @@ function cProcArgList _
 			errReport( FB_ERRMSG_ARGCNTMISMATCH )
 			'' error recovery: fake an expr
 			astDelTree( procexpr )
-			return astNewCONSTz( symbGetType( proc ), symbGetSubType( proc ) )
+			return astBuildFakeCall( proc )
 		end if
 
 		'' add to tree
 		if( astNewARG( procexpr, NULL ) = NULL ) then
-			exit function
+			astDelTree( procexpr )
+			return astBuildFakeCall( proc )
 		end if
 
 		'' next

src/compiler/parser-proccall.bas

@@ -349,10 +349,6 @@ function cProcCall _
 
 	'' ProcArgList
 	procexpr = cProcArgList( base_parent, sym, ptrexpr, @arg_list, options )
-	if( procexpr = NULL ) then
-		hSkipUntil( CHAR_RPRNT )
-		exit function
-	end if
 
 	'' ')'
 	if( (checkprnts) or (parser.prntcnt > 0) ) then
@@ -974,28 +970,19 @@ private sub hBaseInit( )
 	lexSkipToken( )
 
 	subtype = symbGetSubtype( base_ )
-	initree = NULL
 
 	'' Has a ctor?
 	if( symbGetCompCtorHead( subtype ) ) then
 		'' CtorCall
 		ctorcall = cCtorCall( subtype )
-		if( ctorcall ) then
-			'' Will be a CTORCALL except in case of error recovery
-			if( astIsCALLCTOR( ctorcall ) ) then
-				'' cCtorCall() created a temporary object to
-				'' call the constructor on, we delete it though:
-				ctorcall = astCALLCTORToCALL( ctorcall )
-
-				'' Turn the ctorcall into an initree
-				initree = astTypeIniBegin( FB_DATATYPE_STRUCT, subtype, TRUE )
-				astTypeIniAddCtorCall( initree, base_, ctorcall )
-				astTypeIniEnd( initree, TRUE )
-			else
-				astDelTree( ctorcall )
-				ctorcall = NULL
-			end if
-		end if
+		'' cCtorCall() created a temporary object to
+		'' call the constructor on, we delete it though:
+		ctorcall = astCALLCTORToCALL( ctorcall )
+
+		'' Turn the ctorcall into an initree
+		initree = astTypeIniBegin( FB_DATATYPE_STRUCT, subtype, TRUE )
+		astTypeIniAddCtorCall( initree, base_, ctorcall )
+		astTypeIniEnd( initree, TRUE )
 	else
 		'' Initializer
 		initree = cInitializer( base_, FB_INIOPT_ISINI )
@@ -1153,16 +1140,6 @@ function hForwardCall( ) as integer
 		exit function
     end if
 
-    ''
-    dim as ASTNODE ptr procexpr = cProcArgList( NULL, _
-    											proc, _
-    											NULL, _
-    											@arg_list, _
-    											FB_PARSEROPT_OPTONLY )
-    if( procexpr <> NULL ) then
-    	astAdd( procexpr )
-    end if
-
+	astAdd( cProcArgList( NULL, proc, NULL, @arg_list, FB_PARSEROPT_OPTONLY ) )
 	function = TRUE
-
 end function

src/compiler/parser-quirk-casting.bas

@@ -199,12 +199,7 @@ function cAnonType( ) as ASTNODE ptr
 	if( typeGetDtAndPtrOnly( dtype ) = FB_DATATYPE_STRUCT ) then
 		'' Has a ctor?
 		if( symbGetCompCtorHead( subtype ) ) then
-			initree = cCtorCall( subtype )
-			if( initree = NULL ) then
-				'' Error recovery
-				initree = astNewCONSTz( dtype, subtype )
-			end if
-			return initree
+			return cCtorCall( subtype )
 		end if
 	end if
 

src/compiler/parser-quirk-mem.bas

@@ -134,9 +134,6 @@ function cOperatorNew( ) as ASTNODE ptr
 				errReport( FB_ERRMSG_EXPLICITCTORCALLINVECTOR, TRUE )
 			else
 				initexpr = cCtorCall( subtype )
-				if( initexpr = NULL ) then
-					return NULL
-				end if
 			end if
 		else
 			dim as FBSYMBOL ptr ctor = symbGetCompDefCtor( subtype )
@@ -147,9 +144,6 @@ function cOperatorNew( ) as ASTNODE ptr
 				'' only if not a vector
 				if( op <> AST_OP_NEW_VEC ) then
 					initexpr = cCtorCall( subtype )
-					if( initexpr = NULL ) then
-						return NULL
-					end if
 				else
 					'' check visibility
 					if( symbCheckAccess( ctor ) = FALSE ) then

tests/functions/byref-as-any-error-recovery.bas

@@ -0,0 +1,5 @@
+' TEST_MODE : COMPILE_ONLY_FAIL
+
+'' Testing error recovery while parsing the argument expression for a
+'' BYREF AS ANY parameter
+clear tests(0), , 1