235930 - Javascript (Switch+)|Case:| handling issues a false "function does not always return a value" warning on an given code pattern

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

This is spun off from bug 235879 comment 3:
{{{
Yes, the strict warning stuff for return vs. falling off the end of a function
in jsparse.c is cheesy.  Please file a bug on me.  Thanks,
}}}

Reminder on bug 235879 testcase:
{{{
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)

Testcase:
1. From MailNews (3-pane), double-click a message to open it in a new window !

> Part B:
> {{
> Warning: anonymous function does not always return a value
> Source File: chrome://messenger/content/messageWindow.js
> Line: 857, Column: 1
> Source Code:
> 	},
> }}

The involved code is:
(I added a comment on the 3 places which trigger this warning)
{{
      case "cmd_createFilterFromPopup":
      case "cmd_createFilterFromMenu":
        loadedFolder = GetLoadedMsgFolder();
        if (!(loadedFolder && loadedFolder.server.canHaveFilters))
          return false;
        // *** Place number 1 ***
			case "cmd_delete":
        UpdateDeleteCommand();
        // fall through
        // *** Place number 2 ***
			case "button_delete":
			case "cmd_shiftDelete":
        loadedFolder = GetLoadedMsgFolder();
        return gCurrentMessageUri && loadedFolder &&
(loadedFolder.canDeleteMessages || isNewsURI(gCurrentFolderUri));
      case "button_junk":
        UpdateJunkToolbarButton();
        // fall through
        // *** Place number 3 ***
      case "cmd_markAsJunk":
			case "cmd_markAsNotJunk":
      case "cmd_recalculateJunkScore":
        // can't do junk on news yet
        return (!isNewsURI(gCurrentFolderUri));
}}
The JS engine fails to take into account the |return| which is after the next
|case "xxx" :| :-(
}}}

Updating:
*(A.To) [e-mail] -> brendan@m.o, per explicit request.

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix, plus a few context-sensitive regexp scanning fixes

Narcissus shook out the context-sensitive lexical analysis fixes.

/be

Comment 2

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)

(for the record)
Odd:
Either I don't know how to trigger it correctly !??
or this bug is not showing up in <mail3PaneWindowCommands.js> !!?
{{
      case "cmd_delete":
        UpdateDeleteCommand();
        // fall through
      case "button_delete":
        if (gDBView)
          gDBView.getCommandStatus(nsMsgViewCommandType.deleteMsg, enabled,
checkStatus);
        return enabled.value;
}}

Comment 3

[Brendan Eich [:brendan]]

Serge: Is there a return later on in the function, in a successor basic block to
the switch?

/be

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> Serge: Is there a return later on in the function, in a successor basic block to
> the switch?

I see what you mean... My mistake, no mystery/bug here :->

In <messageWindow.js>, there is not
{{
		switch (command)
		{
                 // various |case:|...

			default:
				return false;
		}
	},
}}

In <mail3PaneWindowCommands.js>, there is one
{{
    switch ( command )
    {
      // various |case:|...

      default:
        return false;
    }
    return false;
  },
}}


*****

Not this bug, but while we're talking about it:

Actually, this last |return false;| is "unreachable", as the |switch()| has
|return|s everywhere (not any |break;|).

Could the JS.Engine catch/warn this, or is it too complex/costy ?

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> 
> Actually, this last |return false;| is "unreachable", as the |switch()| has
> |return|s everywhere (not any |break;|).

Confirmation:
I removed it, and I now get this false bug warning too :->

Comment 6

[Brendan Eich [:brendan]]

Serge, unreachability in general requires more computation than the JS parser is
willing to throw at the task of generating a parse tree, with the strict warning
as a side effect only to the point where it would demand too much more
computation.  I think people will be happy if the patch for this bug gets
checked in; any other such bugs short of unreachability, please report ASAP.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 142564 [details] [diff] [review]
proposed fix, plus a few context-sensitive regexp scanning fixes

Shaver r='d over IRC with the proviso that I fix HasFinalReturn's return type
;-).

/be

Comment 8

[Brendan Eich [:brendan]]

Fix checked in.

/be