The hint checks that the logger variable is both static and final - there shouldn't be any problem with serializing a class that has a static final Logger, AFAIK. In almost all cases, not having the Logger static is a mistake, and for the cases where it is intentional to have it as an instance variable, I would suggest either using @SuppressWarnings("NonConstantLogger") or disable the hint.

The JavaDoc for Logger does not indicate that it implements Serializable.  Therefore, if a serializable class has a Logger, it must be transient, not final.

Is this too hard to implement?  It sound like this hint need to be removed since it doesn't handle all appropriate cases correctly.

Sorry, but I do not understand. The hint checks that the field is static (and final). Static fields are not serialized. So if the field is static, there is no problem with having it final, even for Serializable classes. Is there a widely use pattern/usecase where the logger is non-static?

Thanks.

Okay, maybe I am missing something here.  If it is static and final, and not serialized, then when the class is reloaded, the static final field will be reconstructed correctly, and therefore readObject() is not required to reinitialize the field?  I am not thinking about static final correctly if so.  I have an old codebase without "static final" Logger instances which uses readObject() to initialize the "transient" Logger instances.

Also, another point is that inner classes also can not have static field references in them.  In several cases, the inner classes in this codebase have

private Logger log = Logger.getLogger( getClass().getName().replace("$",".") );

in them, and the hint action in that cases creates a declaration that indicates an error that inner classes can not have static fields.

(In reply to comment #4)
> Okay, maybe I am missing something here.  If it is static and final, and not
> serialized, then when the class is reloaded, the static final field will be
> reconstructed correctly, and therefore readObject() is not required to
> reinitialize the field?  I am not thinking about static final correctly if so. 

For class like this:
public class T extends Serializable {
    private static final Logger LOG = ...;
    private int i;
}

the serialized format of the instance will not contain the reference to the logger, as it is a class' field, not the instance's field. The static initializer of the class (which initializes the static fields) is run after the bytecode for the class is loaded and before any instances are created. Not sure what exactly is meant by "reloading the class", but technologies I know that allow to reload the class' bytecode do no re-run static initializers. But I do not see the connection with serialization.

> I have an old codebase without "static final" Logger instances which uses
> readObject() to initialize the "transient" Logger instances.

I can understand that, but doing that is still, to the best of my knowledge, considered a bad practice (do you really want the logger to be an instance field? What is the usecase?). And the hint is written exactly to point out this bad practice. So the only options I see are:
-delete the hint, so that it does not warn about bad practices (which in turn means that we may delete virtually any hint for similar reasons)
-disable the hint when working with a legacy system that you cannot change
-update the code not to use bad practices

> 
> Also, another point is that inner classes also can not have static field
> references in them.  In several cases, the inner classes in this codebase have
> 
> private Logger log = Logger.getLogger( getClass().getName().replace("$",".") );
> 
> in them, and the hint action in that cases creates a declaration that indicates
> an error that inner classes can not have static fields.

I will fix the hint to check this case and handle it differently, but it is, IMO, still a poor practice to define a logger that is not static (do you really want each of the classes to have its own reference to the logger?). So the warning should remain, IMO. Solutions are either to move the field into the outter class or make the innerclass static.

I can fix the outer class level loggers.  I was just not thinking about the class level static field initializations, and I feel pretty dumb about it at this point.

Regarding the inner class issue.  I have several macros/abbreviations which insert code using "log.XXXX" references, so that the Logger instance is always called "log".  Thus, for inner classes, I also need a field named "log", but in some cases, the inner class is "verbose" enough that I want to control it's logger specifically using a different Logger instance.

So, I am not sure how to "cleanly" deal with this issue either.  But, having the hint at least not bark about inner class instances not being "static", and then inserting "static" using the "fix" that then causes an error to appear would be appreciated.

One of the things which occurs to me, is to do something like


public class Foo {


>>> private static final Logger _Bar_logger = 
        Logger.getLogger( Bar.getClass().getName().replace("$",".") );
    private class Bar {
        private final Logger log = _Bar_logger;

    }

}

This would keep tones of log instances from being created, and would allow the "log" instance name to be used.

The name of the logger instance, i.e. 'log', would be the problematic point.  You could use a code template that would use "log" but leave the cursor on "log" for the user to type their own desired name.

I created a couple of code templates just now that do make this easy to do from the start.

ilogger: creates the static logger using the inner class name with replacing '$' with '.'.

   private static final Logger _${Cls}_log = 
	Logger.getLogger( ${Cls}.class.getName().replace("$",".") );

ilog: creates the 'log' variable.

   private final Logger log = _${Cls}_log;

so if the hint can correct by providing something close to these two edits that would work for me.

For inner classes, the hint is always about an existing Logger declaration, and thus you already know the inner class name, and the Logger variable name, so I think you should be able to make this kind of edit with only a check for an existing variable by the same name for the static final instance in the outer class, and just have a renaming strategy to pick a different name if there is a collision?

The problem with innerclasses should be gone:
http://hg.netbeans.org/jet-main/rev/cac699bacf68

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/cac699bacf68
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #202795: do not show the not-final static Logger hint for nested classes.

Thanks for the work on this issue.  There could be another hint provided if a Logger instance is not assigned a "static final" value as my two code templates show in that pattern.  

1) Not complaining, at all, does still leave the door opened for unintended problems with lots of Logger instances being created.

2) Providing the fix that my templates illustrate, would still be a good option I believe.

(In reply to comment #11)
> Thanks for the work on this issue.  There could be another hint provided if a
> Logger instance is not assigned a "static final" value as my two code templates
> show in that pattern.  

"could be"=> did you mean to create an enhancement for that? Or what is the justification for a defect?

> 
> 1) Not complaining, at all, does still leave the door opened for unintended
> problems with lots of Logger instances being created.

Actually, Logger.getLogger does not create a new instances. It is more a matter of a good style to have it static final (in a top-level class).

> 
> 2) Providing the fix that my templates illustrate, would still be a good option
> I believe.

I don't think so. I am not aware of any prevalent usecase that would require loggers on nested classes. Generally adding loggers to nested classes seems like a poor practice to me (there may be edge cases where it is justified, but I am not aware about any generally useful and widely used usecases - are there any?), and so the IDE should not lead the users in this direction. I can add a new warning that will complain about any loggers in nested classes, if you prefer.

I wanted to have a bit more conversation on the issue before deciding it was closed and/or we need an enhancement.

First, I often have inner classes which have significant logic and I want to be able to use the logger name to indicate the source of the log statements.  In some cases, I have multiple instances of an interface implementation as inner classes which I need to be able to tell which instance is being used, as they may also have a common abstract class and so I need to know more about what is driving the logging messages.  The logger name is important for these purposes.  I have a formatter that generates lines of the form

date [logger name] message

and having the name in there helps me to find exactly what I need in larger log files because I can just search for the name of the logger in the editor or from the command line with grep etc.

It seems that my use of logging is different than what you've seen or use.  I'm fine with leaving it as you've done it here, because it removes the warning which can't be satisfied by the suggested means.