Home » Modeling » EMF » Generated extensions to Switch<T> are failing null analysis
 Generated extensions to Switch<T> are failing null analysis [message #1556934] |
Sat, 10 January 2015 16:56  |
 Erick Hagstrom
Messages: 107
Registered: April 2014
Location: USA |
Senior Member |
|
|
I'm using Luna, SR1, everything's up to date. In my Preferences, Java -> Compiler -> Errors/Warnings, under Null analysis, I have checked "Enable annotation-based null analysis".
My generated XxxSwitch.java has the following class declaration line:
public class XxxSwitch<T1> extends Switch<T1>
and all of the caseXyz() methods are declared as:
public T1 caseXyz(Xyz object) {
return null;
}
These declarations are as they have always been, but now, because we have type annotations for nullness, I get either a Warning or an Error on the null returned from the caseXyz() methods. (It's either a Warning or an Error because I get to choose in the Preferences how the compiler flags the issue.)
Seems like there are four things I can do:
1) Mod the generated code as
public class XxxSwitch<@Nullable T1> extends Switch<T1>
which I am reluctant to do because that messes up Switch regeneration.
2) Turn off null analysis, which I am reluctant to do because I find it useful.
3) Fork the SwitchClass.javajet template to output something friendlier to null analysis, which I am reluctant to do because I find the updates useful.
4) Make the compiler spit out a Warning for nullness violations so I can at least compile and get on with my life. I picked this one.
It seems like there are two things that could be done to fix the template:
1) Make it declare the type parameter as @Nullable (and drag in the necessary import). This would have the disadvantage of disallowing the use of @NonNull types when instantiating the XxxSwitch.
2) Establish a well-known return value (something that isn't null) and make the doSwitch recognize it instead of null as the "no result, keep trying" indicator. I personally prefer this one.
Given that Luna was advertised as supporting Java 8 in general and type annotations in particular, this behavior seems like a bug. Am I holding it wrong, or should this indeed be corrected?
Thanks as always for your time and efforts.
--Erick
|
|
|
| Re: Generated extensions to Switch<T> are failing null analysis [message #1557254 is a reply to message #1556934] |
Sat, 10 January 2015 21:17  |
Ed Willink
Messages: 7655
Registered: July 2009 |
Senior Member |
|
|
Hi
I suspect that you have enabled the ability to have default annotations
for unspecified annotations.
I have been using the Java 7 annotations for nearly a couple of years
and have not seen this problem, but I do not allow the annotations to
auto-annotate. I define everything manually, or via quickfixes.
There was a talk at EclipseCon Europe on the auto-annotations and to be
honest I really didn't understand it; there seemed to be just too many
subtle exceptions. So I resolved to carry on only having those
annotations that I specified. This will probably solve your problem.
You will discover that although EMF has no support for null annotations,
JMerge treats annotations as user comments and so any annotations that
you add manually are sticky across regeneration.
Regards
Ed Willink
On 10/01/2015 16:56, Erick Hagstrom wrote:
> I'm using Luna, SR1, everything's up to date. In my Preferences, Java ->
> Compiler -> Errors/Warnings, under Null analysis, I have checked "Enable
> annotation-based null analysis".
>
> My generated XxxSwitch.java has the following class declaration line:
>
> public class XxxSwitch<T1> extends Switch<T1>
>
> and all of the caseXyz() methods are declared as:
>
> public T1 caseXyz(Xyz object) {
> return null;
> }
>
>
> These declarations are as they have always been, but now, because we
> have type annotations for nullness, I get either a Warning or an Error
> on the null returned from the caseXyz() methods. (It's either a Warning
> or an Error because I get to choose in the Preferences how the compiler
> flags the issue.)
>
> Seems like there are four things I can do:
>
> 1) Mod the generated code as
> public class XxxSwitch<@Nullable T1> extends Switch<T1> and change the
> class-level @generated to @generated NOT,
> which I am reluctant to do because that messes up Switch regeneration.
>
> 2) Turn off null analysis, which I am reluctant to do because I find it
> useful.
>
> 3) Fork the SwitchClass.javajet template to output something friendlier
> to null analysis, which I am reluctant to do because I find the updates
> useful.
>
> 4) Make the compiler spit out a Warning for nullness violations so I can
> at least compile and get on with my life. I picked this one.
>
> It seems like there are two things that could be done to fix the template:
>
> 1) Make it declare the type parameter as @Nullable (and drag in the
> necessary import). This would have the disadvantage of disallowing the
> use of @NonNull types when instantiating the XxxSwitch.
>
> 2) Establish a well-known return value (something that isn't null) and
> make the doSwitch recognize it instead of null as the "no result, keep
> trying" indicator. I personally prefer this one.
>
> Given that Luna was advertised as supporting Java 8 in general and type
> annotations in particular, this behavior seems like a bug. Am I holding
> it wrong, or should this indeed be corrected?
>
> Thanks as always for your time and efforts.
> --Erick
|
|
|
| Re: Generated extensions to Switch<T> are failing null analysis [message #1557348 is a reply to message #1557254] |
Sat, 10 January 2015 22:32 |
Erick Hagstrom
Messages: 107
Registered: April 2014
Location: USA |
Senior Member |
|
|
Thanks for the quick response, Ed, but that's not it at all. This is related to the new Java 8 type annotations. Oddly enough, if you leave a type parameter unconstrained, it can't ever be assigned null, because the compiler has to allow the instantiator to specify a @NonNull type. This causes a great deal of head scratching at first blush. Please refer to the Luna built-in help under Java development user guide -> Tasks -> Improving Java code quality -> Using null type annotations -> Generics -> Type variables. It says, in part, "A type variable corresponding to an unconstrained type parameter requires pessimistic checking in order to guarantee safety with all legal substitutions: this type can neither be assumed to be nullable nor nonnull." So one can never return a null from a method returning a type defined by a generic type variable that doesn't explicitly specify @Nullable. 
class C<T extends Number> {
int consume(T t) {
return t.intValue(); // NOT OK since T could be nullable
}
T provide() {
return null; // NOT OK since T could require nonnull
}
}
Regarding regeneration:
1) I neglected to mention that I'm using xcore.
2) If I add @Nullable to my class declaration as "public class XxxSwitch<@Nullable T1> extends Switch<T1>", the @Nullable annotation is NOT preserved through regeneration.
3) If I Project -> Clean... I run into all kinds of trouble, as the entire src-gen folder is wiped. And if I try to put my XxxSwitch.java into the src folder, well, I still get another generated in src-gen, and that's obviously not going to fly. I imagine there are some Properties I can set to protect modified generated code, but I haven't invested the time to figure that part out. (But that's a digression from this thread. I only mentioned it because I was trying to verify your statement about sticky annotations.)
So perhaps Java 7 annotations are sticky, but Java 8 type annotations are not?
|
|
|
| Re: Generated extensions to Switch&lt;T&gt; are failing null analysis [message #1558133 is a reply to message #1557348] |
Sun, 11 January 2015 09:15 |
Ed Merks
Messages: 33141
Registered: July 2009 |
Senior Member |
|
|
Erick,
Comments below.
On 10/01/2015 11:32 PM, Erick Hagstrom wrote:
> Thanks for the quick response, Ed, but that's not it at all. This is
> related to the new Java 8 type annotations.
There isn't even support yet for things specific to Java 6, i.e.,
@Override for implementations of interface methods.
> Oddly enough, if you leave a type parameter unconstrained, it can't
> ever be assigned null, because the compiler has to allow the
> instantiator to specify a @NonNull type.
This all comes down to compiler preferences.
> This causes a great deal of head scratching at first blush. Please
> refer to the Luna built-in help under Java development user guide ->
> Tasks -> Improving Java code quality -> Using null type annotations ->
> Generics -> Type variables. It says, in part, "A type variable
> corresponding to an unconstrained type parameter requires pessimistic
> checking in order to guarantee safety with all legal substitutions:
> this type can neither be assumed to be nullable nor nonnull." So one
> can never return a null from a method returning a type defined by a
> generic type variable that doesn't explicitly specify @Nullable. :(
Yes, that's kind of a new thing obviously.
>
> class C<T extends Number> {
> int consume(T t) {
> return t.intValue(); // NOT OK since T could be nullable
> }
> T provide() {
> return null; // NOT OK since T could require nonnull
> }
> }
>
>
>
> Regarding regeneration:
> 1) I neglected to mention that I'm using xcore.
In the end, Xcore just maps to an Ecore model and a GenModel and
generation is driven from that...
> 2) If I add @Nullable to my class declaration as "public class
> XxxSwitch<@Nullable T1> extends Switch<T1>", the @Nullable annotation
> is NOT preserved through regeneration.
That depends on how you've set things up. Yes, by default things end up
in the src-gen folder and those are completely deleted and regenerated.
But via @GenModel annotations you can direct the model directory
property to be the src folder and then "normal" merging is supported.
> 3) If I Project -> Clean... I run into all kinds of trouble, as the
> entire src-gen folder is wiped. And if I try to put my XxxSwitch.java
> into the src folder, well, I still get another generated in src-gen,
> and that's obviously not going to fly.
As mentioned above, that's just the default.
> I imagine there are some Properties I can set to protect modified
> generated code, but I haven't invested the time to figure that part
> out. (But that's a digression from this thread. I only mentioned it
> because I was trying to verify your statement about sticky annotations.)
>
> So perhaps Java 7 annotations are sticky, but Java 8 type annotations
> are not?
No, it depends on where you generate too. Generating to the configured
(to src-gen by default via the project properties) generation target
will produce the behavior you describe above. Generating elsewhere,
i.e., to src will give you the "normal" EMF merging behavior. Of
course you can set JDT compiler options on your project as well.
In the end, there's little point all these extra analysis checks for
generated code; generated code is generally either consistently always
correct or the opposite and will often not conform to your
personal/project ideals.
Ed Merks
Professional Support: https://www.macromodeling.com/
|
|
| |
| Re: Generated extensions to Switch<T> are failing null analysis [message #1569826 is a reply to message #1556934] |
Sat, 17 January 2015 18:41 |
Erick Hagstrom
Messages: 107
Registered: April 2014
Location: USA |
Senior Member |
|
|
Thank you, Ed and Ed. Very insightful.
I agree that null analysis of generated code is of no practical value. Either the generator generates sound code or it doesn't. And in the case of EMF, it does.
This doesn't mean that null analysis has no value in such projects. There are two ways it can be useful that occur to me at the moment.
First, EMF code is designed to be tolerant of hand-written overrides. In fact, the Switch extensions pretty much have to be extended somewhere, or the class using it will never get a meaningful result. It can be useful to verify statically that unexpected nulls are never returned due to hand-written code in a generated class.
Second, in order to make null analysis as useful as it might be, it is important to have it turned on upstream of where you actually need it to work. Code that I have written in other related plug-ins that uses my EMF-generated code can't take advantage of any potential guarantees that null-checked generated code might provide. When I want a value obtained from generated code to carry a @NonNull type, I have to declare it as @Nullable, then assert that it isn't null rather than just relying on the generated code to propagate null-checked types automatically. That's mildly annoying but not a deal-breaker.
Despite that, I can't say that this is a big deal. We've all written a lot of code without null analysis and have managed pretty well so far. And unchecked legacy code will be with us for a very long time, I'm sure, so I can't honestly call this a major issue. Plus, it's not like we have a lot of EMF contributors sitting around wishing they could find something useful to do. And, frankly, I'd rather see you working on cool new functionality rather than trying to shoehorn in a new Eclipse use of a new Java feature that, somewhere in that chain, probably wasn't implemented in the best possible way. Or maybe should have been left for another language.
Anyway, I've just turned off null analysis for my EMF generation plug-ins. Problem solved. Thanks again for your considered responses.
And maybe, if I can ever get my employer to submit a CLA, I might be able to help out somehow. We'll see.
|
|
|
| Re: Generated extensions to Switch<T> are failing null analysis [message #1570882 is a reply to message #1569826] |
Sun, 18 January 2015 09:08 |
Ed Merks
Messages: 33141
Registered: July 2009 |
Senior Member |
|
|
Erick,
Comments below.
On 17/01/2015 7:41 PM, Erick Hagstrom wrote:
> Thank you, Ed and Ed. Very insightful.
>
> I agree that null analysis of generated code is of no practical value.
> Either the generator generates sound code or it doesn't. And in the
> case of EMF, it does.
>
> This doesn't mean that null analysis has no value in such projects.
That's true. There can be hand written code as well.
> There are two ways it can be useful that occur to me at the moment.
>
> First, EMF code is designed to be tolerant of hand-written overrides.
> In fact, the Switch extensions pretty much have to be extended
> somewhere, or the class using it will never get a meaningful result.
> It can be useful to verify statically that unexpected nulls are never
> returned due to hand-written code in a generated class.
Indeed.
>
> Second, in order to make null analysis as useful as it might be, it is
> important to have it turned on upstream of where you actually need it
> to work. Code that I have written in other related plug-ins that uses
> my EMF-generated code can't take advantage of any potential guarantees
> that null-checked generated code might provide. When I want a value
> obtained from generated code to carry a @NonNull type, I have to
> declare it as @Nullable, then assert that it isn't null rather than
> just relying on the generated code to propagate null-checked types
> automatically. That's mildly annoying but not a deal-breaker.
I agree.
>
> Despite that, I can't say that this is a big deal. We've all written a
> lot of code without null analysis and have managed pretty well so far.
And you need Java 8 (I think for all this), and so far we don't even
generated the right annotations for Java 1.6, so quite a bit of work to
deal with all these language-level-specific issues. One day I'll get to
that...
> And unchecked legacy code will be with us for a very long time, I'm
> sure, so I can't honestly call this a major issue. Plus, it's not like
> we have a lot of EMF contributors sitting around wishing they could
> find something useful to do. And, frankly, I'd rather see you working
> on cool new functionality rather than trying to shoehorn in a new
> Eclipse use of a new Java feature that, somewhere in that chain,
> probably wasn't implemented in the best possible way. Or maybe should
> have been left for another language.
Hehehe.
>
> Anyway, I've just turned off null analysis for my EMF generation
> plug-ins. Problem solved. Thanks again for your considered responses.
>
> And maybe, if I can ever get my employer to submit a CLA, I might be
> able to help out somehow. We'll see.
That would be cool. :-) Thanks for being understanding!
Ed Merks
Professional Support: https://www.macromodeling.com/
|
|
|
Goto Forum:
Current Time: Thu Apr 25 23:36:51 GMT 2024
Powered by FUDForum. Page generated in 0.03649 seconds |
] 
Search
Help
Register
Login
Home
