770825 - dataman.js reports 5 strict warnings

Status: NEW

(SeaMonkey :: Passwords & Permissions, defect, P4)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1341379736.1341382775.27840.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/07/03 22:28:56
{
JavaScript strict warning: chrome://communicator/content/dataman/dataman.js, line 697: anonymous function does not always return a value
JavaScript strict warning: chrome://communicator/content/dataman/dataman.js, line 1148: anonymous function does not always return a value
JavaScript strict warning: chrome://communicator/content/dataman/dataman.js, line 1774: anonymous function does not always return a value
JavaScript strict warning: chrome://communicator/content/dataman/dataman.js, line 2149: anonymous function does not always return a value
JavaScript strict warning: chrome://communicator/content/dataman/dataman.js, line 2484: anonymous function does not always return a value
}

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

These are all getCellText implementations with a switch statement that is lacking a default case. I wonder whether implementing the default case will fix the warnings (and whether "return null;" will do) or whether a return statement outside the switch is required for that.

Personally I'd probably just call gDataman.debugError() like in line 122.

Comment 2

[Robert Kaiser]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #1)
> These are all getCellText implementations with a switch statement that is
> lacking a default case.

Yes, all false positives in that we're just falling through and not returning anything in the default case. I dislike doing patches just to silence useless warnings, but we could add a default: line that just doesn't do anything. I don't want it to send an error when there's no real reason.

> Personally I'd probably just call gDataman.debugError() like in line 122.

Umm, that's where it's defined, not where it's used, right?

Also, the line numbers have been changed due to other patches.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Robert Kaiser (:[e-mail]) from comment #2)
> Yes, all false positives in that we're just falling through and not
> returning anything in the default case.

I think you either didn't get what the error message means, or chose to ignore it on purpose. The switch statements are just symptoms; the actual issue is that we have functions that sometimes return something and sometimes not. That's inconsistent and bad practice. Of course it's not a top priority, but I set Importance accordingly from the start.

> I dislike doing patches just to silence useless warnings, but we could add a
> default: line that just doesn't do anything.

/That/ would be useless! The warnings are there for a reason; what you propose is almost like a catch (e) {} which is the cause of numerous issues going unnoticed.

> I don't want it to send an error when there's no real reason.

If one of the mentioned switch statement enters the default case it means that no match was found for aColumn.id. That /is/ a real reason.

> > Personally I'd probably just call gDataman.debugError() like in line 122.
> 
> Umm, that's where it's defined, not where it's used, right?

Context from current c-c (trunk):

121       default:
122         gDataman.debugError("Unexpected change topic observed: " + aTopic);
123         break;

That's a call, not a definition. IOW, I don't know what you mean.

Comment 4

[Robert Kaiser]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #3)
> The switch statements are just symptoms; the actual
> issue is that we have functions that sometimes return something and
> sometimes not.

That's completely OK in those cases. The warnings are bogus, the code doesn't have errors. we can do an empty return if it makes you feel better but in JS not having a return statement or an empty one are defined in the standard as being the same, so it's the warnings that are in error, not the code. I think I now remember a blog post from someone very well-versed in JS, if not actually working on ECMAScript committees, complaining about this bogus warning.

> If one of the mentioned switch statement enters the default case it means
> that no match was found for aColumn.id. That /is/ a real reason.

It's a case that cannot happen in the code as written unless someone makes code changes and has no idea what he's doing. Given the recent changes to Data Manager in SeaMonkey, I admit that's not unlikely though, you and Philip have already added serious brokenness to the permissions UI anyhow - nothing connected to those select statements though. But as you don't even add tests for your code, I see how you care more about bogus warnings than real-world happenings there.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

1. Please read my referring to "error messages" as "warnings". Sorry for not having been clear enough there; I thought it was obvious. I know that the code in question is not broken, but IMHO it's not adhering to good programming practices either.
2. Do with this bug as you see fit (WONTFIX or something); we obviously disagree strongly about the way we think it should be fixed (or at all) and I don't like bad compromises. I don't feel it's worth my time going through this argument we're starting to have for a bug of so little importance. Maybe another time. ;-)