Misc. fixes and improvements for unused assignment detection

source/compiler/sc.h

@@ -239,6 +239,8 @@ typedef struct s_symbol {
  * so the compiler would know it shouldn't set the uLOOPVAR flag when the variable
  * is read inside a loop condition */
 #define uNOLOOPVAR  0x2000
+/* uMODIFIED indicates that the value of a variable was modified, but not used yet */
+#define uMODIFIED   0x4000
 
 #define flagDEPRECATED 0x01  /* symbol is deprecated (avoid use) */
 #define flagNAKED     0x10  /* function is naked */
@@ -319,7 +321,7 @@ typedef struct s_valuepair {
 typedef struct s_assigninfo {
   int lnumber;      /* line number of the first unused assignment made in one of
                      * the branches (used for error messages) */
-  short usage;      /* usage flags to memoize (currently only uASSIGNED) */
+  short usage;      /* usage flags to memoize */
 } symstate;
 
 /* macros for code generation */
@@ -748,7 +750,7 @@ SC_FUNC void delete_symbol(symbol *root,symbol *sym);
 SC_FUNC void delete_symbols(symbol *root,int level,int delete_labels,int delete_functions);
 SC_FUNC int refer_symbol(symbol *entry,symbol *bywhom);
 SC_FUNC void markusage(symbol *sym,int usage);
-SC_FUNC void markinitialized(symbol *sym,int assignment);
+SC_FUNC void markinitialized(symbol *sym,int assignment,int modification);
 SC_FUNC void clearassignments(int fromlevel);
 SC_FUNC void memoizeassignments(int fromlevel,symstate **assignments);
 SC_FUNC void restoreassignments(int fromlevel,symstate *assignments);

source/compiler/sc1.c

@@ -2350,7 +2350,7 @@ static void declglb(char *firstname,int firsttag,int fpublic,int fstatic,int fst
     if (fstatic)
       sym->fnumber=filenum;
     if (explicit_init)
-      markinitialized(sym,TRUE);
+      markinitialized(sym,TRUE,FALSE);
     sc_attachdocumentation(sym);/* attach any documentation to the variable */
     if (sc_status==statSKIP) {
       sc_status=statWRITE;
@@ -2549,7 +2549,7 @@ static int declloc(int fstatic)
       } /* if */
     } /* if */
     if (explicit_init)
-      markinitialized(sym,!suppress_w240);
+      markinitialized(sym,!suppress_w240,FALSE);
     if (pc_ovlassignment)
       sym->usage |= uREAD;
     if (matchtoken(t__PRAGMA))
@@ -6116,8 +6116,8 @@ static int dowhile(void)
   pc_loopcond=tWHILE;
   endlessloop=test(wq[wqEXIT],TEST_DO,FALSE);/* branch to wq[wqEXIT] if false */
   pc_loopcond=0;
-  pc_nestlevel--;
   statement(NULL,FALSE);        /* if so, do a statement */
+  pc_nestlevel--;
   clearassignments(pc_nestlevel+1);
   testloopvariables(loopvars,FALSE,loopline);
   jumplabel(wq[wqLOOP]);        /* and loop to "while" start */
@@ -6555,8 +6555,17 @@ static int dogoto(void)
 
   if (lex(&val,&st)==tSYMBOL) {
     sym=fetchlab(st);
-    if ((sym->usage & uDEFINE)!=0)
+    if ((sym->usage & uDEFINE)!=0) {
       clearassignments(1);
+    } else if (wqptr>wq) {
+      /* The label is not defined yet, which means it must be defined after the
+       * 'goto'. If we're inside of a loop, the target label may or may not be
+       * defined inside of the same loop - we can't know that ahead of time
+       * due to how the compiler works, so the best we can do for now is clear
+       * all the assignments at the current compound statement nesting level
+       * in order to avoid false-positives of warning 240. */
+      clearassignments(pc_nestlevel);
+    } /* if */
     jumplabel((int)sym->addr);
     sym->usage|=uREAD;  /* set "uREAD" bit */
     if ((sym->usage & uDEFINE)!=0) {
@@ -8680,7 +8689,7 @@ SC_FUNC void pragma_unused(symbol *sym, int unread, int unwritten)
   /* mark as read if the pragma wasn't "unwritten" */
   if (!unwritten) {
     sym->usage |= uREAD;
-    sym->usage &= ~uASSIGNED;
+    sym->usage &= ~(uASSIGNED | uMODIFIED);
   } /* if */
   /* mark as written if the pragma wasn't "unread" */
   if (sym->ident == iVARIABLE || sym->ident == iREFERENCE

source/compiler/sc2.c

@@ -3202,13 +3202,18 @@ SC_FUNC void delete_symbols(symbol *root,int level,int delete_labels,int delete_
       mustdelete=delete_labels;
       break;
     case iVARIABLE:
-      /* check that the assigned value was used, but don't show the warning
+      /* check that the assigned/modified value was used, but don't show the warning
        * if the variable is completely unused (we already have warning 203 for that) */
-      if ((sym->usage & (uASSIGNED | uREAD | uWRITTEN))==(uASSIGNED | uREAD | uWRITTEN)
-          && sym->vclass==sLOCAL) {
-        errorset(sSETPOS,sym->lnumber);
-        error(204,sym->name);   /* symbol is assigned a value that is never used */
-        errorset(sSETPOS,-1);
+      if (sym->vclass==sLOCAL) {
+        if ((sym->usage & (uREAD | uWRITTEN | uASSIGNED))==(uREAD | uWRITTEN | uASSIGNED)) {
+          errorset(sSETPOS,sym->lnumber);
+          error(204,sym->name);   /* symbol is assigned a value that is never used */
+          errorset(sSETPOS,-1);
+        } else if ((sym->usage & (uREAD | uWRITTEN | uMODIFIED))==(uREAD | uWRITTEN | uMODIFIED)) {
+          errorset(sSETPOS,sym->lnumber);
+          error(252,sym->name);   /* symbol has its value modified but never used */
+          errorset(sSETPOS,-1);
+        } /* if */
       } /* if */
       /* fallthrough */
     case iARRAY:
@@ -3387,7 +3392,7 @@ SC_FUNC void markusage(symbol *sym,int usage)
   if ((usage & uWRITTEN)!=0)
     sym->lnumber=fline;
   if ((usage & uREAD)!=0 && (sym->ident==iVARIABLE || sym->ident==iREFERENCE))
-    sym->usage &= ~uASSIGNED;
+    sym->usage &= ~(uASSIGNED | uMODIFIED);
   if ((usage & (uREAD | uWRITTEN))!=0 && (sym->ident==iVARIABLE || sym->ident==iREFERENCE))
     markloopvariable(sym,usage);
   /* check if (global) reference must be added to the symbol */
@@ -3402,15 +3407,17 @@ SC_FUNC void markusage(symbol *sym,int usage)
   } /* if */
 }
 
-SC_FUNC void markinitialized(symbol *sym,int assignment)
+SC_FUNC void markinitialized(symbol *sym,int assignment,int modification)
 {
   assert(sym!=NULL);
+  assert(!assignment | !modification);
   if (sym->ident!=iVARIABLE && sym->ident!=iREFERENCE && sym->ident!=iARRAY)
     return;
   if (sc_status==statFIRST && (sym->vclass==sLOCAL || sym->vclass==sSTATIC))
     return;
-  if (assignment && sym->vclass==sLOCAL && (sym->ident==iVARIABLE || sym->ident==iREFERENCE)) {
-    sym->usage |= uASSIGNED;
+  if ((assignment || modification) && sym->vclass==sLOCAL
+      && (sym->ident==iVARIABLE || sym->ident==iREFERENCE)) {
+    sym->usage |= assignment ? uASSIGNED : uMODIFIED;
     sym->assignlevel=pc_nestlevel;
   } /* if */
 }
@@ -3428,7 +3435,7 @@ SC_FUNC void clearassignments(int fromlevel)
   sym=&loctab;
   while ((sym=sym->next)!=NULL)
     if (sym->assignlevel>=fromlevel)
-      sym->usage &= ~uASSIGNED;
+      sym->usage &= ~(uASSIGNED | uMODIFIED);
 }
 
 /* memoizes all assignments done on the specified compound level and higher */
@@ -3460,18 +3467,22 @@ SC_FUNC void memoizeassignments(int fromlevel,symstate **assignments)
   sym=&loctab;
   while ((sym=sym->next)!=NULL && sym->ident==iLABEL) {}    /* skip labels */
   for (num=0; sym!=NULL; num++,sym=sym->next) {
-    /* if the assignment is unused and it was done inside the branch... */
-    if ((sym->usage & uASSIGNED)!=0 && sym->assignlevel>=fromlevel) {
-      /* clear the assignment flag, so the compiler won't report this assignment as unused
-       * if the next "if" or "switch" branch also contains an assignment to this variable */
-      sym->usage &= ~uASSIGNED;
-      /* memoize the assignment only if there was no other unused assignment
-       * in any other "if" or "switch" branch */
+    int flag=sym->usage & (uASSIGNED | uMODIFIED);
+    /* can't have both flags set at the same time */
+    assert((flag & uASSIGNED)==0 || (flag & uMODIFIED)==0);
+    /* if the assignment/modification is unused and it was done inside the branch... */
+    if (flag!=0 && sym->assignlevel>=fromlevel) {
+      /* memoize the assignment/modification only if there was no other unused
+       * assignment or modification in any other "if" or "switch" branch */
       assert_static(sizeof(sym->usage)==sizeof((*assignments)->usage));
-      if (((*assignments)[num].usage & uASSIGNED)==0) {
+      if (((*assignments)[num].usage & (uASSIGNED | uMODIFIED))==0) {
         (*assignments)[num].lnumber=sym->lnumber;
-        (*assignments)[num].usage |= uASSIGNED;
+        (*assignments)[num].usage |= flag;
       } /* if */
+      /* unset the assignment/modification flag for the variable, so the compiler
+       * won't report the assignment/modification as unused if the next "if" or
+       * "switch" branch also contains an assignment to this variable */
+      sym->usage &= ~(uASSIGNED | uMODIFIED);
     } /* if */
   } /* for */
 }
@@ -3485,8 +3496,9 @@ SC_FUNC void restoreassignments(int fromlevel,symstate *assignments)
   sym=&loctab;
   while ((sym=sym->next)!=NULL && sym->ident==iLABEL) {}    /* skip labels */
   for (num=0; sym!=NULL; num++,sym=sym->next) {
-    if (assignments!=NULL && (assignments[num].usage & uASSIGNED)!=0) {
-      sym->usage |= uASSIGNED;
+    int flag;
+    if (assignments!=NULL && (flag=(assignments[num].usage & (uASSIGNED | uMODIFIED)))!=0) {
+      sym->usage |= flag;
       sym->lnumber=assignments[num].lnumber;
     } /* if */
     /* demote all assignments that were made inside any of the "if"/"switch"

source/compiler/sc3.c

@@ -194,8 +194,6 @@ static void (*unopers[])(void) = { lneg, neg, user_inc, user_dec };
     if (sym==NULL /*|| (sym->usage & uDEFINE)==0*/)
       return FALSE;
   } /* if */
-  if (oper==NULL)
-    pc_ovlassignment=TRUE;
 
   /* check existence and the proper declaration of this function */
   if ((sym->usage & uMISSING)!=0 || (sym->usage & uPROTOTYPED)==0) {
@@ -285,6 +283,8 @@ static void (*unopers[])(void) = { lneg, neg, user_inc, user_dec };
     store(lval);                /* store PRI in the symbol */
     moveto1();                  /* make sure PRI is restored on exit */
   } /* if */
+  if (oper==NULL)
+    pc_ovlassignment=TRUE;
   return TRUE;
 }
 
@@ -1104,6 +1104,17 @@ static int hier14(value *lval1)
     if (lval2.ident==iARRAY || lval2.ident==iREFARRAY)
       error(6);         /* must be assigned to an array */
   } /* if */
+  /* check if the previously assigned value was used (it's important to do this
+   * before generating the code for storing the value into the variable, as the latter
+   * might clobber the line number of the previous assignment used by warning 240) */
+  assert(lval3.sym!=NULL);
+  if (oper==NULL && lval3.sym->vclass==sLOCAL && (lval3.sym->usage & uASSIGNED)!=0
+      && lval3.sym->assignlevel>=pc_nestlevel) {
+    errorset(sSETPOS,lval3.sym->lnumber);
+    error(240,lval3.sym->name);     /* assigned/modified value is never used */
+    errorset(sSETPOS,-1);
+  } /* if */
+  /* store the expression result and mark the lvalue as modified */
   if (leftarray) {
     memcopy(val*sizeof(cell));
   } else {
@@ -1113,19 +1124,15 @@ static int hier14(value *lval1)
   } /* if */
   if (!oper)
     check_tagmismatch(lval3.tag,lval2.tag,TRUE,-1); /* tagname mismatch (if "oper", warning already given in plunge2()) */
-  if (lval3.sym)
-    markusage(lval3.sym,uWRITTEN);
+  markusage(lval3.sym,uWRITTEN);
   pc_sideeffect=TRUE;
   bitwise_opercount=bwcount;
   lval1->ident=iEXPRESSION;
-  if (oper==NULL) {
-    symbol *sym=lval3.sym;
-    assert(sym!=NULL);
-    if ((sym->usage & uASSIGNED)!=0 && sym->assignlevel>=pc_nestlevel && sym->vclass==sLOCAL)
-      error(240,sym->name); /* previously assigned value is unused */
-    markinitialized(sym,TRUE);
-    if (pc_ovlassignment)
-      markusage(sym,uREAD);
+  if (!pc_ovlassignment) {
+    if (oper==NULL)
+      markinitialized(lval3.sym,TRUE,FALSE);
+    else
+      markinitialized(lval3.sym,FALSE,TRUE);
   } /* if */
   return FALSE;         /* expression result is never an lvalue */
 }
@@ -1312,6 +1319,7 @@ static int hier2(value *lval)
     } /* if */
     rvalue(lval);               /* and read the result into PRI */
     pc_sideeffect=TRUE;
+    markinitialized(lval->sym,FALSE,TRUE);
     return FALSE;               /* result is no longer lvalue */
   case tDEC:                    /* --lval */
     if (!hier2(lval))
@@ -1326,6 +1334,7 @@ static int hier2(value *lval)
     } /* if */
     rvalue(lval);               /* and read the result into PRI */
     pc_sideeffect=TRUE;
+    markinitialized(lval->sym,FALSE,TRUE);
     return FALSE;               /* result is no longer lvalue */
   case '~':                     /* ~ (one's complement) */
     if (hier2(lval))
@@ -1724,6 +1733,7 @@ static int hier2(value *lval)
         if (saveresult)
           popreg(sPRI);         /* restore PRI (result of rvalue()) */
         pc_sideeffect=TRUE;
+        markinitialized(lval->sym,FALSE,TRUE);
         return FALSE;           /* result is no longer lvalue */
       case tDEC:                /* lval-- */
         if (!lvalue)
@@ -1745,6 +1755,7 @@ static int hier2(value *lval)
         if (saveresult)
           popreg(sPRI);         /* restore PRI (result of rvalue()) */
         pc_sideeffect=TRUE;
+        markinitialized(lval->sym,FALSE,TRUE);
         return FALSE;
       case tCHAR:               /* char (compute required # of cells */
         if (lval->ident==iCONSTEXPR) {

source/compiler/sc5.c

@@ -198,7 +198,7 @@ static char *warnmsg[] = {
 /*237*/  "user warning: %s\n",
 /*238*/  "meaningless combination of class specifiers (%s)\n",
 /*239*/  "literal array/string passed to a non-const parameter\n",
-/*240*/  "previously assigned value is never used (symbol \"%s\")\n",
+/*240*/  "assigned value is never used (symbol \"%s\")\n",
 /*241*/  "negative or too big shift count\n",
 /*242*/  "shift overflow in enum element declaration (symbol \"%s\")\n",
 /*243*/  "redundant code: switch control expression is constant\n",
@@ -209,7 +209,8 @@ static char *warnmsg[] = {
 /*248*/  "possible misuse of comma operator\n",
 /*249*/  "check failed: %s\n",
 /*250*/  "variable \"%s\" used in loop condition not modified in loop body\n",
-/*251*/  "none of the variables used in loop condition are modified in loop body\n"
+/*251*/  "none of the variables used in loop condition are modified in loop body\n",
+/*252*/  "variable has its value modified but never used: \"%s\"\n"
 };
 
 static char *noticemsg[] = {

source/compiler/tests/warning_240.meta

@@ -1,20 +1,20 @@
 {
   'test_type': 'output_check',
   'errors': """
-warning_240.pwn(12) : warning 240: previously assigned value is never used (symbol "local_var")
+warning_240.pwn(11) : warning 240: assigned value is never used (symbol "local_var")
 warning_240.pwn(14) : warning 204: symbol is assigned a value that is never used: "local_var"
-warning_240.pwn(38) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(52) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(66) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(74) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(85) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(97) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(109) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(120) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(139) : warning 240: previously assigned value is never used (symbol "local_var")
-warning_240.pwn(174) : warning 240: previously assigned value is never used (symbol "arg")
-warning_240.pwn(178) : warning 240: previously assigned value is never used (symbol "arg")
-warning_240.pwn(183) : warning 240: previously assigned value is never used (symbol "refarg")
+warning_240.pwn(35) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(46) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(63) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(71) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(82) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(93) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(106) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(114) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(136) : warning 240: assigned value is never used (symbol "local_var")
+warning_240.pwn(168) : warning 240: assigned value is never used (symbol "arg")
+warning_240.pwn(176) : warning 240: assigned value is never used (symbol "arg")
+warning_240.pwn(182) : warning 240: assigned value is never used (symbol "refarg")
 warning_240.pwn(178) : warning 204: symbol is assigned a value that is never used: "arg"
 warning_240.pwn(212) : warning 204: symbol is assigned a value that is never used: "local_var"
 """