@Spoil ?

I noticed that over time again and again I find bugs (coded by me as well as lots of other people) in usage of EntityManager.merge() or similar methods (e.g. in my current large project using another O/R mapping framework).

The problem is that once you merged an object it is almost always a bug to re-use the same object instance.
You should instead use the return value.
http://java.sun.com/javaee/5/docs/api/javax/persistence/EntityManager.html#merge(T)

Note that it is somewhat similar to the class of "return value is not used" problems (e.g. on BigDecimal or String), but it actually differs:
In case of merge() et.al. it is ok not to use the return value at all if the object is not needed, but it is not ok to continue using parameter after the method call.

So what if there were an annotation to say this parameter is "@Spoiled" by the method call - it should no longer be used:
Do you think that's useful? Should I create a Jira issue?
Is something like that discussed in JSR 305? (There isn't a public draft, yet, but JetBrains is a member of the expert group, so maybe somebody can comment on this.)

14 comments

Stephen Friedrich wrote:

So what if there were an annotation to say this parameter is "@Spoiled" by the method call - it should no longer be used:
Do you think that's useful? Should I create a Jira issue?
Is something like that discussed in JSR 305? (There isn't a public draft, yet, but JetBrains is a member of the expert group, so maybe somebody can comment on this.)


Sounds good to me, and could be instantly applied to IDEA's OpenAPI for PsiElement.add() & friends.

Sascha

0

Nothing like @Spoiled has been proposed for JSR-305, but I can certainly do so. My guess is that it will be thought somewhat too ambitious for a standardization effort. Spoiling of parameters is a special case of the general problem of lifecycle/aliasing/ownership, and annotations for that are an open research problem. As a general rule, JSR-305 is mostly limiting itself to annotations that have been used in existing production tools. That's not to say that I don't really like the idea, and it might be close enough to @CheckReturnValue to slip in under the wire.

Questions:

If I have a method which takes an input stream, does a bunch of stuff with it, and then closes it, should the stream parameter be annotated as @Spoiled? (My answer: Hell yeah, that'll catch a lot of bugs!)

Should Stream.close() itself be annotated as @Spoiled, indicating that it makes it's target mostly-useless? Closable.close()? (My answer: see previous answer!)

And most importantly, should it be @Spoil, @Spoils, or @Spoiled? (Sorry, that's a JSR-305 in-joke.)

--Dave Griffith

0

could be instantly applied to IDEA's OpenAPI for PsiElement.add() & friends.

Preach it. I can't even begin to imagine how many bugs this would have saved me.

--Dave Griffith

0

More questions:

Would there be value in warning if a field becomes @Spoiled, even if that field is not subsequently used in the same method? That would prevent @Spoiled entities from being reused by other methods, at some cost of possible false positives. Spoiling an object stored in a field and immediately reassigning the field would be okay ( foo = entityManager.merge(foo);), as would spoiling a field in a method marked as @Spoiled.

0

You should google for "Substructural Type Systems" or look into Pierce's
book (http://www.cis.upenn.edu/~bcpierce/attapl/index.html).

The basic idea if we try to do it in Java is quite simple (don't
remember the original terminology though):

  • A type can have various type attributes. A stream can be

opened/closed, frame active/disposed.

  • If a certain method requires a frame in certain state, it should

annotate a parameter:

void readData(@Open InputStream is);
void initFrame(@Active Frame f);

  • A method can change type state (pseudo code):


void readData(@ChageState(@Open, @Closed) InputStream is);

  • Code highlighting should understand that after close() method

execution (in code flow sense) all @Open streams are @Closed:

@Open is = obtainStream();

readData(is);

//is is @Closed

readData(is); // error

  • It should also check that parameter state is either clearly defined or

not changed:


//error: is changes its state
void doSomething(@Open InputStream is) {
readData(is);
}

  • To help code analysis some state checking should be added:



boolean isOpen(@Verify(@Open) InputStream is);

void doSomething(InputStream is){
readData(is); //prohibited - "is" is not open


if (isOpen(is)) {
readData(is); //OK
}

}

-



Any brave soul for plugin? :)

0

>Any brave soul for plugin?

Hmm, this doesn't actually look that hard, except for requiring some light dataflow. Oh, well, I guess I had to cross that particular bridge eventually. A couple of questions/thoughts:

1) It looks like a cleaner solution would have a single state annotation for a class, taking an enum as a value (e.g. @StreamState(StreamState.CLOSED)). That way things are a bit more strongly typed.

2) If @Open or @StreamState where itself annotated by a @SubstructureAnnotation meta-annotation, all of this could be driven automagically.

3) This all becomes a lot less valuable with closures, since so many life-cycle issues that currently require programmer diligence will disappear into lifecycle-managing utility methods.

--Dave Griffith

0

JSR-305 is mostly limiting itself to annotations that have been used in existing production tools
Well, that means if there is indeed a brave soul who implements IDEA-12587 quickly enough, chances for inclusion in JSR-305 will be quite good ;)
Feel free to vote for it
http://www.jetbrains.net/jira/browse/IDEA-12587

