Proposal for *warn-on-reflection* to be true by default

230 views
Skip to first unread message

Andy Fingerhut

unread,
Feb 25, 2012, 1:49:26 PM2/25/12
to cloju...@googlegroups.com
Questions I was hoping to get a response to about the ideas below:

+ Does something like this sound desirable?
+ Is it worth creating a wiki page to document design ideas?

Please correct me if I've got any of this wrong, but it is my impression that using (set! *warn-on-reflection* true) at the beginning of a file, or the Leiningen :warn-on-reflection key, is somewhat annoying, in the following way.

While it helps you find reflection warnings, typically you fix the ones that you want to fix, and then there are often warnings left over that you either don't know how to fix, or you consider that they are necessary uses of reflection, or the code isn't performance critical so you leave it as is.

However, you don't want to see those handful of warnings every time you compile or test, because then it makes you accustomed to seeing a handful of warnings, and you would rather that warnings indicate something important to investigate, not "noise" to ignore. So you remove the (set! *warn-on-reflection* true) or :warn-on-reflection before checking in code. It requires a conscious effort to turn them back on later if you want to look for new reflection warnings in your code, and then you will also typically revisit the ones you saw in the past, too.

I've got my own local Clojure tree where I've tried out the following experiment (I'm happy to publish the code somewhere, e.g. a github fork, if there is interest):

Change the default value of *warn-on-reflection* to true in the compiler.

Create a new expression (known-reflection ...) that behaves identically to (do ...), except that it sets *warn-on-reflection* to false within its scope.

The idea is that reflection warnings are always on, everywhere, except where you explicitly turn them off, either through setting *warn-on-reflection* to false, wrapping an expression in (known-reflection), or using the Java system property before invoking the JVM. The preferred method for production code is to eliminate the reflection warning, or if you want to keep the warning code unchanged, wrap it in (known-reflection ...). If it is test code, a blanket use of (set! *warn-on-reflection* false) near the beginning of the file seems appropriate to me. That can be commented out temporarily if you want to try to optimize your tests.


Run "ant" to build and test.

Insert type hints, uses of known-reflection around individual expressions, or in a few cases (primarily in test files) add (set! *warn-on-reflection* false) before code that has lots of reflection warnings.

From some quick grep'ing through my diffs, I estimate that I've added:

37 type hints (my grep regexp here might be a little off)

27 uses of known-reflection

4 bracketed pairs of (set! *warn-on-reflection* false) followed shortly later by (set! *warn-on-reflection true) around a few functions grouped together that had reflection warnings that seemed either necessary, or would require too many sprinklings of (known-reflection ...) to do it more surgically.

9 uses of (set! *warn-on-reflection* false) that are in effect from that point until the end of the file, all in test files where performance is less of a concern, and where I did not want to unintentionally modify the intent of the test. That might be the best policy for test code in general, especially in Clojure core tests, because some of them are specifically testing for whether type hints make a difference.

I can go on to see what the effect would be on particular contrib libraries if there is interest, but given their relative sizes to the Clojure code, I suspect the number of warnings, and changes required to fix them, would be significantly smaller.

Andy


David Santiago

unread,
Feb 25, 2012, 2:11:12 PM2/25/12
to cloju...@googlegroups.com
If I'm understanding you right, another approach to this is the
"check" task coming in leiningen 2. When you run lein check, it will
compile all the files in your project with warn-on-reflection turned
on, so that you see all the reflection warnings, with no need to
reference warn-on-reflection in your code or project file. This way
you don't have to do the dance where you turn it on in your
code/project.clj and then turn it off again after you see the results.

David

> --
> You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
> To post to this group, send email to cloju...@googlegroups.com.
> To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.
>

Andy Fingerhut

unread,
Feb 25, 2012, 2:18:36 PM2/25/12
to cloju...@googlegroups.com, cloju...@googlegroups.com
That is nice.

An additional advantage of the proposed (known-reflection ...) form is that you can surgically disable warnings permanently, for single expressions. That way, you can focus on new warnings without having to sift through old ones, too.

Andy

Sent from my iPhone

Hugo Duncan

unread,
Feb 25, 2012, 8:44:56 PM2/25/12
to cloju...@googlegroups.com
Andy Fingerhut <andy.fi...@gmail.com> writes:

> An additional advantage of the proposed (known-reflection ...) form is
> that you can surgically disable warnings permanently, for single
> expressions. That way, you can focus on new warnings without having
> to sift through old ones, too.

known-reflection sounds useful in it's own right, independent of the
*warn-on-reflection* default, but I also would argue that tool support
is a better way of making sure reflection warnings are handled.

For interactive code development, I think the reflections warnings would
quickly become intrusive.

Hugo

Phil Hagelberg

unread,
Feb 25, 2012, 10:34:39 PM2/25/12
to cloju...@googlegroups.com
Hugo Duncan <dunca...@gmail.com> writes:

>> An additional advantage of the proposed (known-reflection ...) form is
>> that you can surgically disable warnings permanently, for single
>> expressions. That way, you can focus on new warnings without having
>> to sift through old ones, too.
>
> known-reflection sounds useful in it's own right, independent of the
> *warn-on-reflection* default, but I also would argue that tool support
> is a better way of making sure reflection warnings are handled.

Isn't known-reflection just (binding [*warn-on-reflection* false] ~@body) ?

> For interactive code development, I think the reflections warnings would
> quickly become intrusive.

As someone who has spent three years in Clojure and only first turned on
reflection warnings a few weeks ago, I heartily agree.

-Phil

Alan Malloy

unread,
Feb 26, 2012, 2:43:26 AM2/26/12
to Clojure Dev
On Feb 25, 7:34 pm, Phil Hagelberg <p...@hagelb.org> wrote:
> Isn't known-reflection just (binding [*warn-on-reflection* false] ~@body) ?

No, because the warnings occur when the form is compiled, and the
binding is not in place until it is run.

Andy Fingerhut

unread,
Feb 26, 2012, 5:14:17 PM2/26/12
to cloju...@googlegroups.com
On Feb 25, 2012, at 7:34 PM, Phil Hagelberg wrote:

> Hugo Duncan <dunca...@gmail.com> writes:
>
>> For interactive code development, I think the reflections warnings would
>> quickly become intrusive.
>
> As someone who has spent three years in Clojure and only first turned on
> reflection warnings a few weeks ago, I heartily agree.

I agree that changing Clojure to have reflection warnings on by default is a bad idea. I had not considered how annoying it would be to enable them for interactive use, and was not aware how easy it is with Leiningen and ant to turn them on or off for a whole project.

For Clojure, uncommenting the following line in build.xml enables reflection warnings when compiling Clojure, but not the tests.

<!-- <sysproperty key="clojure.compile.warn-on-reflection" value="true"/> -->

With 30 type hints (eliminating reflection), 20 additions of known-reflection, and one blanket use of:

(set! *warn-on-reflection* false)

near the beginning of pretty_writer.clj, there are no reflection warnings when compiling Clojure. I believe there is a good way to eliminate the uses of reflection in pretty_writer.clj, not just silence the warnings, but I'm still thinking about a satisfying way to do it.

If you want to see any of these changes, I've created a patch [1] that contains 3 separate commits, described below.

[1] http://homepage.mac.com/jafingerhut/files/clojure/reflect-warn-patches-v1.txt

Andy


Commit 1: Add type hints to avoid reflection

Adds 30 type hints. All of them eliminate some use of reflection in the code. They should be double-checked by someone else, in case I've added an incorrect one. All Clojure tests pass.


Commit 2: Add special form known-reflection to Clojure compiler

It is identical to do, except that *warn-on-reflection* is false within its scope.


Commit 3: Add uses of known-reflection to silence all current reflection warnings

The reflection is still there, of course, just not warned about, even when you enable *warn-on-reflection* at a file level.

File pretty_writer.clj currently has a blanket (set! *warn-on-reflection* false). There are ways to eliminate the reflection, not merely silence it, but it would be better if it could be made syntactically cleaner.

P.S.: One motivation for a relatively long name like known-reflection is making it easy to search source code for uses of it. This is handy if you want to review the code and find uses of reflection that you want to try to eliminate.

Another idea would be to have another Java property like clojure.compile.warn-on-reflection that enables reflection warnings even inside of known-reflection expressions. I haven't implemented that.

Reply all
Reply to author
Forward
0 new messages