483714 - Static analysis to warn/error when deprecated code is used

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre
Build Identifier: 

In bug 58904, we want to deprecate PR_NewLock() in favor of nsLock::NewLock() (and similarly for PR_Monitor/nsMonitor).  It would great to have a simple analysis that warned or error'd when it came across these uses.

Reproducible: Always

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ben T. pointed out that we already have an NS_DEPRECATED decl attribute that will result in warnings on at least gcc.  This should work for most cases.

The proposed analysis is intended to complement this attribute in two ways: (1) when the function is not entirely deprecated, but shouldn't be used outside of a particular code module/class (as in bug 58904); and (2) for code patterns that are too complicated to detect by just putting an NS_DEPRECATED attribute on a decl.

Comment 2

[Benjamin Smedberg]

Are you planning on writing the analysis? It doesn't sound hard, and I'd love to get more people involved with writing simple analysis passes.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes.  The first check I'm adding is quite trivial, so it'll be done in a few minutes.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Simple analysis, v1

I'll wait for review until I figure out how to integrate this into the build.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Benjamin: should I track this with bug 430328?  I feel almost guilty putting this next to "serious" analyses like outparams and stack, as this code is more like a rich grep.

Comment 6

[(dormant account)]

(In reply to comment #5)
> Benjamin: should I track this with bug 430328?  I feel almost guilty putting
> this next to "serious" analyses like outparams and stack, as this code is more
> like a rich grep.

yes. I have no intention of excluding simpler analyses. I think in the longer term simpler ones are the winning ones as they encourage people to get into analyzing their code.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Checker, v2

Since this "analysis" does no more at this point than essentially "grep -rn PR_Lock mozilla-central/* | grep -v nsLock", I'm going to wait until we have a more compelling use for seeking review etc.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Checker, v3

Updated to recommend newly-renamed API.

Comment 9

[(dormant account)]

Comment on attachment 371558 [details] [diff] [review]
Checker, v3

>diff --git a/config/static-checking-config.mk b/config/static-checking-config.mk
>--- a/config/static-checking-config.mk
>+++ b/config/static-checking-config.mk
>@@ -7,16 +7,17 @@ DEHYDRA_MODULES = \
>   $(topsrcdir)/xpcom/analysis/final.js \
>   $(topsrcdir)/layout/generic/frame-verify.js \
>   $(NULL)
> 
> TREEHYDRA_MODULES = \
>   $(topsrcdir)/xpcom/analysis/outparams.js \
>   $(topsrcdir)/xpcom/analysis/stack.js \
>   $(topsrcdir)/xpcom/analysis/flow.js \
>+  $(topsrcdir)/xpcom/analysis/deprecated-warn.js \
>   $(topsrcdir)/js/src/jsstack.js \
>   $(NULL)
> 
> DEHYDRA_ARGS = \
>   --topsrcdir=$(topsrcdir) \
>   --objdir=$(DEPTH) \
>   --dehydra-modules=$(subst $(NULL) ,$(COMMA),$(strip $(DEHYDRA_MODULES))) \
>   --treehydra-modules=$(subst $(NULL) ,$(COMMA),$(strip $(TREEHYDRA_MODULES))) \
>diff --git a/xpcom/analysis/deprecated-warn.js b/xpcom/analysis/deprecated-warn.js
>new file mode 100644
>--- /dev/null
>+++ b/xpcom/analysis/deprecated-warn.js
>@@ -0,0 +1,182 @@
>+/**
>+ * This analysis warns about uses of deprecated APIs or code patterns.
>+ */
>+
>+require ({ after_gcc_pass: 'cfg' });
>+
>+include ('gcc_print.js');
>+include ('gcc_util.js');
>+include ('treehydra.js');
>+include ('unstable/analysis.js');
>+include ('util.js');
>+
>+//-----------------------------------------------------------------------------
>+// Configuration
>+//
>+
>+/** Should instances of deprecated code be warnings or errors? */
>+let DEPRECATED_IS_ERROR = false;
>+
>+/** Warn when we come across nodes that DeprecatedCodeCheck doesn't
>+ * understand? */
>+let WARN_NYI = false;
>+
>+/** How much detail to see on PartialDeprecation checks. */
>+let TRACE_PARTIAL_DEPRECATED = 0;

s/let/const/ here.

>+
>+//-----------------------------------------------------------------------------
>+// Check dispatcher, with convenience functions.
>+//
>+function DeprecatedCodeCheck () {
>+    this.onFundeclDudes = [];
>+    this.onCallexprDudes = [];
>+}
>+DeprecatedCodeCheck.prototype = new Object;

don't need that

>+
>+DeprecatedCodeCheck.prototype.deprecate = function (why, node) {
>+    let msg = 'deprecated code: '+ why;
>+    let loc = node ? location_of (node) : undefined;
>+    if (DEPRECATED_IS_ERROR)
>+        error (msg, loc);
>+    else
>+        warning (msg, loc);
>+};
>+
>+DeprecatedCodeCheck.prototype.addListener = function (evt, handler) {
>+    this[evt+'Dudes'].push (handler);
>+};

to save some typing you can do
DeprecatedCodeCheck.prototype = {
deprecate: function(..) {},
addListener: function(..) {}
}


also, addListener smells too much like Java :)

Comment 10

[Nathan Froyd [:froydnj]]

I don't think we want to go through with something like this.