Mike, concerning the complexity of even the simpler case that I described here, I doubt that we'll see an implementation for that in a commercial product any time soon. Also the complexity and sheer number of annotations to remember will probably hinder acceptance by programmers.

0

The main point of having structural subtype annotations in IDEA is to
let you create the annotations you need in your project, and not to
include all of them into IDEA.

0

Hmm, this doesn't actually look that hard, except for requiring some light dataflow.


You should try to persuade Maxim to open it a bit.

BTW, is data flow really needed for this one? I think it can be solved
with control flow only.

1) It looks like a cleaner solution would have a single state annotation for a class, taking an enum as a value (e.g. @StreamState(StreamState.CLOSED)). That way things are a bit more strongly typed.


but a bit more verbose. I think we should take a look at complete code
sample.

2) If @Open or @StreamState where itself annotated by a @SubstructureAnnotation meta-annotation, all of this could be driven automagically.


Indeed.

3) This all becomes a lot less valuable with closures, since so many life-cycle issues that currently require programmer diligence will disappear into lifecycle-managing utility methods.


I don't understand how closures will help you not to use a psiElement
after psiElement.delete();

0

Nothing like @Spoiled has been proposed for JSR-305, but I can certainly do so.


BTW, do you have any information about the progressing of JSR-305? Is any work on it still being done? And is it goint to be part of JavaSE 7?

According to their JCP page, the JSR is way behind schedule. I sent a similar question to the email address given on that page three weeks ago, but got no answer, so I don't know whether it is still alive at all.

Regards,
Jens

0

On 2007-05-03 15:13:44 +0400, Jens Voss <no_reply@jetbrains.com> said:

>> Nothing like @Spoiled has been proposed for JSR-305, but I can
>> certainly do so.


BTW, do you have any information about the progressing of JSR-305? Is
any work on it still being done? And is it goint to be part of JavaSE 7?

According to their JCP page, the JSR is way behind schedule. I sent a
similar question to the email address given on that page three weeks
ago, but got no answer, so I don't know whether it is still alive at
all.

Regards,
Jens


Well, it's not dead but evolving far too slow indeed. You may want to
look at (or even participate in)
http://groups.google.com/group/jsr-305?hl=en where all the discussions
are being held.

0

>BTW, is data flow really needed for this one? I think it can be solved
with control flow only.

stream.close();
stream = new FileInputStream();
stream.read();

>I don't understand how closures will help you not to use a psiElement
after psiElement.delete();

I don't, but some very large percentage of the sorts of bugs that this would find are misuses of paired operations on transient objects. That's one of the key use cases that closures will address.

--Dave Griffith

0

Dave Griffith wrote:

And most importantly, should it be @Spoil, @Spoils, or @Spoiled?
(Sorry, that's a JSR-305 in-joke.)


Is Spoil even the right word? I was just thinking @Invalidate or
something might be better. Although, having just read all the threads I
have @Spoil stuck in my head for this :)

Mark

0

At first I was thinking of @Taint(s|ed)? but then I saw that such an annotation is already planned to deal with Strings that may contain sql injection attempts.

I'd prefer @Spoil over @Invalidate simply because it's shorter and snappier.
From what I read from my dictionary and googled the usual meaning of "spoil" matches quite well:
http://www.thefreedictionary.com/spoil

I am not a native english speaker, so maybe there's a better word.

0

Please sign in to leave a comment.